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 PSQLFrontendMessageDecoder #178

Merged
merged 1 commit into from Sep 19, 2021

Conversation

fabianfett
Copy link
Collaborator

Motivation

In tests we would like to use EmbeddedChannel, if we decode and encode in the PSQLChannelHandler using the NIOSingleStepByteToMessageDecoder, we need to decode messages the handler wrote to the channel from bytes.

Changes

  • Add PSQLFrontendMessageDecoder
  • Add ReverseByteToMessageHandler

Notes

The changes are mostly in the test target. Enabling easier tests in the future.

@fabianfett fabianfett added enhancement New feature or request part of a series labels Sep 18, 2021
@fabianfett fabianfett force-pushed the ff-add-PSQLFrontendMessageDecoder branch from fc1ad29 to 8efa8b4 Compare September 18, 2021 14:28
@fabianfett fabianfett closed this Sep 18, 2021
@fabianfett fabianfett force-pushed the ff-add-PSQLFrontendMessageDecoder branch from 8efa8b4 to ce57b02 Compare September 18, 2021 14:31
@fabianfett fabianfett reopened this Sep 18, 2021
}

if self.isInStartup {
guard buffer.readableBytes >= 4 else {
Copy link
Member

Choose a reason for hiding this comment

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

This and the other guards on readableBytes are redundant; getInteger(at:as:) already does this:

        guard let range = self.rangeWithinReadableBytes(index: index, length: MemoryLayout<T>.size) else {
            return nil
        }

So for that read you can just check that result for nil instead of having a separate check. All other read*() and get*() methods also do this, so you can simplify and get rid of the !s.

This all being said, I very, very vaguely once recall there possibly having been a reason not to do that... I have no idea what it was or if it was a real thing and not my fevered imagining, but if so, please tell me 😰

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.

Just the one commentary on dealing with the ByteBuffer, if there's a reason to leave it as is I'm not gonna try to argue the point 🙂

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 419c20b into vapor:main Sep 19, 2021
@fabianfett fabianfett deleted the ff-add-PSQLFrontendMessageDecoder branch September 19, 2021 19:20
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.

None yet

2 participants