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 PostgresQuery #223

Merged
merged 3 commits into from
Feb 21, 2022
Merged

Add PostgresQuery #223

merged 3 commits into from
Feb 21, 2022

Conversation

fabianfett
Copy link
Collaborator

Motivation

Right now we encode bindings right before we send them out, in PSQLFrontendMessageEncoder. This means that a JSONEncoder must be set per connection. This is not an ideal design. Further we need to wrap the values that we want to bind in an existential [PSQLEncodable].

I propose changing this API and introducing an explicit PostgresQuery type, that is ExpressibleByStringInterpolation.

Changes

  • Add PostgresQuery
  • Add PostgresBindings
  • Remove PostgresJSONEncoder from PSQLFrontendMessageEncoder

Result

  • No more existentials in the encoding path.
  • Cleaner, less cluttered code.

@fabianfett fabianfett requested a review from gwynne February 19, 2022 10:50
@@ -26,7 +26,7 @@ public typealias PostgresFormatCode = PostgresFormat

/// The data type's raw object ID.
/// Use `select * from pg_type where oid = <idhere>;` to lookup more information.
public struct PostgresDataType: RawRepresentable, Equatable, CustomStringConvertible {
public struct PostgresDataType: RawRepresentable, Hashable, CustomStringConvertible {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gwynne We extend public API here!

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me 🙂

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2022

Codecov Report

Merging #223 (426f6a3) into main (f588870) will increase coverage by 4.57%.
The diff coverage is 57.44%.

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
+ Coverage   38.62%   43.19%   +4.57%     
==========================================
  Files         120      121       +1     
  Lines        9271     9322      +51     
==========================================
+ Hits         3581     4027     +446     
+ Misses       5690     5295     -395     
Flag Coverage Δ
unittests 43.19% <57.44%> (+4.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...resNIO/Connection/PostgresConnection+Connect.swift 0.00% <0.00%> (ø)
...esNIO/Connection/PostgresConnection+Database.swift 0.00% <0.00%> (ø)
Sources/PostgresNIO/Data/PostgresDataType.swift 58.49% <ø> (+41.50%) ⬆️
Sources/PostgresNIO/New/PSQLFrontendMessage.swift 85.05% <ø> (+14.94%) ⬆️
Sources/PostgresNIO/New/PSQLTask.swift 35.48% <0.00%> (+1.19%) ⬆️
Sources/PostgresNIO/Postgres+PSQLCompat.swift 0.00% <0.00%> (ø)
Sources/PostgresNIO/New/PSQLConnection.swift 30.00% <12.50%> (+0.29%) ⬆️
Sources/PostgresNIO/New/PSQLChannelHandler.swift 60.53% <20.51%> (+12.56%) ⬆️
...nection State Machine/ConnectionStateMachine.swift 54.82% <42.85%> (+8.88%) ⬆️
...tion State Machine/ExtendedQueryStateMachine.swift 65.26% <50.00%> (+7.78%) ⬆️
... and 49 more

Postgres compiles

Fix tests
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 have no problems with this as it stands on its own, though it may need some tweaking to be ideal for SQLKit/FluentKit interop, mostly in terms of the serialization of positional parameters. That can be addressed later, though.

@@ -26,7 +26,7 @@ public typealias PostgresFormatCode = PostgresFormat

/// The data type's raw object ID.
/// Use `select * from pg_type where oid = <idhere>;` to lookup more information.
public struct PostgresDataType: RawRepresentable, Equatable, CustomStringConvertible {
public struct PostgresDataType: RawRepresentable, Hashable, CustomStringConvertible {
Copy link
Member

Choose a reason for hiding this comment

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

Fine by me 🙂

@fabianfett fabianfett merged commit dd5b17c into vapor:main Feb 21, 2022
@fabianfett fabianfett deleted the ff-add-PostgresQuery branch February 21, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants