feat: PostgreSQL sink backend - #1687
Draft
dcoric wants to merge 82 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 #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 #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 #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 #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 #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 #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
# 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.
main now ships a cross-backend migration framework (src/db/migrations) that records logical migrations by string id through Sink hooks, and the postgres adapter implements those hooks in src/db/postgres/migrations.ts. That is a different concern from this branch, which versions the postgres DDL itself and serialises concurrent runs with an advisory lock. Both are kept: - src/db/postgres/migrations.ts holds the Sink hooks for main's framework - src/db/postgres/schemaMigrations.ts holds the DDL runner from this branch, renamed so the two no longer collide on one filename The DDL runner replaces the CREATE TABLE bootstrap, so it now also creates the framework's bookkeeping table as migration v3.
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Dependency ReviewThe following issues were found:
OpenSSF ScorecardScorecard details
Scanned Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1687 +/- ##
=======================================
Coverage 86.06% 86.06%
=======================================
Files 100 100
Lines 5548 5548
Branches 988 988
=======================================
Hits 4775 4775
Misses 525 525
Partials 248 248 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 (#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.
# Conflicts: # src/db/postgres/helper.ts
… 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.
feat: add PostgreSQL as a supported sink backend
feat: versioned schema migration runner for the PostgreSQL sink
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.
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 parityThe diff below reflects what has merged so far. This PR stays a draft and fills out as the remaining pieces land; it will be marked ready for review once the backend is complete. 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.