feat: normalise PostgreSQL repo permissions into a repo_users join table - #1590
Merged
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 86.49% 86.48% -0.02%
=================================================
Files 108 108
Lines 5962 5986 +24
Branches 1106 1111 +5
=================================================
+ Hits 5157 5177 +20
Misses 527 527
- Partials 278 282 +4 ☔ 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 👍🏼
…kfill Review feedback on the repo_users normalisation: - updateRepo issues several statements (row update, permission replacement, last_modified bump); they now run in one transaction via a new withTransaction helper, so a failure partway cannot leave a repo without its roles. - The backfill migration lowercases usernames to match the runtime writers, so legacy mixed-case JSONB entries stay retrievable; ON CONFLICT collapses case-only duplicates.
…-repo-users # Conflicts: # test/db/postgres/schemaMigrations.integration.test.ts
…ateRepo case handling
… module The backfill integration test still imported MIGRATIONS from src/db/postgres/migrations, which after the split holds the cross-backend framework hooks and no longer exports it; the named import resolved to undefined and the test crashed on first real lane execution. The DDL list and pool-level runner live in schemaMigrations.
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)