Repository navigation
feat: PostgreSQL sink backend - #1687
Merged
Merged
Conversation
Implements the foundation shared by the postgres adapter modules: - `connect()` lazily constructs a `pg.Pool` from the configured connection string and runs an idempotent `CREATE TABLE IF NOT EXISTS` bootstrap exactly once per process. All adapter modules acquire the pool through this function, so the schema is in place before any query is executed against `users` / `repos` / `pushes`. - `query()` is a thin convenience wrapper that awaits `connect()` and delegates to `pool.query`. - `resetConnection()` tears down the pool and bootstrap latch — used by the integration test harness between suites. - `getSessionStore()` returns a `connect-pg-simple` store bound to the same pool. Per issue #1497 it MUST NOT silently return undefined when postgres is the active sink (express-session would silently fall back to MemoryStore), so a missing connection string throws instead. The schema covers the three application tables plus the indexes used by `getPushes` (timestamp DESC) and `getRepo` (name lookup). The session table is left to `connect-pg-simple` via `createTableIfMissing: true`. Refs #1497
Implements the `Sink` push methods against the `pushes` table: - `getPushes`: filters by the same keys the mongo backend supports (error/blocked/allowPush/authorised/canceled/rejected/type) via a small allow-list mapping, then sorts `ORDER BY timestamp DESC` to preserve current backend ordering (issue #1497 must-fix). - `getPush` / `deletePush`: lookups by `id` PK. - `writeAudit`: upsert on `id` with the full Action serialized into the `data` JSONB column and the projection columns kept in sync. Throws `Invalid id` to match mongo behaviour. - `authorise` / `cancel` / `reject`: read-modify-write through `getPush` + `writeAudit`, identical to the mongo flow. `reject` assigns `action.rejection = rejection` so the persisted payload shape (reason / reviewer / timestamp) matches the existing backends. The Action class is reconstructed from the `data` JSONB via the existing `toClass` helper. Refs #1497
Implements the `Sink` user methods against the `users` table:
- `findUser` / `findUserByEmail` / `findUserByOIDC`: lower-case the
lookup keys to match the mongo and fs case-insensitivity behaviour.
- `getUsers`: optional username / email filters with the same
lower-casing; SELECT projects `password` away (matching mongo's
`.project({ password: 0 })`).
- `createUser`: insert with lower-cased username / email.
- `deleteUser`: delete by lower-cased username.
- `updateUser`: dynamic SET-builder that mirrors mongo's partial
upsert. Identity is by `_id` when supplied, otherwise by `username`;
if no matching row exists when keyed on username, a new row is
inserted so callers can patch-or-create without two round trips.
`_id` is exposed as an opaque string (UUID rendered as text) so the
HTTP/UI contract is unchanged versus the mongo backend (which renders
ObjectId via `.toString()`).
Refs #1497
Implements the `Sink` repo methods against the `repos` table. Permissions (`canPush` / `canAuthorise`) are stored as a single JSONB column matching the existing mongo/fs shape, with a TODO marker pointing at a future migration to a normalized `repo_users` join table (open question called out in issue #1497). Notable details: - `addUser*` use `jsonb_set` + a DISTINCT subquery so re-adding an existing user is a no-op, matching the fs adapter's `includes` guard. - `removeUser*` use `coalesce(..., '[]'::jsonb)` around the `array_agg` filter so that removing the last user leaves the array as `[]`, not `null` — issue #1497 explicitly requires this and the reader path additionally defaults `null` arrays to `[]` for belt-and-braces resilience against legacy rows. - `getRepos` accepts the same query keys as the mongo backend (name / project / url) with the same lower-casing on `name`. - `createRepo` returns the row with `_id` populated (UUID rendered as text), matching the mongo backend's contract. Refs #1497
Adds the `postgres` branch to the runtime `start()` selector so a sink config of `type: 'postgres'` resolves to the new adapter modules, and re-exports the full `Sink` surface from `src/db/postgres/index.ts`. The `getSessionStore` return type on the `Sink` interface and on the top-level `src/db/index.ts` re-export is widened from `MongoDBStore | undefined` to `MongoDBStore | Store | undefined`, where `Store` is the express-session base class — `connect-pg-simple` extends it. This keeps the existing mongo / fs callers type-compatible. Refs #1497
Per issue #1497 must-fix: when the active sink is one that promises a persistent session store (currently `mongo` or `postgres`), `db.getSessionStore()` returning undefined must NOT silently fall through to express-session's default `MemoryStore` — that store loses sessions on every restart and is unsafe in any multi-process deployment. `createApp` now resolves the store before registering the session middleware and throws if a persistent backend produced `undefined`. The `fs` backend is unaffected: it has always returned `undefined` deliberately, and falling back to MemoryStore there matches existing single-node-only fs semantics.
Mocks the `query` export from the postgres helper so the suite runs without a live database. Covers: - `getPushes` ordering — asserts the generated SQL contains `ORDER BY timestamp DESC` (issue #1497 must-fix). - `getPushes` column translation — `allowPush` filter maps to the `allow_push` snake_case column. - `getPushes` unknown filter keys are ignored (no spurious WHERE). - `getPush` returns null when the row is absent. - `writeAudit` throws `Invalid id` for non-string ids (mongo parity). - `writeAudit` upserts via `ON CONFLICT (id) DO UPDATE`. - `reject` writes a serialized Action into the `data` JSONB column with the `rejection` field populated — confirming the payload shape matches the existing backends. - `reject` throws when the push is missing.
Covers behaviour-critical paths:
- case insensitivity: findUser / findUserByEmail / createUser /
deleteUser all lower-case their lookup or stored values (parity
with the mongo and fs adapters).
- getUsers omits `password` from the projection (mirrors mongo's
`.project({ password: 0 })`).
- updateUser dispatches on `_id` vs `username`, and when the
username-keyed UPDATE matches nothing it falls back to INSERT —
this is the upsert semantics issue #1497 calls for.
- updateUser throws when given neither `_id` nor `username`.
Targets the parity invariants called out in issue #1497: - `getRepoById` defaults a NULL `users` JSONB to empty arrays — guards against legacy or partial rows and matches the fs/mongo contract. - `getRepo` lower-cases the lookup name. - `createRepo` serialises the default `{canPush:[],canAuthorise:[]}` into the JSONB column and stamps the generated `_id` back onto the returned object. - `addUserCanPush` lower-cases the user value before storing. - `removeUserCanPush` and `removeUserCanAuthorise` emit a SQL fragment that wraps the filtered array in `coalesce(..., '[]'::jsonb)`, so the array remains `[]` when the last user is removed — this is the explicit must-fix from the issue, and an end-to-end check sits in the integration suite.
Mocks the `pg` Pool and `connect-pg-simple` constructors so the suite can exercise the helper without a real database. Covers: - `connect()` is concurrency-safe: many parallel calls share one Pool and run the bootstrap SQL exactly once. - the bootstrap SQL creates `users`, `repos`, and `pushes` (assertion via regex against the inlined statement). - bootstrap failure does not permanently latch the helper: the next `connect()` retries instead of returning the rejected promise. - `query()` surfaces the helpful error message when the configured connection string is missing. - `getSessionStore()` throws (not returns undefined) when the connection string is missing — the explicit must-fix from issue #1497 to prevent a silent MemoryStore fallback. - `getSessionStore()` constructs the `connect-pg-simple` store with `createTableIfMissing: true` and shares the helper's pool. Full suite: 791 unit tests passing (+27 new), zero regressions.
Adds the scaffolding for postgres-backed integration tests: - `vitest.config.integration.postgres.ts`: separate vitest config that scopes `include` to `test/db/postgres/**/*.integration.test.ts`, sets `RUN_POSTGRES_TESTS=true`, points `CONFIG_FILE` at the dedicated postgres test config, and uses a single-fork pool so the lazy pg.Pool is shared across the suite. Mirrors the shape of `vitest.config.integration.ts` for mongo. - `test/setup-integration-postgres.ts`: connects a `pg.Client`, truncates the app tables (and the connect-pg-simple `session` table if it exists) between tests, drops them in `afterAll`, and calls `resetConnection()` + `invalidateCache()` so each test sees a fresh helper state. - `test-integration.postgres.proxy.config.json`: minimal config with a single enabled postgres sink and local auth, so `getDatabase()` resolves to postgres without the default fs entry winning first. - `package.json`: adds `npm run test:integration:postgres`. No suites yet — added in the next commit.
Parity with the mongo pushes integration suite, gated on RUN_POSTGRES_TESTS=true (set automatically by vitest.config.integration.postgres.ts). The suite is skipped in normal `npm test` runs and only executes against a real Postgres in the dedicated `npm run test:integration:postgres` task. The added test that goes beyond the mongo parity: - `getPushes` returns results in descending timestamp order across three rows with deliberately distinct timestamps — exercising the must-fix ordering requirement end-to-end against a real database. Otherwise the assertions mirror the existing mongo integration suite verbatim so backend parity is verifiable side-by-side.
Parity with the mongo users integration suite, gated on RUN_POSTGRES_TESTS=true. Mirrors the same case-insensitivity and filtering assertions, plus one additional test exercising the upsert-on-username path through `updateUser` end-to-end (the mongo adapter has this via its `upsert: true`, our postgres adapter implements it as an `UPDATE … WHERE username` fallback to `INSERT`). `getUsers` asserts `password` is `null` in list responses rather than `undefined`: the postgres SELECT projects `NULL::text AS password`, which round-trips as `null` rather than being elided from the JSON shape — semantically equivalent to mongo's omission for the API consumers.
Parity with the mongo repo integration suite, gated on RUN_POSTGRES_TESTS=true. The permission-JSONB block is the centrepiece — it exercises the explicit issue #1497 must-fix end-to-end against a real database: - starts with empty arrays in the JSONB column. - adding a user is deduplicated (re-adding does not double-insert). - removing the *last* user leaves the array as `[]`, not `null`. - the invariant applies symmetrically to `canAuthorise`. - removing one user from a multi-user list keeps the rest intact. Skipped without postgres available: full unit suite is 791 passing, 90 skipped (45 mongo + 18 postgres-pushes + 14 postgres-users + 13 postgres-repo), zero failures, zero regressions.
Adds a `postgres:16` service container to the `build-ubuntu` job and a new `PostgreSQL Integration Tests` step that runs `npm run test:integration:postgres` against it. The service uses the default `postgres` superuser with database `git_proxy_test`, matching the connection string our adapter and test harness default to. Per the issue's "Open Questions" section, a single Postgres version is sufficient for the initial lane; a broader matrix can follow once the backend has soaked in. Refs #1497
Documents the new `postgres` backend in the `sink` section of the architecture reference: - Lists `postgres` as a supported sink alongside `fs` and `mongo`. - Shows the minimal config block. - Documents the `GIT_PROXY_POSTGRES_CONNECTION_STRING` env-var fallback. - Calls out the v1 limitations explicitly (no migration tooling, no AWS RDS IAM auth, JSONB permissions, no split PG env vars, fail- loudly on missing connection string). Refs #1497
Brings in the config deprecation refactor (#1708) and the agent-base and c8 dependency bumps. Clean merge, no conflicts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kriswest
reviewed
Sep 23, 2026
Contributor
There was a problem hiding this comment.
Looks pretty good - a few inline comments to look at.
One thing I think is off is the ENV var handling - I think its also wrong for mongo DB connection string which probably casued that. Every other ENV var overrides config I believe - which makes sense as we set defaults in the default config, so those are really the fallback: Env var -> user config -> default config
Expect conflicts with the CVE resolution thats also refactoring gitAccount into scmIdentities.
Signed-off-by: Kris West <kristopher.west@natwest.com>
Contributor
|
@dcoric looks like I broke the CI with the conflict resolution. Will need the lock file regenerating with |
The lock from the main merge was written by npm 11, which drops two nested entries that npm 10 requires (@aws-sdk/rds-signer/node_modules/@aws-sdk/credential-providers and vite-tsconfig-paths/node_modules/typescript), so npm ci failed on every CI job. Regenerated with npm 10.9.8, the version CI uses on node 22; the result installs cleanly under both npm 10 and npm 11.
Every other GitProxy environment variable overrides the config file, with precedence env var, then user config, then default config. The postgres connection string was the opposite: the env var only applied when the config had no connectionString, and since the default config ships one it could never take effect. That also broke the documented use of GIT_PROXY_POSTGRES_CONNECTION_STRING to run schema migrations with elevated credentials. The env var now overrides the config when set, with tests for both directions. The schema descriptions and architecture doc describe the new precedence. The mongo connection string has the same inversion and is left unchanged here, to be aligned separately so existing mongo deployments are not affected by this PR. Also sets additionalProperties: false on the postgres sink entry and its pool and awsIamAuth objects so the generated types no longer accept arbitrary properties (ssl stays open since it passes through any Node TLS option), and documents hasDiscreteFields.
Contributor
Author
|
No worries, done: I regenerated it with npm 10.9.8, which is what CI uses on node 22. A plain Once installs work, expect one test still failing, and it fails on main too: the push-id scoping test from the 1074b9b fork merge uses a fake parent commit id ( |
This was referenced Oct 4, 2026
17 of 24 tasks
kriswest
reviewed
Oct 5, 2026
kriswest
reviewed
Oct 5, 2026
kriswest
left a comment
Contributor
There was a problem hiding this comment.
Looks pretty good to me - just some small copy issues to sort out then ready to merge
Signed-off-by: Denis Ćorić <denis85@gmail.com>
Signed-off-by: Denis Ćorić <denis85@gmail.com>
Signed-off-by: Denis Ćorić <denis85@gmail.com>
Signed-off-by: Denis Ćorić <denis85@gmail.com>
jescalada
approved these changes
Oct 6, 2026
This was referenced Oct 6, 2026
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
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.
Adds PostgreSQL as a supported sink backend, complete with schema migrations, data migration, and production connection/auth options.
This is an integration branch. The PostgreSQL work was split into small, reviewable PRs that build on each other, and merging them into
mainone at a time would leave a half-configured backend in place for days at a time. They target this branch instead, so each keeps its own review and CI, andmainsees the finished backend in one piece.Collected PRs
repo_usersjoin tablesink-parityagent skill and AGENTS.md steering for backend parityAll nine pieces have merged and each was reviewed and CI-verified individually against this branch (full pipeline including the PostgreSQL integration lane). The branch was synchronized with
mainon 2026-10-04. This PR is the complete feature, ready for final review. Related issues: #1688, #1690, #1691.What lands
postgressink selectable via the existingsinkconfig, alongsidefsandmongoSinkinterface parity with the mongo and NeDB backends, includinggetRepoPushRollupsByCanonicalUrl,getPushesForUserProfile,updateRepoand the migration hooksmigrate-to-postgrescommand for existing mongo/fs deploymentsPG*env vars, with TLS and pool tuningsink-parityskill so future adaptor changes keep all backends alignedNote on migrations
mainhas since grown a cross-backend migration framework insrc/db/migrations, which records logical migrations by string id throughSinkhooks. That is a different concern from #1581, which versions the postgres DDL itself and serialises concurrent runs with an advisory lock. Both are kept: the postgres adapter implements the framework hooks insrc/db/postgres/migrations.ts, and the DDL runner lives alongside it insrc/db/postgres/schemaMigrations.ts.Existing deployments are unaffected: the default sink remains the filesystem backend and
postgresis opt-in.Integration update (2026-10-04)
Merged FINOS main at
41dcc473and regenerated the configuration types. PostgreSQL now implements provider-scopedscmIdentitiesandfindUserByScmIdentity, rejects ambiguous identity matches, and persistsmustChangePasswordthrough create, update, and read operations. Append-only schema migration 8 adds the fields and lookup index. Legacy account values remain available to the shared identity migration and rollback, including after data imports.Local validation: production and CLI builds, server type check, lint (one existing Cypress warning), formatting, 1,693 unit tests, 42 CLI tests, and 75 PostgreSQL integration tests passed. A smoke check through the compiled database service verified identity linking/unlinking and password-change flag persistence against PostgreSQL 16. GitHub CI runs on the updated head.
Fresh CI exposed two existing test failures. SSH signature tests now generate Ed25519 keys with Node crypto, preserving leading zero bytes that ssh2's key generator sometimes strips. The synthetic error test's heap-allocation threshold was replaced with a real malformed-push parsing test that checks rejection state and audit details. The affected 60 SSH and 17 proxy tests pass locally.