-
Notifications
You must be signed in to change notification settings - Fork 85
Use postgres UUID types for actee ids #1589
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
base: master
Are you sure you want to change the base?
Changes from all commits
6b8cd1a
e3dc1e0
f342e22
fe018df
e994a09
d9aec4f
1ceaee0
88a7679
8ca97af
83ebf49
aba23f6
2c6dcaa
431f7dd
e3655b5
04645f0
7358c81
f9fd902
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // Copyright 2025 ODK Central Developers | ||
| // See the NOTICE file at the top-level directory of this distribution and at | ||
| // https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| // This file is part of ODK Central. It is subject to the license terms in | ||
| // the LICENSE file found in the top-level directory of this distribution and at | ||
| // https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| // including this file, may be copied, modified, propagated, or distributed | ||
| // except according to the terms contained in the LICENSE file. | ||
|
|
||
| const { sql } = require('slonik'); | ||
|
|
||
| // Take care when changing these values - they should probably remain identical to those in the db | ||
| // migration where they were first defined - "actee-id-as-uuid". | ||
| const sqlSpecial = { | ||
| '*': sql`'00000000-0000-8000-8000-000000000001'`, // eslint-disable-line key-spacing | ||
| actor: sql`'00000000-0000-8000-8000-000000000002'`, // eslint-disable-line key-spacing | ||
| group: sql`'00000000-0000-8000-8000-000000000003'`, // eslint-disable-line key-spacing | ||
| user: sql`'00000000-0000-8000-8000-000000000004'`, // eslint-disable-line key-spacing | ||
| form: sql`'00000000-0000-8000-8000-000000000005'`, // eslint-disable-line key-spacing | ||
| submission: sql`'00000000-0000-8000-8000-000000000006'`, // eslint-disable-line key-spacing | ||
| field_key: sql`'00000000-0000-8000-8000-000000000007'`, // eslint-disable-line key-spacing | ||
| config: sql`'00000000-0000-8000-8000-000000000008'`, // eslint-disable-line key-spacing | ||
| project: sql`'00000000-0000-8000-8000-000000000009'`, // eslint-disable-line key-spacing | ||
| role: sql`'00000000-0000-8000-8000-000000000010'`, // eslint-disable-line key-spacing | ||
| assignment: sql`'00000000-0000-8000-8000-000000000011'`, // eslint-disable-line key-spacing | ||
| audit: sql`'00000000-0000-8000-8000-000000000012'`, // eslint-disable-line key-spacing | ||
| system: sql`'00000000-0000-8000-8000-000000000013'`, // eslint-disable-line key-spacing | ||
| singleUse: sql`'00000000-0000-8000-8000-000000000014'`, // eslint-disable-line key-spacing | ||
| dataset: sql`'00000000-0000-8000-8000-000000000015'`, // eslint-disable-line key-spacing | ||
| public_link: sql`'00000000-0000-8000-8000-000000000016'`, // eslint-disable-line key-spacing | ||
| }; | ||
|
|
||
| const uuidFor = acteeId => { | ||
| if (acteeId == null) return null; | ||
| else if (Object.prototype.hasOwnProperty.call(sqlSpecial, acteeId)) return sqlSpecial[acteeId]; | ||
| else if (acteeId.length === 36) return acteeId; | ||
| else throw new Error(`Unexpected acteeId: '${acteeId}'`); | ||
| }; | ||
|
|
||
| module.exports = { uuidFor }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Copyright 2025 ODK Central Developers | ||
| // See the NOTICE file at the top-level directory of this distribution and at | ||
| // https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| // This file is part of ODK Central. It is subject to the license terms in | ||
| // the LICENSE file found in the top-level directory of this distribution and at | ||
| // https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| // including this file, may be copied, modified, propagated, or distributed | ||
| // except according to the terms contained in the LICENSE file. | ||
|
|
||
| /* eslint-disable key-spacing, indent */ | ||
|
|
||
| const fkTables = [ | ||
| 'actors', | ||
| 'assignments', | ||
| 'audits', | ||
| 'datasets', | ||
| 'forms', | ||
| 'projects', | ||
| ]; | ||
|
|
||
| // These values must never change. If special actees are added or removed in | ||
| // future, it is of no concern to this migration. | ||
| const specialActees = { | ||
| '*': '00000000-0000-8000-8000-000000000001', | ||
| actor: '00000000-0000-8000-8000-000000000002', | ||
| group: '00000000-0000-8000-8000-000000000003', | ||
| user: '00000000-0000-8000-8000-000000000004', | ||
| form: '00000000-0000-8000-8000-000000000005', | ||
| submission: '00000000-0000-8000-8000-000000000006', | ||
| field_key: '00000000-0000-8000-8000-000000000007', | ||
| config: '00000000-0000-8000-8000-000000000008', | ||
| project: '00000000-0000-8000-8000-000000000009', | ||
| role: '00000000-0000-8000-8000-000000000010', | ||
| assignment: '00000000-0000-8000-8000-000000000011', | ||
| audit: '00000000-0000-8000-8000-000000000012', | ||
| system: '00000000-0000-8000-8000-000000000013', | ||
| singleUse: '00000000-0000-8000-8000-000000000014', | ||
| dataset: '00000000-0000-8000-8000-000000000015', | ||
| public_link: '00000000-0000-8000-8000-000000000016', | ||
| }; | ||
| if (Object.keys(specialActees).length !== | ||
| new Set(Object.values(specialActees)).size) { | ||
| throw new Error('Check specialActees values are unique'); | ||
| } | ||
|
Comment on lines
+41
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel we really need this little inline test-in-a-migration. |
||
|
|
||
| const tableName = t => t.padStart(16, ' '); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this for aesthetics?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this purely for aesthetics? We don't look at the migration output that much, especially not on an ongoing basis going forward, so... leave it out, less is more? If I want to look at formatted SQL code I'll just pipe it through my SQL formatter on an ad-hoc basis. |
||
|
|
||
| const up = (db) => db.raw(` | ||
| -- drop constraints | ||
| ${fkTables.map(t => `ALTER TABLE ${tableName(t)} DROP CONSTRAINT IF EXISTS ${t}_acteeid_foreign;`).join('\n ')} | ||
| -- update references to their special values | ||
| ${Object.entries(specialActees).flatMap(([ str, uuid ]) => [ | ||
| `UPDATE actees SET id='${uuid}' WHERE id='${str}';`, | ||
| `UPDATE actees SET species='${uuid}' WHERE species='${str}';`, | ||
| ...fkTables.map(t => `UPDATE ${tableName(t)} SET "acteeId"='${uuid}' WHERE "acteeId"='${str}';`), | ||
| ]).join('\n ')} | ||
| -- change column types | ||
| ALTER TABLE actees ALTER COLUMN id TYPE UUID USING id::UUID; | ||
| ALTER TABLE actees ALTER COLUMN parent TYPE UUID USING parent::UUID; | ||
| ALTER TABLE actees ALTER COLUMN species TYPE UUID USING species::UUID; | ||
| ${fkTables.map(t => `ALTER TABLE ${tableName(t)} ALTER COLUMN "acteeId" TYPE UUID USING "acteeId"::UUID;`).join('\n ')} | ||
| -- add missing special actees | ||
| INSERT INTO actees (id, species) VALUES('${specialActees.system}', '${specialActees['*']}'); | ||
| INSERT INTO actees (id, species) VALUES('${specialActees.singleUse}', '${specialActees['*']}'); | ||
| INSERT INTO actees (id, species) VALUES('${specialActees.dataset}', '${specialActees['*']}'); | ||
| INSERT INTO actees (id, species) VALUES('${specialActees.public_link}', '${specialActees['*']}'); | ||
| -- re-add constraints | ||
| ALTER TABLE actees ADD CONSTRAINT actees_parent_foreign FOREIGN KEY(parent) REFERENCES actees(id); | ||
| ALTER TABLE actees ADD CONSTRAINT actees_species_foreign FOREIGN KEY(species) REFERENCES actees(id); | ||
| ${fkTables.map(t => `ALTER TABLE ${tableName(t)} ADD CONSTRAINT ${tableName(t)}_acteeid_foreign FOREIGN KEY("acteeId") REFERENCES actees(id);`).join('\n ')} | ||
| CREATE INDEX idx_fk_actees_species ON "actees" ("species"); | ||
| CREATE INDEX idx_fk_datasets_acteeId ON "datasets" ("acteeId"); | ||
|
Comment on lines
+76
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This (from 83ebf49) is bycatch, right? Bycatch as in an improvement (or bugfix) that you stumble upon while fullfilling the primary mission? Personally I'm not against bycatch especially if it's denoted as such in a commitmessage (the one of 83ebf49 suffices, but it could be made more distinct with a |
||
| `); | ||
|
|
||
| const down = (db) => db.raw(` | ||
| -- TODO reverse all statements from up() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice. Ktuite likes to use down-migrations.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'd be nice. I know ktuite likes to use down-migrations. |
||
| `); | ||
|
|
||
| module.exports = { up, down }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| const { sql } = require('slonik'); | ||
| const { map } = require('ramda'); | ||
| const { uuidFor } = require('../../actz'); | ||
| const { Frame, into } = require('../frame'); | ||
| const { Actor, Blob, Form } = require('../frames'); | ||
| const { getFormFields, merge, compare } = require('../../data/schema'); | ||
|
|
@@ -566,7 +567,7 @@ inner join | |
| inner join (select id from roles where verbs ? 'form.read' or verbs ? 'open_form.read') as role | ||
| on role.id=assignments."roleId" | ||
| where "actorId"=${auth.actor.map((actor) => actor.id).orElse(-1)}) as assignment | ||
| on assignment."acteeId" in ('*', 'form', projects."acteeId", forms."acteeId") | ||
| on assignment."acteeId" in (${uuidFor('*')}, ${uuidFor('form')}, projects."acteeId", forms."acteeId") | ||
| group by forms.id) as filtered | ||
| on filtered.id=forms.id | ||
| where "projectId"=${projectId} and state not in ('closing', 'closed') and "currentDefId" is not null | ||
|
|
@@ -627,7 +628,7 @@ const _getByActeeId = ({ forUpdate, maybeOne, acteeId, defVersion }) => { | |
| sql`* FROM forms` : | ||
| sql`${_unjoiner.fields} FROM forms JOIN form_defs on ${versionJoinCondition(defVersion)}`; | ||
| const conditions = sql` | ||
| WHERE "acteeId"=${acteeId} | ||
| WHERE "acteeId"=${acteeId}::UUID | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the typename is actually in lowercase, it just so happens that things get converted to lowercase unless quoted, so this happens to work, but I don't like things that happen to work if there's a more to-the-point way. Witness: Versus: So the type is called |
||
| AND "deletedAt" IS NULL | ||
| `; | ||
| const maybeForUpdate = forUpdate ? sql`FOR UPDATE` : sql``; | ||
|
|
@@ -695,7 +696,7 @@ inner join | |
| inner join (select id from roles where verbs ? 'form.update') as role | ||
| on role.id=assignments."roleId" | ||
| where "actorId"=${actorId}) as assignment | ||
| on assignment."acteeId" in ('*', 'project', projects."acteeId") | ||
| on assignment."acteeId" in (${uuidFor('*')}, ${uuidFor('project')}, projects."acteeId") | ||
| group by id) as filtered | ||
| on filtered.id=forms."projectId"`} | ||
| where ${sqlEquals(condition)} and forms."deletedAt" is ${deleted ? sql`not` : sql``} null | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,7 +94,10 @@ describe('database migrations: removing default project', function() { | |
| })); | ||
| }); | ||
|
|
||
| describe('database migrations: intermediate form schema', function() { | ||
| // skipped: test setup relies on populateUsers(), which relies on application | ||
| // code which has changed since the test was written. | ||
| // REVIEW: this test will never work again, and should probably be deleted. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. review? is this addressed to me? 🤔 |
||
| describe.skip('database migrations: intermediate form schema', function() { | ||
alxndrsn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this.timeout(20000); | ||
|
|
||
| it('should test migration', testServiceFullTrx(async (service, container) => { | ||
|
|
@@ -257,7 +260,10 @@ describe('database migrations: 20230123-01-remove-google-backups', function() { | |
| return actor.id; | ||
| }; | ||
|
|
||
| it('consumes a token', testContainerFullTrx(async (container) => { | ||
| // skipped: test setup relies on createToken(), which relies on application | ||
| // code which has changed since the test was written. | ||
| // REVIEW: this test will never work again, and should probably be deleted. | ||
| it.skip('consumes a token', testContainerFullTrx(async (container) => { | ||
| const actorId = await createToken(container); | ||
| const { one } = container; | ||
| const count = () => one(sql`SELECT | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not store these constants as sql-templates. That's a boxing (of sorts) for a particular use case for a particular DB driver. So I'd leave the base definition assumption-free which in this case would mean storing the UUIDs as string values as there is no UUID type in the stdlib.
And then of course you can construct this
sqlSpecialspecial purpose mapping automatically from that base assumption-free definition. Alternatively the ensql-ing it could take place insideuuidFor()though of course from a performance-aesthetics POV it's nicer to not repeat that work, which speaks for creating a mapping once on initialization. And then that sql-specific stuff should then probably go intoquery/actees.js, and read the table of constants from a newlib/constants/actees.jswhich contains just the mapping constants and no presumptions about the usage. (actees.jsis imo preferable toactz.js).