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))#1618brontolosone merged 10 commits intogetodk:masterfrom
entities.uuid DB column of type uuid (rather than varchar(255))#1618Conversation
0d35408 to
5e69b48
Compare
46169c4 to
20a68fd
Compare
lib/model/migrations/20250904-01-entities-uuid-column-uuid-datatype.js
Outdated
Show resolved
Hide resolved
|
@brontolosone, how's this PR looking in terms of status? I know you've been busy working on other things. I think it'd be nice to include this change in v2025.3, but to do so, we'll need to merge it within the next couple of days (before regression testing begins). From the comments above, it sounds like there are some small code changes to make. |
|
I'd thought it better to leave this one (and #1594) for the next cycle as I think they might be a bit of a distraction in light of the .3 release objectives. Plus it'd be nice to see improved benchmarks, but benchmarking infra is still in the works. |
|
Sounds good, I'll go ahead and remove it from v2025.3. 👍 |
61769e6 to
39a552e
Compare
39a552e to
814ff6a
Compare
814ff6a to
0c0ddff
Compare
ktuite
left a comment
There was a problem hiding this comment.
I'm happy with this!
See two fiddly comments about choice of uuid in tests for test clarity.
test/integration/api/entities.js
Outdated
| await asAlice.post('/v1/projects/1/datasets/people/entities/bulk-delete') | ||
| .send({ | ||
| ids: ['12345678-1234-4123-8234-nonexistent'] | ||
| ids: ['12345678-1234-4123-8234-0123456789ab'] |
There was a problem hiding this comment.
With fresh eyes, I'd like to propose always using 00000000-0000-4000-8000-000000000000 in tests where the entity is not supposed to exist so it's clear and I don't have to remember which uuids the test fixtures are creating.
test/integration/api/entities.js
Outdated
| const asAlice = await service.login('alice'); | ||
|
|
||
| await asAlice.post('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/restore') | ||
| await asAlice.post('/v1/projects/1/datasets/people/entities/uuid:12345678-1234-4123-8234-123456789abc/restore') |
There was a problem hiding this comment.
So there are a couple new places entity uuid validation has been added/strengthened.
-
normalizeUuid(wildtypeUuid)inEntities.getById(here) which covers situations like this, where a request was made with theuuid:prefix. -
Sanitize.queryParamToUuidArrayinGeoExtracts.getEntityFeatureCollectionGeoJson(here) which reminds me of the situation above because it's about sanitizing query params, but isn't related because it actually for arrays of uuids specifically in the geo extraction code.
I didn't necessarily want a test of the restore endpoint to also test the uuid: prefix validation.
What if we remove the uuid: prefixes here in and in the next test, but leave the new test as is -- uuid:not-a-uuid seems fine for that situation.
And what if we add a more basic test like this near the top, say around line 265:
it('should strip uuid: prefix from query param in entity requests', testEntities(async (service) => {
const asAlice = await service.login('alice');
await asAlice.get('/v1/projects/1/datasets/people/entities/uuid:12345678-1234-4123-8234-123456789abc')
.expect(200)
.then(({ body }) => body.uuid.should.equal('12345678-1234-4123-8234-123456789abc'));
}));
Following up on this Slack discussion.
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?
N/A
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.
Not really.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes