feat: add PostgreSQL as a supported sink backend#1532
Draft
dcoric wants to merge 22 commits into
Draft
Conversation
Stub modules for the upcoming Postgres sink adapter. No behavior yet — each adapter file is a placeholder so subsequent commits can land each concern (helper, pushes, repos, users) in isolation. Refs finos#1497
Adds a third `oneOf` entry for the `database` definition in the JSON schema and regenerates the TypeScript config types. `connectionString` is optional at the schema level so an env-var fallback can supply it at runtime (added in a follow-up commit). Refs finos#1497
Adds `GIT_PROXY_POSTGRES_CONNECTION_STRING` to `serverConfig` and wires the postgres branch of `getDatabase()` to populate `connectionString` from it when the user config omits one. Mirrors the existing pattern used for `GIT_PROXY_MONGO_CONNECTION_STRING`. Refs finos#1497
Documents the new sink type in the shipped default config. Disabled by default so the `fs` backend continues to be selected unless an operator explicitly enables postgres. Refs finos#1497
Runtime deps for the new PostgreSQL sink adapter: - `pg` — node-postgres client + Pool, used by the adapter modules. - `connect-pg-simple` — express-session store backed by Postgres, used to persist UI sessions when the postgres sink is active. - `@types/pg`, `@types/connect-pg-simple` — TypeScript definitions. Refs finos#1497
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 finos#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 finos#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 finos#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 finos#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 finos#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 finos#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 finos#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 finos#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 finos#1497
Per issue finos#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 finos#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 finos#1497 calls for.
- updateUser throws when given neither `_id` nor `username`.
Targets the parity invariants called out in issue finos#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 finos#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 finos#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 finos#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 finos#1497
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (74.31%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1532 +/- ##
==========================================
- Coverage 90.21% 89.04% -1.18%
==========================================
Files 69 74 +5
Lines 5511 5949 +438
Branches 944 1012 +68
==========================================
+ Hits 4972 5297 +325
- Misses 521 634 +113
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Closes #1497
Summary
Adds PostgreSQL as a first-class
sink[]backend alongside the existingfsandmongobackends, per the scope in #1497. Additive only — no behavior change forfsormongousers.postgresis a new value in the sink schema withconnectionString+enabledGIT_PROXY_POSTGRES_CONNECTION_STRINGenv var (mirrors the mongo env-var pattern)src/db/postgres/implements the fullSinkinterface againstpg, with an idempotentCREATE TABLE IF NOT EXISTSbootstrap on startupconnect-pg-simplebound to the same pool, withcreateTableIfMissing: truegetSessionStore()returnsundefined— prevents the silent MemoryStore fallback the issue calls outvitest+ mockedpg.Pool) cover all adapter modules and the helper bootstraptest/db/postgres/*.integration.test.tscover parity with the mongo suites, gated onRUN_POSTGRES_TESTS=truepostgres:16service container in thebuild-ubuntujob + aPostgreSQL Integration Testsstepdocs/Architecture.mdwith config example, env-var fallback, and v1 limitationsIssue must-fix coverage
MemoryStoresrc/service/index.tsguard +getSessionStore()throws when connection string missing; helper unit testreject()payload shape parityaction.rejection = rejection(identical to mongo); unit + integration testsORDER BY timestamp DESCingetPushes; unit + integration testscanPush/canAuthoriseuser leaves[], notnullcoalesce(..., '[]'::jsonb)in JSONB UPDATE; unit + integration tests_idhandled correctlyObjectId.toString())postgres:16service container in.github/workflows/ci.ymldocs/Architecture.mdupdatedOpen Questions from the issue — answered
TODO(#1497-followup)comment seeded insrc/db/postgres/repo.ts.GIT_PROXY_POSTGRES_CONNECTION_STRINGin v1.postgres:16lane; broader matrix can follow.Out of scope (explicit follow-ups)
repo_usersjoin tablePGHOST/PGPORT/PGDATABASE/PGUSER/PGPASSWORDenv varsfsormongotopostgresLocal verification
npm run check-types:server— passesnpm run lint— passesnpm test— 791 passing, 90 skipped (45 mongo + 45 postgres integration, allRUN_*_TESTS-gated), zero failures, zero regressionspostgres:16smoke run will execute in CI on this PR via the new laneTest plan
PostgreSQL Integration Testssteppostgressink locally, restart the server, verifyLoading PostgreSQL database adaptorlog linerejection.reason+rejection.reviewersurvive in thepushes.dataJSONBcanPushuser — verifyrepos.users.canPushis[]inpsql, notnullpostgressink — verify UI session persists (proves the connect-pg-simple store is wired, not MemoryStore)