Introduce backend-neutral database facade and transaction boundary - #119
Open
felixgateru wants to merge 2 commits into
Open
felixgateru wants to merge 2 commits into
felixgateru wants to merge 2 commits into
Conversation
…-001)
Adds the internal storage boundary described in
product-docs/development/database-backends/RFC.md: DatabaseKind and a
cloneable Database enum own pool construction, URL scheme classification,
migration dispatch, and sanitized (credential-free) startup logging;
DbTransaction wraps a transaction with nested-savepoint support via its own
begin(). Only a Postgres variant exists -- DATABASE_URL schemes other than
postgres://[ql] (including sqlite://) now fail startup fast, before any pool
or migration work, with an actionable message.
AppState.pool: PgPool is replaced by AppState.db: Database. AppState::new
takes impl Into<Database> so every existing call site passing a raw PgPool
(main.rs, ~20 test fixtures) keeps compiling unchanged via the From<PgPool>
impl. A transitional AppState::pool()/Database::as_postgres() accessor
returns the same &PgPool as before, so the ~640 existing repository/handler
call sites that read state.pool need only the mechanical `.pool` ->
`.pool()` rename (this commit) -- no query logic changes. Removing that
transitional accessor from every call site is Milestone A's exit criterion
(DB-003 through DB-006), not this phase.
Zero intended behavior change. Verified: cargo fmt --check, cargo clippy
--lib --bins -- -D warnings, full workspace compiles (lib + bin + all
integration test binaries), 332 non-DB unit tests, and ~115 DB-gated
integration tests across authz, identity/credentials, tenants, PKI,
audit/outbox, bootstrap, and soft-delete/restore run green against a real
Postgres instance including the single-connection-pool canaries
(creating/deleting_role_assignment_works_with_a_single_connection_pool).
One pre-existing test-order flake (m34_pki_renewal, unrelated fixture
collision with m30/32/33 sharing one database) was confirmed to reproduce
identically on unmodified main -- not a regression from this change.
Also collapses two pre-existing `else { if .. }` clippy findings in
src/certs/graphql.rs (unrelated to this change, but needed to unblock
`cargo clippy -- -D warnings` as a green gate for this phase).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cation (DB-002) Migrates every transaction-taking function (all ~200 sites across 22 identity/authz/tenants/certs/PKI/audit/bootstrap files) from sqlx::Transaction<'_, Postgres> to the backend-neutral db::DbTransaction<'_> introduced in DB-001, and the six audit.rs commit helpers (commit_with_audit, commit_with_observation, commit_observed_with_cache[_*], observe_in_tx) to take it by value/reference instead of a concrete Postgres transaction. Mechanical, behavior-preserving transform, not a rewrite: - `tx: &mut Transaction<'_, Postgres>` -> `tx: &mut DbTransaction<'_>`; by-value commit-helper params the same way. - Every direct query call against the connection (`&mut **tx` / `&mut *tx`) becomes `tx.as_postgres_mut()`, which returns the exact same live &mut PgConnection -- same SQL, same connection, same transaction. - Nested savepoints (PKI serial-collision retries in certs::service) use DbTransaction::begin(&mut self), mirroring sqlx's own Transaction::begin and preserving the existing SAVEPOINT-scoped rollback behavior. - Guardrail validators that intentionally take &mut PgConnection directly (guardrails::*, *_on_connection) are unchanged; call sites now pass tx.as_postgres_mut() instead of the connection reference directly. - A "wrapper" function that keeps its public &PgPool parameter (kept because tests and internal callers depend on it, per AGENTS.md) opens its transaction via `Database::from(pool.clone()).begin()` instead of `pool.begin()`, so it still hands a DbTransaction to the _in_tx sibling it calls -- zero signature change for those ~40 callers. error.rs: introduces DatabaseErrorKind (NotFound/Unique/ForeignKey/Check/ Internal) and classify_database_error, and routes db_err/IntoResponse/ tonic::Status through it instead of the ad hoc constraint-code match inline at each site. Only the classification that Postgres already produces is represented -- no busy/unavailable variant, since nothing produces one yet and adding an unreachable branch either does nothing or risks silently changing the fallback-to-500 behavior for errors like PoolTimedOut. That stays scoped to whichever backend phase first needs it. restore_conflict/entity_write_conflict keep matching on the raw sqlx::Error directly, since they need the violated constraint's name, not just its kind. Zero intended behavior change. Verified: cargo fmt --check, cargo clippy -- -D warnings (lib+bins, the AGENTS.md-specified gate) and cargo clippy --tests (only 3 findings, all pre-existing on unmodified code and unrelated to this change), full workspace compiles (lib + bin + all 60+ integration test binaries), 334 non-DB unit tests, and ~230 DB-gated integration tests across authz, identity/credentials, tenants, PKI (issuance/renewal/revocation/CRL/OCSP/enrollment/lifecycle-automation/ purge-after-revocation), audit/outbox, bootstrap, cache invalidation (including every lock-ordering/cache-barrier test), and the config-managed same-transaction-recheck guards all run green against a real Postgres (and Redis, for cache tests) instance -- including the single-connection- pool canaries. Two pre-existing test-order-dependent flakes (m34_pki_renewal, m35_pki_revocation -- a shared profile fixture collides across PKI test files sharing one database) and one pre-existing environment-gated test (m41_pki_est, requires an external ATOM_EST_CLIENT) were confirmed to reproduce identically on unmodified code / require setup unavailable here -- not regressions from this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
First phase of the database-backends initiative described in
product-docs/development/database-backends/(PRD/RFC/ROADMAP). Delivered as a single PR per the delivery model documented there, covering DB-001 and DB-002 of that plan:DatabaseKind/Database/DbTransactiontypes insrc/db.rs.AppState.pool: PgPoolbecomesAppState.db: Database, with a transitional.pool()accessor so existing repository/handler call sites keep compiling.DATABASE_URLscheme validation and migration dispatch go through the facade; startup logs a sanitized location instead of the raw URL. Non-postgres:///postgresql://schemes (includingsqlite://) now fail fast at startup with an actionable message.sqlx::Transaction<'_, Postgres>now takes&mut DbTransaction<'_>(or by value for theaudit.rscommit helpers). Nested PKI savepoints route throughDbTransaction::begin().error.rsgained aDatabaseErrorKindclassifier used bydb_err/IntoResponse/tonic::Status.Only Postgres is a working backend after this PR -- no SQLite code lands here. This is purely the internal boundary described in the RFC so a future backend can be added without touching transports or domain services. Zero intended behavior change.
Test plan
cargo fmt --checkcargo clippy -- -D warnings(lib + bins, the AGENTS.md-specified gate)cargo test --lib --bins-- 343 unit tests passorigin/mainline-by-line to confirm the only changes are the facade/transaction-type mechanics, not reverted feature logic