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

Explicit jsonb support for custom pg clients #5201

Conversation

viqueen
Copy link
Contributor

@viqueen viqueen commented May 30, 2022

I ran into an issue where my jsonb columns are created as text when using pg-mem in tests and aurora friendly clients like knex-data-api-client

It resulted in a lot of cast blah as jsonb workarounds, but really, the issue is that under the hood, knex just falls back to text data type when it cannot resolve a valid pg version.

This change lets custom clients like knex-data-api explicitly state they support jsonb instead of passing in a potentially wrong postgres version (think serverless setup)

@viqueen viqueen marked this pull request as draft May 30, 2022 12:07
@coveralls
Copy link

coveralls commented May 30, 2022

Coverage Status

Coverage increased (+0.06%) to 92.284% when pulling 7ca0f2f on viqueen:issue/PYN-1078-explicit-jsonb-support-for-custom-pg-clients into 8a50dd7 on knex:master.

@OlivierCavadenti
Copy link
Collaborator

Ready for review ?

@viqueen viqueen marked this pull request as ready for review June 7, 2022 08:39
@viqueen
Copy link
Contributor Author

viqueen commented Jun 7, 2022

@OlivierCavadenti it's ready , though I think I should leave the check on cockroachdb client in place to avoid breaking somebody's world

@@ -138,8 +138,8 @@ ColumnCompiler_PG.prototype.uuid = 'uuid';
function jsonColumn(client, jsonb) {
if (
!client.version ||
client.config.client === 'cockroachdb' ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was cockroachdb removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brought it back , I removed it initially just to get a sense of the tests in order to eventually replicate them for pg-mem

@kibertoad
Copy link
Collaborator

@viqueen Can you also add integration test for this?

@kibertoad
Copy link
Collaborator

Documentation needs to be added here: https://github.com/knex/documentation/pulls

@viqueen
Copy link
Contributor Author

viqueen commented Jun 14, 2022

@kibertoad documentation added over here knex/documentation#423
I added a unit test only.
a meaningful integration test would be with a serverless db like aurora postgres, I'm thinking to wire it up with localstack, but can't sink more time this week.
unit test sounds like a good middle ground for now , what do you think ?

@viqueen viqueen requested a review from kibertoad June 16, 2022 23:14
@viqueen
Copy link
Contributor Author

viqueen commented Aug 26, 2022

and another ping @kibertoad , is there more to do documentation wise ?
I've already opened this knex/documentation#423 , but it's also not reviewed yet

@kibertoad
Copy link
Collaborator

sorry for the delay, I was swamped a bit
will take a look over the weekend

@kibertoad
Copy link
Collaborator

kibertoad commented Aug 26, 2022

unit test sounds like a good middle ground for now , what do you think ?

sure, let's go with a unit test

@kibertoad kibertoad merged commit 97fccdf into knex:master Aug 31, 2022
@kibertoad
Copy link
Collaborator

Released in 2.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants