feat: normalise PostgreSQL repo permissions into a repo_users join table - #1590
Open
dcoric wants to merge 22 commits into
Open
feat: normalise PostgreSQL repo permissions into a repo_users join table#1590dcoric wants to merge 22 commits into
dcoric wants to merge 22 commits into
Conversation
…o-users Resolutions: the user public_keys / optional email migration takes version 2 (it ships first with the schema-migrations PR), so the repo_users backfill moves to version 3 and the JSONB column drop to version 4; version assertions and table lists updated accordingly. repo.ts keeps the repo_users implementation and drops the remaining issue reference in line with the review feedback on finos#1532.
The migration integration suite reset to an empty database before every run, so migration v3's INSERT...SELECT backfill always executed over zero rows and the JSONB-to-repo_users data migration was never observed. Add a test that stages the pre-v3 schema (v1+v2 applied by hand, legacy `users` JSONB column present), seeds repos with single-user, multi-user and same-user-in-both-roles permissions, runs the remaining migrations, and asserts every entry lands in repo_users and the legacy column is dropped. Also correct an off-by-one comment: the legacy JSONB column is dropped by migration v4, not v3.
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/postgres #1590 +/- ##
=================================================
+ Coverage 90.41% 90.58% +0.16%
=================================================
Files 87 93 +6
Lines 8661 9215 +554
Branches 1603 1711 +108
=================================================
+ Hits 7831 8347 +516
- Misses 799 835 +36
- Partials 31 33 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…o-users The schema migrations branch renamed the postgres DDL runner to schemaMigrations.ts so it no longer collides with the Sink migration hooks main expects in migrations.ts. The repo_users table and the drop of the legacy JSONB column move with it, renumbered as versions 4 and 5 to sit after the framework bookkeeping table. Also implements updateRepo, which main added to the Sink interface. Since permissions live in repo_users rather than a column on repos, a supplied users object replaces that repo's rows instead of overwriting JSONB.
…o-users Carries the repo dateCreated/lastModified parity work onto the repo_users variant of the adapter: the join query returns the date columns, createRepo stamps them, updateRepo maps them, and every permission change bumps last_modified on repos with a separate update, matching the mongo and fs backends.
jescalada
reviewed
Aug 26, 2026
jescalada
left a comment
jescalada
left a comment
Contributor
There was a problem hiding this comment.
Just a few comments on this one 👍🏼
| -- legacy repos.users column is dropped in a later migration once the adapter | ||
| -- reads and writes repo_users instead. | ||
| INSERT INTO repo_users (repo_id, username, role) | ||
| SELECT r._id, elem.username, 'canPush' |
Contributor
There was a problem hiding this comment.
Wondering if it's okay to select the usernames directly rather than the lowercased version here and a few lines below? Notice how in addUserToRole and removeUserFromRole we're lowercasing them:
const addUserToRole = async (
_id: string,
user: string,
role: 'canPush' | 'canAuthorise',
): Promise<void> => {
await query(
`INSERT INTO repo_users (repo_id, username, role)
VALUES ($1, $2, $3)
ON CONFLICT DO NOTHING`,
[_id, user.toLowerCase(), role],
);
await query(`UPDATE repos SET last_modified = $2 WHERE _id = $1`, [
_id,
new Date().toISOString(),
]);
};We might also want a few unit tests to cover this specifically so that previously added user entries are still retrievable
| ON CONFLICT DO NOTHING; | ||
|
|
||
| INSERT INTO repo_users (repo_id, username, role) | ||
| SELECT r._id, elem.username, 'canAuthorise' |
| if (users !== undefined) { | ||
| await query(`DELETE FROM repo_users WHERE repo_id = $1`, [_id]); | ||
| for (const username of users.canPush ?? []) { | ||
| await addUserToRole(_id, username, 'canPush'); |
Contributor
There was a problem hiding this comment.
Should we be handling errors in these? There are a few operations happening in updateRepo - we should do something to prevent invalid or half-filled entries (repos added successfully, but without the user roles assigned) in case there's some unexpected error (disconnection/network glitch, etc.)
Do we have anything to ensure rollbacks happen on errors?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Normalises PostgreSQL repo permissions out of the
repos.usersJSONB column into a dedicatedrepo_users(repo_id, username, role)join table, the follow-up to the v1 JSONB parity shipped in #1532.repo_users_table) createsrepo_userswith a composite primary key(repo_id, username, role), aCHECK (role IN ('canPush','canAuthorise'))constraint, arepo_idindex, andON DELETE CASCADEtorepos. It backfills the table from the existing JSONB permissions in the same step.drop_repos_users_jsonb) drops the now-unusedrepos.userscolumn.src/db/postgres/repo.ts) reads permissions via a singleLEFT JOIN+array_agg(...) FILTER (...)(no N+1) and reconstructs the{ canPush, canAuthorise }shape callers expect; writes go through idempotentINSERT ... ON CONFLICT DO NOTHING/DELETE.repo_usersbackfill across single-user, multi-user and same-user-in-both-roles repos.Depends on
This relies on #1558 / #1581 (the versioned schema-migration runner): the backfill is delivered as migration v3 and needs the runner to apply it. It should merge after #1532 and #1581.
Known follow-ups (non-blocking)
createRepo's optional create-time permission inserts are not wrapped in a single transaction. Impact is low because the publiccreateRepowrapper always passes empty permission arrays, so the loop is not exercised in the normal app flow; a follow-up can run it on one client insideBEGIN/COMMIT.$pushcan store duplicates). Both are functionally invisible to the.includes()-based consumers.Related Issue
Resolves #1559
Checklist
General
Documentation
Configuration
config.schema.json) was modified: (not modified in this PR)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)