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

adding SerializeParametersPlugin. #138

Closed
wants to merge 14 commits into from

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Aug 12, 2022

Currently consumers are required to stringify their parameters almost everytime they use JSON data types. This can become rather tedious, results in less clean code (e.g. having to map arrays of insert objects before insertInto) and is error prone (e.g. this pg issue. mysql2 also doesn't play nice). It's also brought up quite often as of late, in discord and issues.

When consumers stringify on their own, they have to use ColumnType<MyType, string, string> when defining their database interface, which leaves inserts and updates not as type-safe as they could be.

A plugin could come in handy here, it's not a breaking change and consumers can opt-out anytime they like (...maybe in a future where database clients finally handle this).

This plugin allows custom serialization when a serializer function is passed to its constructor's options. E.g. consumer's classes, native dates to datetime conversion, memoization, etc.

This plugin allows casting of serialized parameters, e.g. some consumers got postgres errors while stringifying without casting to jsonb.

(Side note, I first tackled this with query compiler changes, it worked but I've decided to stash 'em as it felt risky, too opinionated and not open for customization from consumers)


Basic usage:

interface Person {
   firstName: string
   lastName: string
   tags: string[] // json or jsonb data type in database
   metadata: Record<string, any> // json or jsonb data type in database
}

interface Database {
    person: Person
}

const db = new Kysely<Database>({
    dialect: new PostgresDialect({
        database: 'kysel_test',
        host: 'localhost',
    }),
    plugins: [
        new SerializeParametersPlugin(),
    ],
})

await db.insertInto('person')
    .values([{
        firstName: 'Jennifer',
        lastName: 'Aniston',
        tags: ['celebrity', 'actress'], // no need to stringify.. we got you!
        metadata: { hatesTheMost: 'Brad' }, // no need to stringify, we got you!
    }])
    .execute() // no errors, hooray!

closes #137

Custom serialization:

const db = new Kysely<Database>({
    dialect: new PostgresDialect({
        database: 'kysel_test',
        host: 'localhost',
    }),
    plugins: [
        new SerializeParametersPlugin({
            serializer: (value) => {
                if (value instanceof Date) {
                    return formatDatetime(value)
                }

                if (value !== null && typeof value === 'object') {
                    return JSON.stringify(value)
                }
  
                return value
            }
        }),
    ],
})

Casting:

const db = new Kysely<Database>({
    dialect: new PostgresDialect({
        database: 'kysel_test',
        host: 'localhost',
    }),
    plugins: [
        new SerializeParametersPlugin({
            caster: (serializedValue, value) => sql`${serializedValue}::jsonb`
        }),
    ],
})

await db.insertInto('person')
    .values([{
        firstName: 'Jennifer',
        lastName: 'Aniston',
        tags: ['celebrity', 'actress'],
    }])
    .execute()

Compiled sql query (Postgres):

insert into "person" ("firstName", "lastName", "tags") values ($1, $2, $3::jsonb)

@igalklebanov igalklebanov marked this pull request as ready for review August 12, 2022 22:04
@igalklebanov
Copy link
Member Author

Will add ::jsonb casting support later today, already implemented and tests are passing.

@koskimas
Copy link
Member

Thanks again for your great work, but I think this is too opinionated to be added to kysely core. This would only work in a subset of cases and fail if you have a mixture of arrays and jsonb columns, which is a thing. At least I (and my team) only use JSONB when it's not possible to use a typed array or other strongly typed column.

This would make an excellent third-party plugin though! I think a lot of people that only use jsonb could find it useful. In the core it would cause too many questions and feature requests and would divert focus from the other features.

Eventually the only way to implement this reliably is figuring out the data type of each value and converting based on that. This has been discussed elsewhere. It would be possible to some extent, but would still have failing corner cases.

In feel bad for being such a dictator here and not merging your work, but I have a very strong idea of what Kysely should be. I'm trying to correct the things I did wrong with objection, one of which was feature creep that basically crippled development eventually.

In the future, could you open an issue first where we could discuss a possible new feature before you implement it?

@koskimas koskimas closed this Aug 21, 2022
@koskimas
Copy link
Member

koskimas commented Aug 21, 2022

I've been thinking about adding the generic converter plugin though. It could be used to solve this case too. It would be a cool problem to solve and would really benefit from the AST-ish data representation and plugin system in Kysely, but it has the same problem: it would have corner cases and I'm afraid people would start using it for everything and it would take over the issues.

I don't know if you're familiar with objection, but I added the upsertGraph method which took over the issues and maintenance resources. It was a good feature and an awesome problem to solve, but when people have a hammer, everything looks like a nail. Eventually most issues were about the nasty corner case bugs it had and all other features got less attention.

Anyway, my idea for the general converter plugin is something like this:

let kysely = new Kysely<DB>(config)

const tableMetadata = await kysely.introspection.getTables()
const converter = new ValueConverterPlugin({
  tableMetadata,
  
  onInput(value: unknown, meta: ColumnMetadata): unknown {
    if (meta.dataType === 'jsonb') {
      return JSON.stringify(value)
    }
    
    return value
  },
  
  onOutput(value: unknown, meta: ColumnMetadata): unknown {
    if (meta.dataType === 'jsonb') {
      return JSON.parse(value)
    }
    
    return value
  }
})

kysely = kysely.withPlugin(converter)

as you can imagine, figuring out the data type in a nested subquery's joined subquery's with statement's subquery's where statement would be kinda difficult. Not to mention views etc.

It's possible to deduct the origin table of any ValueNode using the node tree and table/view metadata unless the user has raw SQL somewhere. As soon as there's raw SQL anywhere in the query, the column metadata may be wrong or not found at all.

Again this would probably be better released as a separate 3rd party plugin.

@igalklebanov igalklebanov deleted the pg-jsonb-array branch September 14, 2022 18:01
@igalklebanov igalklebanov added enhancement New feature or request built-in plugin Related to a built-in plugin wontfix This will not be worked on labels Dec 27, 2022
@subframe7536
Copy link

I wrote a plugin. https://github.com/subframe7536/kysely-sqlite-tools/tree/master/packages/plugin-serialize

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
built-in plugin Related to a built-in plugin enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres jsonb array objects support
3 participants