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

The generated type Json = ColumnType<JsonValue, string, string> should just be JsonValue for Postgres. #124

Closed
afg419 opened this issue Dec 13, 2023 · 3 comments · Fixed by #126

Comments

@afg419
Copy link

afg419 commented Dec 13, 2023

If I'm not mistaken, the generated json type type Json = ColumnType<JsonValue, string, string> seems to be overly restrictive for Postgres, as I can use kysely to insert and update JsonValue values, in addition to strings. Since JsonValue | string is just JsonValue, the whole thing can be collapsed type Json = JsonValue.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@afg419
Copy link
Author

afg419 commented Dec 13, 2023

That is, both of these work:

await kysely.insertInto('myTable').values({ address: { street: '111 First Ave', zip: '00000', state: 'MA' }}).executeTakeFirst()

await kysely.insertInto('myTable').values({ address: JSON.stringify({ street: '111 First Ave', zip: '00000', state: 'MA' })}).executeTakeFirst()

@BoscoDomingo
Copy link
Contributor

I came here to post this exact issue. I changed the generated types and it works flawlessly (at least for insertion).

In fact, it works better than inserting a string because (at least in JS/TS) stringifying an object escapes the double quotes, therefore taking up significantly more space than needed. I wouldn't be surprised if that also causes us to lose the SQL optimizations and indexing abilities.

In essence, I echo the OP

@BoscoDomingo
Copy link
Contributor

Pushed a PR, I'm expecting that to work. @RobinBlomberg would love a 👍🏼!

jordanh added a commit to ParabolInc/parabol that referenced this issue Mar 4, 2024
   - also:
      - bumps kysely-codegen for proper JSON typing
        (see: RobinBlomberg/kysely-codegen#124)
      - fixes off-by-one error in parabol/packages/server/postgres/migrations/1677272969994_meetingTemplatesMove.ts
        to avoid copying a bad pattern – I wonder if this should become
	a helper function instead?
jordanh added a commit to ParabolInc/parabol that referenced this issue Mar 8, 2024
   - also:
      - bumps kysely-codegen for proper JSON typing
        (see: RobinBlomberg/kysely-codegen#124)
      - fixes off-by-one error in parabol/packages/server/postgres/migrations/1677272969994_meetingTemplatesMove.ts
        to avoid copying a bad pattern – I wonder if this should become
	a helper function instead?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants