vapor / postgres-nio

🐘 Non-blocking, event-driven Swift client for PostgreSQL.
https://api.vapor.codes/postgresnio/documentation/postgresnio/
MIT License
317 stars 72 forks source link

Add PostgresDynamicTypeThrowingEncodable and PostgresDynamicTypeEncodable #365

Closed marius-se closed 1 year ago

marius-se commented 1 year ago

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 commented 1 year ago

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 ```diff @@ 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](https://app.codecov.io/gh/vapor/postgres-nio/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Ξ” | | |---|---|---| | [...s/PostgresNIO/New/Data/Array+PostgresCodable.swift](https://app.codecov.io/gh/vapor/postgres-nio/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvRGF0YS9BcnJheStQb3N0Z3Jlc0NvZGFibGUuc3dpZnQ=) | `71.71% <ΓΈ> (ΓΈ)` | | | [Sources/PostgresNIO/New/PostgresQuery.swift](https://app.codecov.io/gh/vapor/postgres-nio/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvUG9zdGdyZXNRdWVyeS5zd2lmdA==) | `81.29% <50.00%> (ΓΈ)` | | | [...s/PostgresNIO/New/Data/Range+PostgresCodable.swift](https://app.codecov.io/gh/vapor/postgres-nio/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvRGF0YS9SYW5nZStQb3N0Z3Jlc0NvZGFibGUuc3dpZnQ=) | `83.82% <100.00%> (ΓΈ)` | | | [Sources/PostgresNIO/New/PostgresCodable.swift](https://app.codecov.io/gh/vapor/postgres-nio/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvUG9zdGdyZXNDb2RhYmxlLnN3aWZ0) | `95.23% <100.00%> (+0.23%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/vapor/postgres-nio/pull/365/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor)
marius-se commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 πŸ™‚.

marius-se commented 1 year ago

@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 commented 1 year ago

Good to hear that you like it! 😁

gwynne commented 1 year ago

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 commented 1 year ago

Ahhh got it!

gwynne commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

marius-se commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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