make `entities.uuid` DB column of type uuid (rather than `varchar(255)`) #1618

brontolosone · 2025-09-09T10:40:50Z

What has been done to verify that this works as intended?

Tests, a few of which needed fixing, as they were supplying non-UUIDs as if they were UUIDs.

Why is this the best possible solution? Were any other approaches considered?

No. Well, it'd be good to do some light validation of inputs before they reach the DB. You'll get a 500 when supplying a non-UUID.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Users who get a sense of accomplishment out of disk space usage may be sad to see their DB disk space usage shrink and overall performance improved.
Users who somehow have succeeded in using non-UUID entity IDs (as the DB used to permit so), will be faced with an unmigratable DB. But on Slack I've been assured that this is not a likely scenario and that something must have gone very wrong to end up in that state.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Not really.

Before submitting this PR, please make sure you have:

run make test and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code from external sources are properly credited in comments or that everything is internally sourced

ktuite · 2025-10-08T16:54:22Z

lib/model/migrations/20250904-01-entities-uuid-column-uuid-datatype.js

These migrations are small enough that I don't think we should use this pattern of the sidecar sql files. Especially because the mechanism for loading the extra files is longer than the migrations themselves. If we want to use this pattern more in the future, maybe we put getSqlFiles in a utility file or something instead of copying it from migration to migration.

ktuite · 2025-10-08T17:17:28Z

test/data/xml.js

      one: `<data xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms" id="simpleEntity" version="1.0">
              <meta>
                <instanceID>one</instanceID>
-                <entities:entity dataset="people" id="uuid:12345678-1234-4123-8234-123456789abc" create="1">


These prefixes should stay in these tests. I can't find where it's mentioned explicitly in the spec but this was an explicit decision to allow these prefixes in submissions and then strip them out before storing them because there's a similar functionality for submission instance IDs.

When test/data/xml.js is reverted, most of the other test files (the ones that remove the uuid) also need to be reverted.

allow these prefixes in submissions and then strip them out before storing them because there's a similar functionality for submission instance IDs.

IIUC submission instance IDs are stored as-is though. They're not mangled (that is, their uuid: (if they have one at all - instance IDs are freeform strings...) prefix is not stripped).

ktuite · 2025-10-08T17:20:08Z

test/integration/task/purge.js


    it('should call entities purge if entities uuid is specified', testTask(() =>
-      purgeTask({ entityUuid: 'abc', projectId: 1, datasetName: 'people' })
+      purgeTask({ entityUuid: '00000000-0000-0000-8000-000000000000', projectId: 1, datasetName: 'people' })


This type of change is reasonable to me.

But if you change abc to a valid uuid structure here, you should probably do that for the other tests in this file, even though many of them never get to the uuid validity checking stage.

ktuite · 2025-10-08T17:21:21Z

test/unit/data/submission.js

              create: '1',
              dataset: 'people',
-              id: 'uuid:12345678-1234-4123-8234-123456789abc'
+              id: '12345678-1234-4123-8234-123456789abc'


Revert this to bring uuid: back here and elsewhere in this file

ktuite · 2025-10-08T17:21:29Z

test/unit/data/entity.js

          .then((result) => {
            should(result.system).eql({
              create: '1',
-              id: 'uuid:12345678-1234-4123-8234-123456789abc',


Revert this to bring uuid: back here and elsewhere in this file

ktuite · 2025-10-08T17:21:51Z

test/integration/api/entities.js

          .send(testData.instances.simpleEntity.one
            .replace('create="1"', 'update="1"')
            .replace('<instanceID>one', '<deprecatedID>one</deprecatedID><instanceID>one2')
-            .replace('id="uuid:12345678-1234-4123-8234-123456789abc"', 'id="uuid:12345678-1234-4123-8234-123456789aaa"'))


Revert this to bring uuid: back here and in similar situations in this file

ktuite · 2025-10-08T17:24:17Z

test/integration/api/entities.js

      await asAlice.post('/v1/projects/1/datasets/people/entities/bulk-delete')
        .send({
-          ids: ['12345678-1234-4123-8234-nonexistent']
+          ids: ['12345678-1234-4123-8234-0123456789ab']


This kind of change to make it into a valid UUID is good.

ktuite · 2025-10-08T17:29:48Z

test/integration/api/entities.js

      const asAlice = await service.login('alice');

-      await asAlice.post('/v1/projects/1/datasets/people/entities/nonexistant/restore')
+      await asAlice.post('/v1/projects/1/datasets/people/entities/00000000-0000-0000-0000-000000000000/restore')


This change is fine but maybe there should be another test of the other way this endpoint can fail

it('should reject if the entity uuid is not valid', testEntities(async (service) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/datasets/people/entities/abc/restore') .expect(400) .then(({ body }) => { body.code.should.equal(400.11); body.message.should.equal('Invalid input data type: expected "abc" to be (type uuid)'); }); }));

I'm not thinking of a test like this for every endpoint using entity UUIDs, but at least one somewhere to capture this new validation.

ktuite · 2025-10-08T17:31:25Z

test/integration/api/entities.js

    it('should return notfound if the dataset does not exist', testEntities(async (service) => {
      const asAlice = await service.login('alice');

-      await asAlice.get('/v1/projects/1/datasets/nonexistent/entities/123')


These changes are fine

ktuite · 2025-10-08T17:31:47Z

test/integration/api/datasets.js

        await asAlice.post('/v1/projects/1/forms/multiPropertyEntity2/submissions')
          .send(testData.instances.multiPropertyEntity.one
            .replace('multiPropertyEntity', 'multiPropertyEntity2')
-            .replace('uuid:12345678-1234-4123-8234-123456789aaa', 'uuid:12345678-1234-4123-8234-123456789ccc')


Revert this to bring uuid: back here and elsewhere in this file

brontolosone requested review from alxndrsn and removed request for alxndrsn September 9, 2025 10:41

brontolosone force-pushed the entities-uuid-be-uuid branch from 0d35408 to 5e69b48 Compare September 10, 2025 11:47

brontolosone requested a review from alxndrsn September 10, 2025 11:54

brontolosone added this to ODK Central Sep 10, 2025

github-project-automation bot moved this to 🕒 backlog in ODK Central Sep 10, 2025

brontolosone moved this from 🕒 backlog to ✏️ in progress in ODK Central Sep 10, 2025

brontolosone marked this pull request as ready for review September 10, 2025 12:18

brontolosone force-pushed the entities-uuid-be-uuid branch from 5e69b48 to d0bd95b Compare September 11, 2025 03:46

use DB uuid type for entity uuids: use valid uuids in tests

4338988

brontolosone force-pushed the entities-uuid-be-uuid branch from d0bd95b to 46169c4 Compare September 11, 2025 06:06

brontolosone added 3 commits September 11, 2025 06:32

use DB uuid type for entity uuids: migration and framefield type

2ece00a

use DB uuid type for entity uuids: straightforward type casts

f7ea9f3

use DB uuid type for entity uuids: less straightforward type wrangling

20a68fd

brontolosone force-pushed the entities-uuid-be-uuid branch from 46169c4 to 20a68fd Compare September 11, 2025 06:34

brontolosone requested review from ktuite and sadiqkhoja and removed request for alxndrsn September 15, 2025 08:37

matthew-white assigned brontolosone Sep 15, 2025

ktuite reviewed Oct 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

make `entities.uuid` DB column of type uuid (rather than `varchar(255)`) #1618

make `entities.uuid` DB column of type uuid (rather than `varchar(255)`) #1618

Uh oh!

brontolosone commented Sep 9, 2025 •

edited

Loading

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

brontolosone Oct 9, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

ktuite Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

make entities.uuid DB column of type uuid (rather than varchar(255)) #1618

Are you sure you want to change the base?

make entities.uuid DB column of type uuid (rather than varchar(255)) #1618

Uh oh!

Conversation

brontolosone commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

make `entities.uuid` DB column of type uuid (rather than `varchar(255)`) #1618

make `entities.uuid` DB column of type uuid (rather than `varchar(255)`) #1618

brontolosone commented Sep 9, 2025 •

edited

Loading