Skip to content

Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable#365

Merged
fabianfett merged 21 commits intomainfrom
feature/dynamic_types
Aug 27, 2023
Merged

Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable#365
fabianfett merged 21 commits intomainfrom
feature/dynamic_types

Conversation

@marius-se
Copy link
Copy Markdown
Contributor

@marius-se marius-se commented May 9, 2023

Closes #333

Example:

// Create type
struct Vector: PostgresDynamicTypeNonThrowingEncodable {
    let value: [Float]

    var psqlType: PostgresDataType
    var psqlFormat: PostgresFormat

    func encode<JSONEncoder>(
        into byteBuffer: inout ByteBuffer,
        context: PostgresNIO.PostgresEncodingContext<JSONEncoder>
    ) where JSONEncoder: PostgresJSONEncoder {
        // ...
    }
}

// Create instance with psqlType and psqlFormat
let vector = Vector(value: [1, 2, 3], psqlType: 16345, psqlFormat: .binary)

// Use in query
let query: PostgresQuery = """
INSERT INTO foo (vector) SET (\(vector));
"""

@marius-se marius-se requested a review from fabianfett May 9, 2023 06:59
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 9, 2023 •
edited
Loading

Codecov Report

Merging #365 (c651b1a) into main (689e4aa) will decrease coverage by 0.12%.
The diff coverage is 66.66%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #365      +/-   ##
==========================================
- Coverage   49.12%   49.01%   -0.12%     
==========================================
  Files         108      108              
  Lines        8839     8841       +2     
==========================================
- Hits         4342     4333       -9     
- Misses       4497     4508      +11
Files Changed Coverage Δ
...s/PostgresNIO/New/Data/Array+PostgresCodable.swift 71.71% <ø> (ø)
Sources/PostgresNIO/New/PostgresQuery.swift 81.29% <50.00%> (ø)
...s/PostgresNIO/New/Data/Range+PostgresCodable.swift 83.82% <100.00%> (ø)
Sources/PostgresNIO/New/PostgresCodable.swift 95.23% <100.00%> (+0.23%) ⬆️

... and 2 files with indirect coverage changes

fabianfett
fabianfett reviewed May 9, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Let's polish this up a tiny bit more :)

@marius-se marius-se requested a review from fabianfett May 9, 2023 08:46
fabianfett
fabianfett requested changes May 9, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Naming is a bit off right now.

fabianfett
fabianfett reviewed May 9, 2023
View reviewed changes
fabianfett
fabianfett reviewed May 9, 2023
View reviewed changes
@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented May 9, 2023

CI complains about breaking api changes, which is a bit worrying. However we have default implementations for the new non-static psqlType and psqlFormat properties, and the rest is just inheritance bubbling up from the two new protocols... sooo that shouldn't cause any problems - or does it?

@marius-se marius-se requested a review from fabianfett May 9, 2023 22:52
fabianfett
fabianfett reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few more comments. The API breakage checker is false positives.

fabianfett
fabianfett reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

two small comments

@marius-se marius-se changed the title Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeNonThrowingEncodable Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable May 10, 2023
fabianfett
fabianfett reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

I think now is the correct time to get a review from @gwynne.

@fabianfett fabianfett requested a review from gwynne May 10, 2023 14:20
gwynne
gwynne requested changes May 10, 2023
View reviewed changes
Copy link
Copy Markdown
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.

A pile of nits, several about wording of comments, a couple about naming.

An additional thought: Would it make sense to also provide utility methods for looking up type OIDs by name (e.g. wrapping SELECT 'foo'::"regtype"::"integer" as "oid"), or do we want to leave that to the purview of higher-level packages?

@marius-se @gwynne
Update Sources/PostgresNIO/New/PostgresCodable.swift
67c1a6b 
Co-authored-by: Gwynne Raskind <gwynne@vapor.codes>
@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented May 12, 2023 •
edited
Loading

