feat: configurable PostgreSQL connection (discrete fields, TLS, pool) - #1584
Open
dcoric wants to merge 76 commits into
Open
feat: configurable PostgreSQL connection (discrete fields, TLS, pool)#1584dcoric wants to merge 76 commits into
dcoric wants to merge 76 commits into
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
| * loudly rather than silently defaulting to `localhost`. | ||
| */ | ||
| const hasConnectionConfig = (db: DatabaseConfig): boolean => | ||
| Boolean(db.connectionString || db.host || process.env.PGHOST); |
Contributor
There was a problem hiding this comment.
This only checks PGHOST, but the message says "or the PG* env variables". A setup with PGUSER/PGDATABASE and no PGHOST is valid but gets refused. Could the check also look at PGUSER/PGDATABASE/PGHOSTADDR so only the truly-empty case is blocked?
| */ | ||
| const buildPoolConfig = (db: DatabaseConfig): PoolConfig => { | ||
| const config: PoolConfig = {}; | ||
| if (db.connectionString) { |
Contributor
There was a problem hiding this comment.
When both connectionString and the discrete fields are set, the discrete fields are silently ignored. It's documented as the intended precedence, but it's an easy operational trap (e.g. someone changes host to repoint the DB, nothing happens because an old connectionString is still winning with no signal). Worth a console.warn here noting the discrete fields are being ignored?
# Conflicts: # website/docs/architecture/architecture.md
Brings the branch up to date with main, including the new UI (React 19), vitest 4 and TypeScript 6. Conflicts resolved: - package.json / package-lock.json: took main's UI stack and dependency versions, kept pg, connect-pg-simple and @types/pg. Dropped perfect-scrollbar, react-html-parser and react-router-dom, which the new UI replaced. - src/db/types.ts: kept the session store widened to express-session Store (needed by connect-pg-simple) alongside main's new getRepoPushRollupsByCanonicalUrl. - src/db/index.ts: kept main's runMigrations alongside the widened getSessionStore and ensureSessionStoreReady. - src/config/generated/config.ts: kept postgres in DatabaseType and took main's new AuthType. The Sink interface gained several members while this branch was behind, so the postgres adapter implements them here to keep backend parity: - getRepoPushRollupsByCanonicalUrl and getPushesForUserProfile, matching the mongo and fs semantics (URL canonicalisation happens in Node so the results agree across backends) - updateRepo, a partial update mirroring mongo's $set / $unset behaviour - deriveCreatedAt, getAppliedMigrations, recordMigration and unrecordMigration, backing main's migration framework with a migrations table. Postgres keys are random UUIDs and carry no embedded creation time, so deriveCreatedAt returns undefined as the fs backend does.
CI installs with node 22 / npm 10, whose resolver nests a typescript 5.9.3 under vite-tsconfig-paths. The lock was regenerated with npm 11, which dedupes that entry away, so npm ci failed on every runner. Regenerating with npm 10.9.8 satisfies both npm 10 and npm 11.
main added dateCreated/lastModified to Repo and a startup migration (finos#1681) backfills them through sink.updateRepo. The postgres adapter dropped both fields from its update allowlist, which made that migration throw and abort startup, and reads never returned the dates so the backfill would re-run every boot. Bring the adapter to parity with the mongo and fs backends: TEXT columns on repos, defaults on create, both fields updatable, dates returned from all reads, and last_modified bumped on permission changes.
dcoric
marked this pull request as ready for review
August 24, 2026 10:20
… doc Documents the deliberate choices behind the adapter so reviewers can see the rationale in one place: pushes kept as JSONB documents with typed filter columns, typed rows with JSONB edges for users and repos, server-generated UUID ids, ISO-8601 string timestamps, mongo-matching case rules, best-effort email uniqueness, connect-pg-simple sessions, and loud startup failure when no connection resolves.
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
3 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
Makes the PostgreSQL sink's connection fully configurable, bringing it closer to parity with the mongo backend.
host,port,user,password,database, used whenconnectionStringis not set. If those are not set either, the standardPGHOST/PGPORT/PGUSER/PGPASSWORD/PGDATABASEenvironment variables are read by the client.ssl:truefor TLS with default certificate verification, or an object of TLS options (rejectUnauthorized,ca,cert,key, ...).pool: connection pool tuning (max,idleTimeoutMillis,connectionTimeoutMillis).Precedence:
connectionString(the config field, thenGIT_PROXY_POSTGRES_CONNECTION_STRING) wins; otherwise the discrete fields; otherwise thePG*environment variables.sslandpoolapply in all cases. If none of these resolve to a connection, GitProxy refuses to start rather than silently defaulting tolocalhost.Unit tests cover the precedence, discrete fields, env fallback,
ssl(boolean and object) andpooloptions.Related Issue
Resolves #1560
Checklist
General
Documentation
Configuration
config.schema.json) was modified:npm run generate-config-types)npm run gen-schema-docrequires the externalgenerate-schema-doctool; needs running in an environment that has it)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)