CockroachDB - Creating UUID primary key adds additional rowid

[hairmare]

Describe the Bug

When using CockroachDB with a table that has a uuid field as primary key, the database adds an additional rowid key because it doesn't recognize the UUID based primary key.

To Reproduce

Navigate to /admin/settings/data-model/directus_collections on a fresh installation configured to use CockroachDB.

This can also be reproduced by creating a new table in a knex migration using the following code for the primary key:

table.uuid("id").notNullable().primary();

Errors Shown

All tables that have a UUID primary key will have been created with an additional rowid field:

What version of Directus are you using?

9.12.1

What version of Node.js are you using?

16.14.0

What database are you using?

CockroachDB v22.1.0

What browser are you using?

Firefox

How are you deploying Directus?

Kubernetes

[hairmare]

A workaround to this is creating the primary key using specificType() as follows:

table.specificType("id", "UUID PRIMARY KEY");

This workaround works for both CockroachDB and PostgreSQL but it might break other supported databases given that the UUID PRIMARY KEY type might not be supported across the board.

[azrikahar]

Additional note – This can also be observed in the short clip here at 0:11: #11760 (comment)

[rijkvanzanten]

I believe this is an issue in how knex constructs the sql for creating the primary key, not necessarily a problem in how we're using knex 🤔

I've opened a new issue there which we can track 👍🏻

Ref knex/knex#5211

[rijkvanzanten]

my current understanding is that my change here in knex will enable finding a non-bc-breaking solution for #13749 that should be acceptable whilst not constraining us wrt to a future fix that supports just using primary() in the most obvious fashion.

I don't fully agree @hairmare.. While the current patch allows us to add a dedicated branching path for the primary key + uuid combination, it doesn't really solve the original problem, as the it still exists for any other type (seeing UUID is now the only type that accepts that additional flag). Even though they're less common, we do already expose (and see a lot of use) of the "manual string primary key" type, which would have the same problem with this fix applied 🤔 The real solution here would be to have the other dialects (not just PG either) work consistently with how the MySQL knex dialect is now handling the create statement when it comes to primary keys.

[hairmare]

yeah, my assumption was based on something like the following diff as the main change. I do see why that might not be acceptable tho.

diff --git a/api/src/services/fields.ts b/api/src/services/fields.ts
index e5c8ff0..9db35f9 100644
--- a/api/src/services/fields.ts
+++ b/api/src/services/fields.ts
@@ -551,6 +551,8 @@ export class FieldsService {
                        }
                } else if (field.type === 'string') {
                        column = table.string(field.field, field.schema?.max_length ?? undefined);
+               } else if (field.type === 'uuid') {
+                       column = table.uuid(field.field, { primaryKey: field.schema?.is_primary_key });
                } else if (['float', 'decimal'].includes(field.type)) {
                        const type = field.type as 'float' | 'decimal';
                        column = table[type](field.field, field.schema?.numeric_precision || 10, field.schema?.numeric_scale || 5);

one argument for adding branching code would be that uuid is a special type given that it's only natively supported by postgresql dialects. it would also open up avenues for supporting binary uuids in the future (afaict binary uuids would probably be something that you'd come across with mssql).

i did some digging wrt to the implementation of the mysql dialect and it seems like the logic there would benefit from getting refactored into a more prominent position in the knex code base to make implementing this in a way that keeps the api consistent possible. knex does support a bunch of dialects that i have never used so it's hard to tell how generic this can be made.

I'm going to dig some deeper and see if i can figure out a clean way to get real primary() support implemented in knex. For now knex-next will at least help me make my own migrations a bit cleaner once it's tagged.

[hairmare]

i prepared some tests: hairmare/knex@970832f

implementing this should be possible but will take a while because it needs to touch a lot of knex to work

[hairmare]

progress is here:

• feat: inline primary key creation for postgres flavours knex/knex#5233

[christopher-kapic]

@hairmare's PR was just merged for knex (so this should be fixable once knex has a new release).

[christopher-kapic]

According to this comment Knex should have a new release this week.

[christopher-kapic]

Knex has a new release!