An additional thought: Would it make sense to also provide utility methods for looking up type OIDs by name (e.g. wrapping SELECT 'foo'::"regtype"::"integer" as "oid"), or do we want to leave that to the purview of higher-level packages?

Hmmm personally I feel like this belongs into a higher-level package. As far as I understand PostgresNIO is supposed to provide an interface to build and execute queries, not offer a toolbox of ready-to-use queries. What do you think?

@gwynne
Copy link
Copy Markdown
Member

gwynne commented May 13, 2023

Hmmm personally I feel like this belongs into a higher-level package. As far as I understand PostgresNIO is supposed to provide an interface to build and execute queries, not offer a toolbox of ready-to-use queries. What do you think?

I tend to agree; I pretty much only brought it up to make sure we're all on the same page 🙂.

gwynne
gwynne approved these changes May 13, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Ship it! 😂

This Is wonderful work! ❤️

@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented May 13, 2023 •
edited
Loading

@gwynne What about this comment?

...providing func encode<S>(into byteBuffer: inout ByteBuffer, context: PostgresEncodingContext<PostgresJSONEncoder>) where S: Sequence, S.Element: PostgresArrayEncodable { ... } instead of trying to add protocol conformance.

We would still need to add protocol conformance to the different sequence types, wouldn't we?

@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented May 13, 2023

Good to hear that you like it! 😁

@gwynne
Copy link
Copy Markdown
Member

gwynne commented May 13, 2023 •
edited
Loading

We would still need to add protocol conformance to the different sequence types, wouldn't we?

Exactly the opposite - withholding the conformance from any sequence types (including Array) would remove any ambiguity about which version of encode() to call, although my original comment should have specified that the additional method(s) would have to be available on PostgresBindings, not directly on the various encodable protocols. (That's actually what makes it a no more ideal solution than the suggestion of conforming the sequence types one by one - the ability to invoke the encoding logic independently of any supporting types is lost, just as it was when working with any Encodable pre-Swift 5.7.)

All this being said, that comment was never intended to block this PR; I'd go for trying to address it in a different PR (but before this one gets included in a tagged release, otherwise it'd involve revising public API and it'd be a basically permanent mess).

@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented May 13, 2023

Ahhh got it!

@gwynne
Copy link
Copy Markdown
Member

gwynne commented Jul 7, 2023

@fabianfett Ping on this - do you still have any blocking concerns?

@marius-se Ping as well - can you resolve the merge conflicts and bring this up to date with main?

@fabianfett
Copy link
Copy Markdown
Collaborator

fabianfett commented Jul 7, 2023

IIRC all good from my side. @marius-se can you bring this in line with main? Once done I'll do a final review. Thanks for your work up until here.

@fabianfett fabianfett added the semver-minor Adds new public API. label Jul 21, 2023
@fabianfett
Copy link
Copy Markdown
Collaborator

fabianfett commented Aug 5, 2023

@marius-se small ping... shall I move this forward?

@marius-se
Copy link
Copy Markdown
Contributor Author

marius-se commented Aug 5, 2023

Oh sorry @fabianfett! I'm currently traveling through New Zealand (without electricity 😅). If you need the changes on main now feel free to take this over! Otherwise I'll be back in 3-4 weeks :)

@fabianfett fabianfett added this to the 2.0.0 milestone Aug 24, 2023
@fabianfett
Copy link
Copy Markdown
Collaborator

fabianfett commented Aug 27, 2023

API breakage checker complain. Will need to research this one:

21 breaking changes detected in PostgresNIO:
  💔 API breakage: protocol PostgresArrayEncodable has added inherited protocol PostgresThrowingDynamicTypeEncodable
  💔 API breakage: protocol PostgresRangeEncodable has added inherited protocol PostgresDynamicTypeEncodable
  💔 API breakage: protocol PostgresRangeEncodable has added inherited protocol PostgresThrowingDynamicTypeEncodable
  💔 API breakage: protocol PostgresRangeArrayEncodable has added inherited protocol PostgresDynamicTypeEncodable
  💔 API breakage: protocol PostgresRangeArrayEncodable has added inherited protocol PostgresThrowingDynamicTypeEncodable
  💔 API breakage: protocol PostgresEncodable has generic signature change from  to <Self : PostgresNIO.PostgresThrowingDynamicTypeEncodable>
  💔 API breakage: protocol PostgresEncodable has added inherited protocol PostgresThrowingDynamicTypeEncodable
  💔 API breakage: protocol PostgresNonThrowingEncodable has generic signature change from <Self : PostgresNIO.PostgresEncodable> to <Self : PostgresNIO.PostgresDynamicTypeEncodable, Self : PostgresNIO.PostgresEncodable>
  💔 API breakage: protocol PostgresNonThrowingEncodable has added inherited protocol PostgresDynamicTypeEncodable
  💔 API breakage: protocol PostgresNonThrowingEncodable has added inherited protocol PostgresThrowingDynamicTypeEncodable
  💔 API breakage: func PostgresQuery.StringInterpolation.appendInterpolation(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresEncodable> to <Value where Value : PostgresNIO.PostgresThrowingDynamicTypeEncodable>
  💔 API breakage: func PostgresQuery.StringInterpolation.appendInterpolation(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresEncodable> to <Value where Value : PostgresNIO.PostgresThrowingDynamicTypeEncodable>
  💔 API breakage: func PostgresQuery.StringInterpolation.appendInterpolation(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresNonThrowingEncodable> to <Value where Value : PostgresNIO.PostgresDynamicTypeEncodable>
  💔 API breakage: func PostgresQuery.StringInterpolation.appendInterpolation(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresNonThrowingEncodable> to <Value where Value : PostgresNIO.PostgresDynamicTypeEncodable>
  💔 API breakage: func PostgresQuery.StringInterpolation.appendInterpolation(_:context:) has generic signature change from <Value, JSONEncoder where Value : PostgresNIO.PostgresEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder> to <Value, JSONEncoder where Value : PostgresNIO.PostgresThrowingDynamicTypeEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder>
  💔 API breakage: func PostgresBindings.append(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresEncodable> to <Value where Value : PostgresNIO.PostgresThrowingDynamicTypeEncodable>
  💔 API breakage: func PostgresBindings.append(_:context:) has generic signature change from <Value, JSONEncoder where Value : PostgresNIO.PostgresEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder> to <Value, JSONEncoder where Value : PostgresNIO.PostgresThrowingDynamicTypeEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder>
  💔 API breakage: func PostgresBindings.append(_:) has generic signature change from <Value where Value : PostgresNIO.PostgresNonThrowingEncodable> to <Value where Value : PostgresNIO.PostgresDynamicTypeEncodable>
  💔 API breakage: func PostgresBindings.append(_:context:) has generic signature change from <Value, JSONEncoder where Value : PostgresNIO.PostgresNonThrowingEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder> to <Value, JSONEncoder where Value : PostgresNIO.PostgresDynamicTypeEncodable, JSONEncoder : PostgresNIO.PostgresJSONEncoder>
  💔 API breakage: func PostgresEncodable.encode(into:context:) has been removed
  💔 API breakage: func PostgresNonThrowingEncodable.encode(into:context:) has been removed

fabianfett
fabianfett approved these changes Aug 27, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

LGTM. The API breakage checker complains are false positives.

@fabianfett fabianfett merged commit 0d9f13b into main Aug 27, 2023
@fabianfett fabianfett deleted the feature/dynamic_types branch August 27, 2023 11:42
@fabianfett
Copy link
Copy Markdown
Collaborator

fabianfett commented Aug 27, 2023

@marius-se Thanks so much for pushing this through!

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

@fabianfett fabianfett fabianfett approved these changes

Assignees

No one assigned

Labels

semver-minor Adds new public API.

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

Support Types that have a dynamic Postgres type

4 participants

@marius-se @codecov-commenter @gwynne @fabianfett