Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PSQLBackendMessageEncoder #175

Merged
merged 2 commits into from
Sep 18, 2021

Conversation

fabianfett
Copy link
Collaborator

Motivation

To test a PSQLChannelHandler, that uses an internal NIOSingleStepByteToMessageDecoder, in an EmbeddedChannel we need to writeInbound bytes. To make this easier, this PR introduces a PSQLBackendMessageEncoder as a test util

Changes

  • Add PSQLBackendMessageEncoder
  • Use PSQLBackendMessageEncoder in authentication tests

Note

This pr only changes test code

@fabianfett fabianfett added enhancement New feature or request part of a series labels Sep 18, 2021
@fabianfett fabianfett requested a review from gwynne September 18, 2021 12:22
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm neither approving nor requesting changes for now on this one - I'm not convinced any of my comments are really worth spending effort on, but I'd like to get the sense of your thoughts before moving on.

func encode(data message: PSQLBackendMessage, out buffer: inout ByteBuffer) throws {
switch message {
case .authentication(let authentication):
buffer.writeBackendMessageID(message.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing specifically wrong with having it this way, but it nags at me to have the same line repeated in (almost) every single case like this over and over. Is there any not-horrible way to factor it out? (I can't think of something off the top of my head, and it's certainly not worth spending any amount of time on.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transformed into one liners.

@fabianfett fabianfett requested a review from gwynne September 18, 2021 13:05
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@fabianfett fabianfett merged commit 8b13752 into vapor:main Sep 18, 2021
@fabianfett fabianfett deleted the ff-add-psqlbackendmessageencoder branch September 18, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants