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

fix(postgresql): add primaryKey option for uuid #5212

Merged

Conversation

hairmare
Copy link
Contributor

@hairmare hairmare commented Jun 6, 2022

Adds support for creating a uuid field as primary key directly in a create statement using uuid('id', { primaryKey: true }) for PostgreSQL dialects.

For now the option is only implemented for PostgreSQL dialects since it should work for all of them and it's only really an issue for CockroachDB.

As far as i can tell, using uuid('id', { primaryKey: true }).primary() would still work if you need to support other dialects (the primary() would just generate a no-op alter statement).

It's getting late here, so i'll add a docs PR tomorrow. Please let me know if the integration test (or anything else) is too hacky and i'll improve on it.

Fixes #5211

Docs: knex/documentation#419

@intech
Copy link
Contributor

intech commented Jun 6, 2022

The cockroach dialect needs to be able to set the default value to gen_random_uuid(). The full SQL line looks like this:

UUID PRIMARY KEY DEFAULT gen_random_uuid()

'SELECT c.oid FROM pg_catalog.pg_class c WHERE c.relname = ?',
[table_name]
)
.then((res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use instead await notation


return knex
.raw(
`select c.column_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we select and test tolumn type too ?

@coveralls
Copy link

coveralls commented Jun 7, 2022

Coverage Status

Coverage increased (+0.01%) to 92.237% when pulling 46b8a69 on hairmare:fix/postgresql-uuid-primary-in-create into bb07d42 on knex:master.

@hairmare
Copy link
Contributor Author

hairmare commented Jun 7, 2022

I switched to using await and implemented the column_type check in 3cb450b and added default gen_random_uuid() for the CockroachDB dialect in 9010b3f.

The tests worked locally, can i have another workflow approval here to see if they still work on GitHub Actions?

@hairmare
Copy link
Contributor Author

hairmare commented Jun 7, 2022

I also added docs in knex/documentation#419

Copy link
Collaborator

@kibertoad kibertoad left a comment

Choose a reason for hiding this comment

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

TS types also need to be updated

@hairmare
Copy link
Contributor Author

hairmare commented Jun 7, 2022

@kibertoad thanks for the hint, types have been added in 46b8a69

I also realised that i had forgotten to register the new CockroachDB unit tests which has now also been fixed.

@kibertoad kibertoad merged commit 0918bf9 into knex:master Jun 8, 2022
@kibertoad
Copy link
Collaborator

Thank you! I'll publish new version tonight.

@hairmare hairmare deleted the fix/postgresql-uuid-primary-in-create branch June 8, 2022 09:06
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 this pull request may close these issues.

CockroachDB inserting additional rowid column when using .primary()
5 participants