Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable #365

marius-se · 2023-05-09T06:59:45Z

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));
"""

codecov-commenter · 2023-05-09T07:05:30Z

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

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

Sources/PostgresNIO/New/PostgresCodable.swift

Sources/PostgresNIO/New/PostgresQuery.swift

fabianfett

Naming is a bit off right now.

Sources/PostgresNIO/New/PostgresCodable.swift

docker-compose.yml

marius-se · 2023-05-09T22:52:31Z

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?

fabianfett

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

Sources/PostgresNIO/New/Data/Array+PostgresCodable.swift

Sources/PostgresNIO/New/Data/Range+PostgresCodable.swift

Sources/PostgresNIO/New/PostgresCodable.swift

fabianfett

two small comments

Sources/PostgresNIO/New/PostgresQuery.swift

Tests/PostgresNIOTests/New/PostgresQueryTests.swift

fabianfett

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

gwynne

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?

Sources/PostgresNIO/New/Data/Array+PostgresCodable.swift

Sources/PostgresNIO/New/Data/Range+PostgresCodable.swift

Sources/PostgresNIO/New/PostgresCodable.swift

Co-authored-by: Gwynne Raskind <[email protected]>

marius-se · 2023-05-12T14:15:43Z

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 · 2023-05-13T11:20:08Z

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

Ship it! 😂

This Is wonderful work! ❤️

marius-se · 2023-05-13T18:19:49Z

@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 · 2023-05-13T18:20:43Z

Good to hear that you like it! 😁

gwynne · 2023-05-13T18:41:01Z

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 · 2023-05-13T19:53:09Z

Ahhh got it!

gwynne · 2023-07-07T01:44:37Z

@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 · 2023-07-07T07:45:42Z

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 · 2023-08-05T13:58:21Z

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

marius-se · 2023-08-05T21:01:24Z

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 · 2023-08-27T11:37:58Z

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

LGTM. The API breakage checker complains are false positives.

fabianfett · 2023-08-27T11:42:38Z

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

add PostgresDynamicTypeThrowingEncodable

e369361

marius-se requested a review from fabianfett May 9, 2023 06:59

fabianfett reviewed May 9, 2023

View reviewed changes

Sources/PostgresNIO/New/PostgresCodable.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/New/PostgresQuery.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/New/PostgresQuery.swift Outdated Show resolved Hide resolved

fix change requests

65bf8a7

marius-se requested a review from fabianfett May 9, 2023 08:46

fabianfett requested changes May 9, 2023

View reviewed changes

Sources/PostgresNIO/New/PostgresCodable.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/New/PostgresCodable.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/New/PostgresCodable.swift Outdated Show resolved Hide resolved

fabianfett reviewed May 9, 2023

View reviewed changes

Sources/PostgresNIO/New/PostgresCodable.swift Outdated Show resolved Hide resolved

fabianfett reviewed May 9, 2023

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

marius-se added 6 commits May 9, 2023 23:43

remove psql-15

81ea8f6

update dynamic type protocol names

9c88c5c

remove inherited encode method from PostgresEncodable protocol

a13df9d

remove inherited encode method from PostgresNonThrowingEncodable

1801353

Merge branch 'main' into feature/dynamic_types

a0ff95d

conform Range types to dynamic type protocols

8945eac

marius-se requested a review from fabianfett May 9, 2023 22:52

fabianfett reviewed May 10, 2023

View reviewed changes

marius-se added 2 commits May 10, 2023 14:27

fix change requests

0938343

update Postgres dynamic type conformances for arrays and ranges

c54ddd6

fabianfett reviewed May 10, 2023

View reviewed changes

Sources/PostgresNIO/New/PostgresQuery.swift Outdated Show resolved Hide resolved

Tests/PostgresNIOTests/New/PostgresQueryTests.swift Outdated Show resolved Hide resolved

marius-se added 4 commits May 10, 2023 16:05

fix change requests

e00f5fc

fix linebreak

5533330

add metadata check in dynamic type test

63c95f2

Merge branch 'main' into feature/dynamic_types

6e33c4a

marius-se changed the title ~~Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeNonThrowingEncodable~~ Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable May 10, 2023

fabianfett reviewed May 10, 2023

View reviewed changes

fabianfett requested a review from gwynne May 10, 2023 14:20

gwynne requested changes May 10, 2023

View reviewed changes

Update Sources/PostgresNIO/New/PostgresCodable.swift

67c1a6b

Co-authored-by: Gwynne Raskind <[email protected]>

marius-se and others added 4 commits May 12, 2023 15:37

Update Sources/PostgresNIO/New/PostgresCodable.swift

e7bd249

Co-authored-by: Gwynne Raskind <[email protected]>

fix comment change requests

00810c5

Merge branch 'main' into feature/dynamic_types

cbd49f6

rename to PostgresThrowingDynamicTypeEncodable

615817e

gwynne approved these changes May 13, 2023

View reviewed changes

fabianfett added the semver-minor Adds new public API. label Jul 21, 2023

fabianfett added this to the 2.0.0 milestone Aug 24, 2023

marius-se added 2 commits August 27, 2023 20:38

Merge branch 'main' into feature/dynamic_types

0ae1316

Merge branch 'main' into feature/dynamic_types

c651b1a

fabianfett approved these changes Aug 27, 2023

View reviewed changes

fabianfett merged commit 0d9f13b into main Aug 27, 2023

fabianfett deleted the feature/dynamic_types branch August 27, 2023 11:42

Uh oh!

Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable #365

Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable #365

Uh oh!

Conversation

marius-se commented May 9, 2023

Uh oh!

codecov-commenter commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marius-se commented May 9, 2023

Uh oh!

fabianfett left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

gwynne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marius-se commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gwynne commented May 13, 2023

Uh oh!

gwynne left a comment

Choose a reason for hiding this comment

Uh oh!

marius-se commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marius-se commented May 13, 2023

Uh oh!

gwynne commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marius-se commented May 13, 2023

Uh oh!

codecov-commenter commented May 9, 2023 •

edited

Loading

fabianfett left a comment •

edited

Loading

marius-se commented May 12, 2023 •

edited

Loading

marius-se commented May 13, 2023 •

edited

Loading

gwynne commented May 13, 2023 •

edited

Loading