Skip to content

State machine #135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 33 commits into from
Feb 24, 2021
Merged

State machine #135

merged 33 commits into from
Feb 24, 2021

Conversation

@fabianfett
Copy link
Collaborator

@fabianfett fabianfett commented Jan 29, 2021 •
edited
Loading

Motivation

Connection state changes can be modeled with state machines. This PR introduces a ConnectionStateMachine that models the PostgresConnection's state. This will improve the Unit-testability of this package.

Changes

  • This PR does not change any public API
  • It replaces the underlying implementation of PostgresConnection with a new PSQLConnection
  • All new classes and structs that support the new PSQLConnection live in the /New folder
  • The PSQLConnection is mainly implemented using these structs and classes:
    • PSQLFrontendMessage a new enum modeling outgoing client messages to the server
    • PSQLBackendMessage a new enum modeling incoming server messages to the client
    • PSQLFrontendMessage.Encoder a new MessageToByteEncoder
    • PSQLBackendMessage.Decoder a new ByteToMessageEncoder
    • ConnectionStateMachine with the sub state machines:
      • AuthenticationStateMachine
      • CloseStateMachine
      • ExtendedQueryStateMachine
      • PrepareStatementStateMachine
    • PSQLChannelHandler is the main ChannelDuplexHandler for the PSQLConnection
    • PSQLEventsHandler is a new ChannelHandler that observes the channel events and closes the connection when appropriate.
    • PSQLEncodable is a new protocol to convert Swift types to Postgres types.
    • PSQLDecodable is a new protocol to convert Postgres types to Swift types.
    • PSQLEncodable and PSQLDecodable defer the encoding and decoding as much as possible, saving unnecessary allocations and conversions.
  • Thanks to these changes extended queries and prepared statements will now take the same code paths. Thus we ensure that code improvements for one query type will also lead to improvements for the other.

Result

We get a better testable PostgresConnection.

@fabianfett fabianfett force-pushed the ff-state-machine branch 12 times, most recently from 82a37e9 to 40625a3 Compare February 2, 2021 09:07
@fabianfett fabianfett mentioned this pull request Feb 2, 2021
Closed
@fabianfett fabianfett force-pushed the ff-state-machine branch 3 times, most recently from 1236d8f to 9b8e347 Compare February 3, 2021 08:46
@fabianfett fabianfett force-pushed the ff-state-machine branch 8 times, most recently from b44ab64 to 9afae59 Compare February 4, 2021 13:31
@MrMage MrMage requested review from MrMage and removed request for MrMage February 5, 2021 10:50
gwynne
gwynne requested changes Feb 6, 2021
View reviewed changes
@fabianfett fabianfett marked this pull request as ready for review February 22, 2021 20:00
gwynne
gwynne requested changes Feb 23, 2021
View reviewed changes
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.

Looking pretty great! Mostly just a bunch of silly little nits in strings, plus a few usage fixes.

@gwynne gwynne added bug Something isn't working enhancement New feature or request semver-minor labels Feb 23, 2021
@fabianfett fabianfett requested a review from gwynne February 23, 2021 12:18
gwynne
gwynne reviewed Feb 23, 2021
View reviewed changes
fabianfett and others added 2 commits February 23, 2021 15:47
@fabianfett @gwynne
Update Sources/PostgresNIO/New/Connection State Machine/Authenticatio…
e7b9f72 
…nStateMachine.swift

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>
@fabianfett
Last code comment
a6fd040
gwynne
gwynne approved these changes Feb 23, 2021
View reviewed changes
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.

Okay, I have no nits left to pick! Amazing work!!

@gwynne gwynne requested a review from 0xTim February 23, 2021 15:23
0xTim
0xTim reviewed Feb 24, 2021
View reviewed changes
Sources/PostgresNIO/Connection/PostgresDatabase+PreparedQuery.swift
return prepared
let request = PrepareQueryRequest(query, as: name)
return self.send(PostgresCommands.prepareQuery(request: request), logger: self.logger).map { _ in
request.prepared!
Copy link
Member

@0xTim 0xTim Feb 24, 2021

Choose a reason for hiding this comment

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

Are there any cases where send() won't set prepared and should we be more defensive here rather than force unwrapping?

Copy link
Collaborator Author

@fabianfett fabianfett Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There are none! :) I should add this as a code comment though. This is a workaround since the send only returns a EventLoopFuture<Void>. And we can't change the send method since it is part of the public PostgresDatabase protocol.

0xTim
0xTim reviewed Feb 24, 2021
View reviewed changes
Sources/PostgresNIO/PostgresRequest.swift

/// Protocol to encapsulate a function call on the Postgres server
///
/// This protocol is deprecated going forward.
Copy link
Member

@0xTim 0xTim Feb 24, 2021

Choose a reason for hiding this comment

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

Should we mark it with @deprecated?

Copy link
Collaborator Author

@fabianfett fabianfett Feb 24, 2021

Choose a reason for hiding this comment

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

We don't want to mark anything deprecated in this pr to change as little as possible in the public API surface. However we should definitely change this with subsequent prs.

0xTim
0xTim approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Overall this is an awesome PR! Couple of minor queries that you can take or leave

@0xTim 0xTim merged commit cdb18d1 into vapor:master Feb 24, 2021
@tanner0101
Copy link
Member

tanner0101 commented Feb 24, 2021

These changes are now available in 1.5.0

1 similar comment
@tanner0101
Copy link
Member

tanner0101 commented Feb 24, 2021

These changes are now available in 1.5.0

@fabianfett fabianfett deleted the ff-state-machine branch February 24, 2021 18:17
This was referenced Feb 25, 2021
Closed
Merged
This was referenced May 1, 2021
Closed
Closed
@fabianfett fabianfett mentioned this pull request Sep 18, 2021
Merged
fabianfett added a commit that referenced this pull request Sep 18, 2021
@fabianfett
Extract PSQLRow (#177)
3c29758 
### Motivation

In #135, I did some naming things just wrong. `PSQLRows` is a stupid name. We should name it `PSQLRowStream`. `PSQLRows.Row` is a stupid name for a single table row. We should name it `PSQLRow`. Transforming an incoming data row packet to a `[PSQLData]` array early is expensive and stupid. Let's not do this anymore.

### Changes

- Extract `PSQLRows.Row` to `PSQLRows` (Got its own file). Stop the early `[PSQLData]` madness.
- Rename `PSQLRows` to `PSQLRowStream`
- Fix naming in integration tests to match `PSQLRowStream`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gwynne gwynne gwynne approved these changes

@0xTim 0xTim 0xTim approved these changes

+1 more reviewer

@weissi weissi weissi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gwynne gwynne

@fabianfett fabianfett

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fabianfett @tanner0101 @weissi @gwynne @0xTim