postgres-kit icon indicating copy to clipboard operation
postgres-kit copied to clipboard

Crash in `_KeyedEncoder` when encoding a "SE-0295" `Codable`

Open finestructure opened this issue 4 years ago • 7 comments

Describe the bug

I've defined a Codable enum with associated types with custom encoding to match Swift 5.5's format of Codable synthesis (SE-0295). The idea is that I can start storing data with 5.4 and then remove the custom Codable conformance when 5.5 ships.

When trying to store a value of this type to a Postgres database, the save crashes in _KeyedEncoder, trying to encode the data.

NB: the fact that I'm trying to match SE-0295's format may be a red herring, I've not tried a different format!

To Reproduce

I'm attaching a standalone Swift playground that reproduces the crash and shows that encoding with JSONEncoder works as expected.

Expected behavior

Encoding of a value that's encodable with JSONEncoder should not crash.

Environment

Xcode 12.5

postgres-kit Playground.zip

finestructure avatar May 27 '21 07:05 finestructure

@finestructure Thanks for making this issue so easy to reproduce! It appears that PostgresDataEncoder does not support summoning nested containers (it just fatalErrors). I don't have the full overview of whether it should but I can provide a workaround. By conforming ProductType to PostgresJSONCodable the whole encoding step is short-circuited and you should get the expected result.

siemensikkema avatar May 28 '21 21:05 siemensikkema

Hi @siemensikkema , thanks a lot for taking a look!

There's probably also another way to work around this issue. I haven't tried it in this instance yet but I'm pretty sure we have older encodable enums with associated types where we used

"key": null

to encode presence of keys and keys with values rather than nested containers. The reason I went for the new format is that with SE-0295 in Swift 5.5 I believe the standard auto-generated format is going to be the nested one.

I.e. any user trying to store an enum with associated types using Codable synthesis will probably hit this exception.

I'll try your suggested workaround as that should allow me to retain the format and then I can just drop the PostgresJSONCodable should support be added. I'll report back how it goes!

finestructure avatar May 30 '21 09:05 finestructure

Just to follow up: making ProductType conform to PostgresJSONCodable allows saving but not reading apparently:

/Users/sas/Projects/spi-server/Tests/AppTests/ProductTests.swift:59: error: -[AppTests.ProductTests test_Product_save] : XCTUnwrap failed: threw error "invalid field: type type: ProductType error: typeMismatch(App.ProductType, Swift.DecodingError.Context(codingPath: [], debugDescription: "Could not convert to ProductType: {\"library\": {\"_0\": \"automatic\"}}", underlyingError: nil))"

It saved to the db ok:

CleanShot 2021-05-31 at 11 05 20@2x

This is the test: https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/blob/e99471c81a2973253ed5867ec18a88a4f54da11c/Tests/AppTests/ProductTests.swift#L8

I've also double checked and we have no other enums with associated types marked as Codable, so I'm not yet sure if that work-around would be feasible. (I'd also rather not introduce a custom coding conformance if it can be avoided and made forward compatible since we're talking about persisting results here.)

finestructure avatar May 31 '21 09:05 finestructure

So digging into this a bit further, it the problem was that the PostgresDataType type when reading back is jsonb. Making the conformance PostgresJSONBCodable instead fixes that.

(As an aside, .json in the field definition generates jsonb in the schema. So maybe PostgresJSONCodable should also try guard let value = try? postgresData.jsonb(as: Self.self) else { in addition to just .json(as: Self.self)?)

finestructure avatar May 31 '21 10:05 finestructure

@finestructure ah I missed that you needed JSONB. However, are you sure about it generating jsonb? Looking at PostgresColumnType.swift I see:

    /// textual JSON data
    public static var json: PostgresColumnType {
        return .init(.json)
    }

    /// binary JSON data, decomposed
    public static var jsonb: PostgresColumnType {
        return .init(.jsonb)
    }

siemensikkema avatar May 31 '21 10:05 siemensikkema

Mmm, here's our migration:

struct UpdateProductType: Migration {
    func prepare(on database: Database) -> EventLoopFuture<Void> {
        database.schema("products")
            .deleteField("type")
            .field("type", .json, .required)
            .update()
    }

    func revert(on database: Database) -> EventLoopFuture<Void> {
        database.schema("products")
            .deleteField("type")
            .field("type", .string, .required)
            .update()
    }
}

and here's the schema:

CREATE TABLE products (
    id uuid PRIMARY KEY,
    created_at timestamp with time zone,
    updated_at timestamp with time zone,
    version_id uuid REFERENCES versions(id) ON DELETE CASCADE,
    name text NOT NULL,
    targets text[] DEFAULT '{}'::text[],
    type jsonb NOT NULL
);

This is true for all our .json columns - they are jsonb in the schema.

finestructure avatar May 31 '21 10:05 finestructure

@finestructure Ah yes, this is because in FluentKit .json is an alias for DatabaseSchema.DataType.dictionary(of: DataType?) which PostgresConverterDelegate interprets as SQLRaw("JSONB"). You should be able to do .sql(SQLRaw("JSON")) in your migration to force it to be textual if you wanted to. You probably don't though but it's just a bit confusion the way it's set up.

siemensikkema avatar May 31 '21 11:05 siemensikkema