diff --git a/CHANGELOG.md b/CHANGELOG.md index 9deea30b..363cebc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added + - `BE-03`: implement the approved bounded **protected publisher service configuration** as an inactive additive foundation. Adds the canonical optimistic-concurrency token `publisher.service_configuration_updated_at` (`timestamptz DEFAULT CURRENT_TIMESTAMP NOT NULL`, strictly increasing per publisher through `GREATEST(CURRENT_TIMESTAMP, previous + interval '1 microsecond')`), the closed two-value `publisher_service_configuration_source` type (`SUPERUSER_API`, `MIGRATION_BACKFILL`; no `OTHER`, no `UNKNOWN`, no `Default`) and the append-only `publisher_service_configuration_history` audit table (`actor text NOT NULL` with a named non-whitespace check constraint `CHECK (actor ~ '[^[:space:]]')`, enforcing that an audit actor contains at least one non-whitespace character, `ON DELETE CASCADE` foreign key, and a deterministic `(publisher_id, created_at DESC, id DESC)` composite index), with `thoth-api/src/schema.rs` updated manually and atomically in the same PR under `ADR-0003` Architecture A. Introduces **exactly one authoritative production write path** — the service-configuration write coordinator — which owns every committed write of package, enabled-platform desired state, configuration version token and audit history in one transaction on one connection under an explicit caller-supplied source/actor context, taking the publisher row lock first, rejecting a stale `expectedUpdatedAt` before any validation or lifecycle call, pre-validating the whole normalized desired platform set, applying platform state only through `BE-02`'s lifecycle primitives, committing the package and the token together in **exactly one** publisher `UPDATE` written directly (so no `publisher_history` row is written, and the shared `AFTER UPDATE` work-freshness cascade runs once per committed change whatever its shape, while a true no-op or stale request issues zero publisher `UPDATE`s), and writing exactly one audit row per committed change with bounded three-key canonical before/after state. Refactors `BE-02`'s lifecycle additively into connection-scoped `enable_on`/`disable_on` primitives that return a `#[must_use]` `Changed`/`Unchanged` outcome and validate `is_assignable()` themselves before any write, leaving the pool-level `enable`/`disable` signatures, semantics, early non-assignable check and existing tests unchanged and keeping exactly one linked-platform algorithm in the repository — so a membership-equal but split OAPEN/DOAB pair is **repaired**, bumping the token and writing one audit row, while a fully normalized request is a true no-op that writes nothing. Adds the protected `PublisherServiceConfiguration` type exposing the publisher, `subscriptionPackage`, `effectiveCapabilities`, `enabledDistributionPlatforms` and the version token; effective capabilities are **derived on read** from the canonical package through `BE-01`'s code-owned `ThothPackage::capabilities()` in that slice's order and persisted nowhere, so no package/capability inconsistency is representable and a package change updates them automatically. Adds the owner-and-superuser `publisherServiceConfiguration` query under a least-privilege per-publisher check (superuser, or exactly `PUBLISHER_USER` for that publisher's organisation, with **no role inheritance**: `PUBLISHER_ADMIN`, `WORK_LIFECYCLE` and `CDN_WRITE` without `PUBLISHER_USER` are denied, and a `NULL` `zitadel_id` fails closed for every non-superuser), the superuser-only `publisherServiceConfigurations` staff report and `publisherServiceConfigurationCount` with `[Uuid!] = []`, `[ThothPackage!] = []` and `[DistributionPlatform!] = []` filters, `AND` semantics for `enabledPlatforms`, a mandatory `publisher_id ASC` tie-breaker and a bounded two-statement page (one publisher page plus one `DISTINCT ON` latest-change lookup, with assignments resolved through `BE-02`'s existing request-local `ADR-0007` loader and **no second loader**), and the superuser-only `replacePublisherServiceConfiguration` mutation. Adds exactly one new error variant, `ThothError::StalePublisherServiceConfiguration`, and exactly one new `into_field_error` arm, `STALE_SERVICE_CONFIGURATION`, disclosing no SQL, column name or current token; every other error family keeps its merged mapping. `ThothPackage` and `PublisherCapability` become SDL-reachable for the first time, exclusively through the protected type: neither is on the public `Publisher`, and no package or capability value is anonymously readable. **Accepted and measured consequence:** because the token is a `publisher` column, every committed change — including a platform-only change and a linked-state repair — also moves `publisher.updated_at` and refreshes `work.updated_at_with_relations` for every work of that publisher through the existing `AFTER UPDATE` trigger's single set-based statement (never a per-work application loop), invalidating that publisher's cached export records; a stale request, a true no-op and a rolled-back transaction move none of the three values, and another publisher's works are never touched. **Inactive foundation only: the migration creates zero audit rows and changes no package or assignment; no distribution job, job target, job attempt or fabricated job status exists; no dissemination, upload or feed is produced; no capability state is persisted; no production migration, deployment, environment migration, backfill or distribution activation is performed; mutation-guard mode is unchanged; and no `BE-04`, `MIG-01`, `APP-01`, `APP-02` or PR [799](https://github.com/thoth-pub/thoth/pull/799) work is included.** - `BE-03-SPEC`: propose the complete bounded implementation specification for `BE-03` - **protected service configuration** (`docs/engineering/ai-delivery/tasks/BE-03.md`). Settles the full contract for the protected `PublisherServiceConfiguration` type — which, per `ADR-0001` section 4.4, exposes the current package, the **effective capability codes** and the enabled distribution platforms — the owner-and-superuser `publisherServiceConfiguration` query with its complete deny/allow matrix resolved through the existing ZITADEL publisher-role model (no new ownership table, no new role), the superuser-only paginated and deterministically ordered `publisherServiceConfigurations` staff report with publisher/package/enabled-platform filters declared in the merged repository shape (`[Uuid!] = []`, `[ThothPackage!] = []`, `[DistributionPlatform!] = []`), and the superuser-only `replacePublisherServiceConfiguration` mutation executing in exactly one transaction with a documented deterministic lock order matching `BE-02`. Effective capabilities are **derived on read** from the canonical `subscription_package` through `BE-01`'s code-owned `ThothPackage::capabilities()`, in that mapping's canonical order, and are **persisted nowhere** — no capability column, table, override or cache, no second mapping, and no capability input — so no package/capability inconsistency is representable and a package change updates them automatically; they are protected by the same single read decision as the rest of the type and must not appear on the public `Publisher`. States the protected read as a **least-privilege per-publisher check** — superuser, or exactly `PUBLISHER_USER` for the target publisher's organisation — with **no role inheritance**, so `PUBLISHER_ADMIN`, `WORK_LIFECYCLE` or `CDN_WRITE` without `PUBLISHER_USER` are denied and commercial package visibility is not widened by an unrelated scoped capability. Fixes one authoritative internal **service-configuration write coordinator** owning every committed write of package, enabled-platform desired state, canonical configuration version and configuration audit history in one caller-owned transaction under an explicit source/actor context, with `BE-02`'s pool-level `enable`/`disable` retained unchanged but reclassified as lower-level domain/compatibility functions barred from new production configuration call sites, and required call-site and bypass-search evidence. Selects a **dedicated `publisher.service_configuration_updated_at` concurrency token** with a strict per-publisher monotonicity rule, recording why `publisher.updated_at` and a computed multi-timestamp maximum were rejected, and documenting the accepted consequences that follow from the token being a `publisher` column: `public.publisher` carries **two** existing `UPDATE` triggers, so every committed configuration change — including a platform-only change or a linked-state repair — moves the public `Publisher.updatedAt` through `set_updated_at` **and** refreshes `work.updatedAtWithRelations` for every work of that publisher through its imprints through the existing `AFTER UPDATE` `set_work_updated_at_with_relations`, while a stale request, a true no-op and a rolled-back transaction move none of the three values and no other publisher's works are ever touched. The work-freshness value is a **public downstream signal** that invalidates that publisher's cached export records, so a platform-only change and a linked-state repair newly cause a cascade merged `BE-02` does not cause; the committed transaction therefore writes one publisher row plus bounded configuration/audit rows plus N related work rows through **one set-based trigger statement, never a per-work application loop**, with row-lock footprint and transaction duration growing with catalogue size while the publisher lock is held, and `BE-04` inherits this with the same transaction boundary. The token stays on `publisher` because a separate version table — whose genuine benefit for platform-only persistence is recorded honestly — would not avoid the same cascade for canonical package changes while `BE-01`'s `subscription_package` remains on `publisher`, and eliminating it fully would mean wider changes to approved `BE-01` storage or shared publisher trigger semantics; acceptance is therefore **evidence-conditional**, requiring six-case evidence against a real disposable PostgreSQL database with at least two imprints, at least two target works and a control work of another publisher, plus a catalogue-scale write-amplification and lock-footprint measurement recorded without production extrapolation, and a new stop condition requiring `BLOCKED` and a separate architecture escalation — never a silent trigger or storage change — if that evidence is bad. This freshness cascade is **not** distribution activation: no distribution job, upload, feed or dissemination is created. `Publisher.updatedAt` is explicitly **not** the configuration concurrency token, the protected values themselves stay behind the single read decision (timing correlation is acknowledged rather than denied), and the token's addition to the `Publisher` struct may surface as an additional key in future `publisher_history.data` snapshots; specifies the additive `publisher_service_configuration_history` audit table with an explicit `actor text NOT NULL` column (non-blank check constraint; following the repository's established account-FK-free actor representation but deliberately not named `user_id`, because `source` fixes the actor's provenance) and a closed two-value `source` type whose `SUPERUSER_API`/`MIGRATION_BACKFILL` actor contract is defined now, binding a future `MIG-01` to the same coordinator, the same token update and the same audit invariants while creating no job; plus bounded canonical before/after state that exposes no credential, endpoint, activation identifier or unrelated publisher metadata; requires reuse of `BE-02`'s linked OAPEN/DOAB normalization and non-assignable `JISC_NBK` behaviour through an additive connection-scoped refactor rather than a second implementation, with the primitives reporting whether they changed persisted state so that a **membership-equal but split OAPEN/DOAB pair is repaired rather than skipped** — a repair being a committed change that bumps the token and writes exactly one audit row — and with the connection-scoped enable primitive required to check `is_assignable()` **itself** before any write, as defence in depth alongside the retained pool-level early check and the coordinator's whole-set pre-validation; and requires reuse of `BE-02`'s existing request-local `ADR-0007` assignment DataLoader rather than a second equivalent loader. Raises one **programme-decision candidate** for CTO approval, under a durable `ADR-0005` authority condition rather than a mutable status word — it becomes an approved programme decision when the exact specification content carrying it receives explicit CTO specification approval **and** that content is reachable from `develop`, with no separate lifecycle-status commit required: `BE-03` owns desired configuration only and creates no `distribution_job`, `distribution_job_target` or `distribution_job_attempt` row, no placeholder job and no fabricated job status, with `BE-04` later extending the same transaction boundary; the decision additionally reconciles `APP-01`, whose configuration scope BE-03 satisfies but whose durable job, attempt, failure and pending-onboarding elements require `BE-04`. **Specification and control records only - no runtime, Cargo, GraphQL, schema, migration, workflow or infrastructure change, no implementation, and no implementation branch created.** `BE-03` implementation remains **NOT AUTHORIZED** and `feature/publisher-services/be-03` must not exist; `BE-04`, `MIG-01`, `APP-01`, `APP-02`, deployment, environment and production migration, assignment backfill, distribution activation, `OBSERVE`/`ENFORCE` and PR [799](https://github.com/thoth-pub/thoth/pull/799) all remain outside scope and unauthorized - `BE-02-CLOSEOUT-01`: correct the materially stale active Publisher Services programme and dependency state left after the `BE-02` implementation merged. Active controls now record `BE-02` as `CLOSED - INACTIVE FOUNDATION` with its repository implementation merged, record the `BE-03` dependency on `BE-01` and `BE-02` as satisfied, and stop asserting that `BE-02` is unmerged, awaiting independent review or merge authorization, blocked or unauthorized, or that no `DistributionPlatform` enum is implemented. Adds the bounded `BE-02-CLOSEOUT-01` task record and implementation report. **Documentation and control records only: no runtime, schema, migration, API, generated contract, client artifact or workflow change.** Under `ADR-0005` this corrects materially incorrect programme state only; no review, approval or merge-authorization identifier, merge commit SHA or merge timestamp is transcribed into repository files, GitHub remains the terminal lifecycle authority, and historical implementation-time evidence is preserved as written. Deployment, environment and production migration execution, assignment creation/backfill, distribution activation, `OBSERVE`/`ENFORCE` and production access all remain **NOT AUTHORIZED**, `BE-03` implementation remains **NOT AUTHORIZED**, and PR [799](https://github.com/thoth-pub/thoth/pull/799) and issue [765](https://github.com/thoth-pub/thoth/issues/765) are untouched - [805](https://github.com/thoth-pub/thoth/pull/805) - `BE-02`: implement the approved bounded **distribution platform model** as an inactive additive foundation. Adds the closed 17-value `DistributionPlatform` inventory from `ADR-0004` (no `OTHER`, no fallback, no `Default`, no shared enum or conversion with Thoth Metrics), code-owned compile-time-exhaustive platform descriptors, the PostgreSQL `distribution_platform` enum and the additive `publisher_distribution_platform` relation (composite primary key `(publisher_id, platform)`, `ON DELETE CASCADE` foreign key to `publisher`, named `enabled == (disabled_at IS NULL)` check constraint, partial enabled index and `set_updated_at` trigger), with `thoth-api/src/schema.rs` updated manually and atomically in the same PR under `ADR-0003` Architecture A. Implements the six-transition assignment activation lifecycle (retained disabled rows, application-generated activation UUIDs, one transaction timestamp per transition, same-state no-ops that move no timestamp) and atomic OAPEN/DOAB linked normalization that repairs one-sided, split-activation and split-timestamp pairs rather than treating them as idempotent; `OCLC_KB` and `EX_LIBRIS_KB` stay independently assignable, and `JISC_NBK` is included but inactive and non-assignable, failing closed before any write through the new stable `ThothError::DistributionPlatformNotAssignable` variant. Adds exactly four public additive GraphQL read surfaces — `distributionPlatformOptions`, `publishersByDistributionPlatform`, `publisherCountByDistributionPlatform` and `Publisher.distributionPlatforms` — with two new object types and three new enums, and **no** new mutation, input, scalar or interface; activation IDs, disabled history, adapter/feed identity, package/capability state and protected configuration are not exposed. `Publisher.distributionPlatforms` is the **first production consumer of the `ADR-0007` request-local non-cached DataLoader foundation**: a typed loader added to the existing `RequestLoaders` bundle, keyed on `publisher_id`, built through `configured_loader` with explicit `200`/`10`, loader-first at resolver entry, `try_load` only, total fail-closed batches, and one set-based `eq_any` statement per dispatch chunk executed entirely inside `tokio::task::spawn_blocking`. **Inactive foundation only: no distribution is activated, no distribution job or dissemination exists, the migration creates zero assignment rows, no production migration, deployment, backfill or assignment is performed, mutation-guard mode is unchanged, and no `BE-03`, `BE-04`, `MIG-01`, OAI, Metrics or PR [799](https://github.com/thoth-pub/thoth/pull/799) work is included.** diff --git a/docs/engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md b/docs/engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md new file mode 100644 index 00000000..de7070f1 --- /dev/null +++ b/docs/engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md @@ -0,0 +1,1528 @@ +# BE-03 Implementation Report + +## 1. Repository state + +Repository: `thoth-pub/thoth` +Workflow: STANDARD +Base branch: `develop` +Base commit: `3b6b3a31f9358011f0c998015dfd0c2508380e83` (PR +[#808](https://github.com/thoth-pub/thoth/pull/808) merge commit; verified live +before any edit, `0` commits added since authorization) +PR target: `develop` +Programme integration branch: None +Task branch: `feature/publisher-services/be-03` (created by the control agent at +exactly the authorized base; not recreated by this agent) +Head commit: recorded on the pull request; see section 3 for the commit series +Pull request: [#809](https://github.com/thoth-pub/thoth/pull/809) - OPEN, DRAFT, +UNMERGED +Expected branch deletion after merge: YES +Final programme PR required: NO +Implementing model: Claude Opus 5 (`claude-opus-5`), implementation agent +Reasoning level: Extra High / xhigh +Independent reviewer/model: NOT PERFORMED BY THE IMPLEMENTATION AGENT - a +different agent/model must review the exact head. The previous exact head +`1315057983d389d1ef6b85bc4e69b81eda53aa79` received an independent HIGH-risk +implementation review whose decision was **BLOCKED** (section 16.1). That review +applies **only** to that head. The remediation head produced by this cycle has +**not** been independently reviewed and requires a fresh one. + +### 1.1 Preflight record + +| Item | Observed | +|---|---| +| `origin/develop` | `3b6b3a31f9358011f0c998015dfd0c2508380e83` | +| `feature/publisher-services/be-03` at start | `3b6b3a31f9358011f0c998015dfd0c2508380e83` | +| Approved specification head `a3fc7064...` | ancestor of the base (`git merge-base --is-ancestor` succeeded) | +| Working tree at start | clean | +| Ahead / behind `develop` at start | `+0 / -0` | +| Commits added to `develop` since authorization | `0` (develop head equals the authorized base) | +| Competing BE-03 implementation PR | none (`gh pr list --state open` returned #806, #799, #752, #744, #742, #668 only) | +| PR #799 | OPEN, DRAFT, untouched | +| `thoth-api/migrations/20260813_v1.7.0` before `make migration` | did not exist | + +No rebase, force-push or history rewrite was performed. `develop` did not move +during implementation, so no drift assessment was required. + +## 2. Scope confirmation + +Approved specification: +[`docs/engineering/ai-delivery/tasks/BE-03.md`](../tasks/BE-03.md), merged +through PR [#808](https://github.com/thoth-pub/thoth/pull/808) as `3b6b3a31`. +Both halves of the section 19 stop-condition 5 authority condition therefore +hold. + +Implemented objective: the complete approved BE-03 specification as an inactive +additive foundation — the canonical configuration token, the closed audit-source +type and append-only audit table, the single authoritative service-configuration +write coordinator, the additive connection-scoped BE-02 lifecycle refactor, the +protected `PublisherServiceConfiguration` type with derived effective +capabilities, the owner-and-superuser read, the superuser-only staff report and +count, the superuser-only replace mutation, one new error variant and one new +`into_field_error` arm, and the section 18 test and evidence matrix. + +Out-of-scope changes made: NONE. + +Explicitly not done, and not authorized: merge; deployment; environment or +production migration execution; production access; MIG-01; assignment creation +or backfill; distribution activation; dissemination; BE-04; APP-01; APP-02; +`thoth-app` changes; shared publisher-trigger changes; `subscription_package` +storage relocation; `OBSERVE`/`ENFORCE` or mutation-guard changes; workflow +changes or `workflow_dispatch`; any action on PR +[#799](https://github.com/thoth-pub/thoth/pull/799). + +## 3. Commits + +- `3facc3f9` - `feat(publisher-services): implement BE-03 protected service configuration` +- `80eeb60c` - `docs(publisher-services): record BE-03 CI evidence` +- `13150579` - `docs(publisher-services): clarify BE-03 CI evidence scope` + — the exact head that received the independent review recorded in section 16.1 +- `[remediation sha]` - `fix(publisher-services): remediate BE-03 independent review findings` + +The exact head SHA is recorded on the pull request under ADR-0005. Remediation +was appended as an ordinary commit on top of `13150579`: no rebase, no amend, no +squash, no force-push, no new branch and no new pull request. + +## 4. Files changed + +- `thoth-api/migrations/20260813_v1.7.0/up.sql` (new) + - reason: the additive BE-03 database foundation. + - behavioural effect: adds `publisher.service_configuration_updated_at`, the + `publisher_service_configuration_source` enum type and the + `publisher_service_configuration_history` table with its primary key, + `ON DELETE CASCADE` foreign key, named non-whitespace actor check constraint + `CHECK (actor ~ '[^[:space:]]')` and composite index. Creates zero rows. +- `thoth-api/migrations/20260813_v1.7.0/down.sql` (new) + - reason: reversibility evidence. + - behavioural effect: drops the table, the type and the column. +- `thoth-api/src/schema.rs` + - reason: ADR-0003 Architecture A — the repository-authoritative Diesel + contract is edited manually and atomically with the migration. + - behavioural effect: adds `sql_types::PublisherServiceConfigurationSource`, + appends `service_configuration_updated_at` to the `publisher` table after + `updated_at` so `Queryable` column order still matches the `Publisher` + struct, adds the `publisher_service_configuration_history` table, its + `joinable!` and its `allow_tables_to_appear_in_same_query!` entry. No + unrelated schema reformatting. +- `thoth-api/src/model/publisher/mod.rs` + - reason: the token is a `Publisher` column. + - behavioural effect: appends `#[serde(default)] pub + service_configuration_updated_at: Timestamp` after `updated_at`. No GraphQL + field is added to the public `Publisher` type. +- `thoth-api/src/model/publisher_service_configuration/mod.rs` (new) + - reason: the BE-03 domain module. + - behavioural effect: the `PublisherServiceConfigurationSource` DB/GraphQL + enum, the `PublisherServiceConfiguration`, + `PublisherServiceConfigurationChange` and + `PublisherServiceConfigurationSummary` projections, the audit row + `Queryable`/`Insertable` pair, the bounded three-key + `CanonicalServiceConfigurationState`, the + `ServiceConfigurationWriteContext` and the + `ReplacePublisherServiceConfigurationInput` GraphQL input. +- `thoth-api/src/model/publisher_service_configuration/crud.rs` (new) + - reason: the canonical write coordinator and the staff-report queries. + - behavioural effect: section 5 below. +- `thoth-api/src/model/publisher_service_configuration/tests.rs` (new) + - reason: coordinator, audit, concurrency, linked-platform, trigger-cascade, + report and migrated-contract evidence (45 tests). +- `thoth-api/src/model/publisher_distribution_platform/crud.rs` + - reason: the additive connection-scoped lifecycle refactor of section 7.7. + - behavioural effect: adds `AssignmentLifecycleOutcome`, extracts + `enable_on`/`disable_on`, reduces the pool-level `enable`/`disable` to + acquire-connection/open-transaction/delegate/discard, and widens + `lock_publisher` to `pub(crate)`. Public signatures, semantics, ordering and + error behaviour are unchanged. +- `thoth-api/src/model/publisher_distribution_platform/tests.rs` + - reason: BE-02 regression evidence for the new primitives. + - behavioural effect: **adds** two tests; no existing test's expectation is + changed. +- `thoth-api/src/model/mod.rs` + - reason: register the new model module. +- `thoth-api/src/graphql/model.rs` + - reason: the protected GraphQL types. + - behavioural effect: adds three `graphql_object` implementations. The public + `Publisher` type is unchanged. +- `thoth-api/src/graphql/query.rs` + - reason: the protected read and the superuser report/count. +- `thoth-api/src/graphql/mutation.rs` + - reason: the superuser-only replace mutation and its authorize/build-context/ + delegate helper. +- `thoth-api/src/graphql/mod.rs` + - reason: register the BE-03 GraphQL test module. +- `thoth-api/src/graphql/service_configuration_tests.rs` (new) + - reason: authorization-matrix, capability, error-shape, query-count, + loader-reuse and catalogue-scale evidence (23 tests). +- `thoth-api/src/graphql/sdl_support.rs` (new, test-only) + - reason: the remediation of the P1 SDL guard coverage defect (section 16.3). + - behavioural effect: none in production. Gated + `#[cfg(all(test, feature = "backend"))]`, it provides the single + brace-balanced, string-aware `sdl_block` extractor both guards now use, plus + six tests of the extractor itself including the post-`imprints` tampered-field + regression. +- `thoth-api/src/graphql/tests.rs` + - reason: **authorized SDL guard amendment** (section 12.3), plus the section + 16.3 coverage repair — it now uses the shared extractor and asserts the + post-`imprints` sentinels. +- `thoth-api/src/graphql/distribution_platform_tests.rs` + - reason: **authorized SDL guard amendment** (section 12.3), plus the section + 16.3 coverage repair. Every `.split_once('}')` extraction in the file is + replaced by the shared extractor; BE-02's four verbatim read-surface + assertions are unchanged. +- `thoth-errors/src/lib.rs` + - reason: exactly one new variant and exactly one new `into_field_error` arm. +- `CHANGELOG.md`, `docs/publisher-services/task-status.md`, + `docs/engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md` + - reason: required control records. + +## 5. Implementation decisions + +Decisions taken within the approved design: + +1. **Coordinator identity.** Name: + `replace_publisher_service_configuration`. Module: + `thoth-api/src/model/publisher_service_configuration/crud.rs`. Signature: + + ```rust + pub(crate) fn replace_publisher_service_configuration( + db: &PgPool, + write_context: &ServiceConfigurationWriteContext<'_>, + data: &ReplacePublisherServiceConfigurationInput, + ) -> ThothResult + ``` + + It acquires one connection, opens exactly one transaction and executes + specification section 7.3 steps 2–12 in a **private** module-local function + `replace_in_transaction`. That helper is deliberately not `pub(crate)`: it is + the coordinator's transaction body — the seam BE-04 will extend — and not a + second entry point. The GraphQL mutation is its only production caller. +2. **Write context.** `ServiceConfigurationWriteContext { source, actor }` is a + parameter. The resolver authorizes with `require_superuser()`, then builds + `source = SUPERUSER_API` and `actor = PolicyContext::user_id()?`. The + coordinator makes no authentication or authorization decision. +3. **`after_state` is read back from the database** after the lifecycle calls + rather than assumed from the request. It equals the normalized desired set, + and recording observed state is strictly more faithful for an audit row. +4. **Returned configuration.** The step 10 `UPDATE` uses `RETURNING + publisher::all_columns`, so the returned configuration carries the actual + committed row — including the trigger-updated `publisher.updated_at` — with no + extra statement. A true no-op returns the row read under the lock. +5. **Exactly one publisher `UPDATE` per committed change.** Under the corrected + section 7.3 (see section 16.2 item 3), step 8 only compares the requested + package with the locked current package and records `package_changed`; the + write is deferred to step 10, which issues a single `UPDATE` carrying the + package when it changed and always carrying the token. The two Rust branches + differ only in whether the package travels with the token, and exactly one + executes. `publisher::table` is written inline in both rather than through a + hoisted local, so both remain visible to the section 9.13 containment search. + This matters because the publisher row carries the shared `AFTER UPDATE` + work-freshness trigger: a second `UPDATE` in the same transaction would re-run + that trigger's set-based cascade over the same N work rows for no additional + effect. A combined package-and-platform change therefore now costs the same + single cascade as a platform-only change or a linked repair, and a true no-op + or stale request issues zero publisher `UPDATE`s. +6. **Accessor placement.** `subscriptionPackage`, `effectiveCapabilities` and + `updatedAt` are defined **only** as GraphQL resolvers reading the one + `publisher` row held by `PublisherServiceConfiguration`. They are deliberately + not duplicated as model-layer inherent methods: one definition means a + response can never report a package and a capability set that disagree. +7. **Report filters** are shared by the list and count queries through one + `filtered_publishers` function, so they cannot diverge. `enabledPlatforms` + uses a single grouped subquery with `HAVING count(*) = n` over the + deduplicated requested set, which is exact `AND` semantics because + `(publisher_id, platform)` is the assignment primary key. +8. **Error convention.** BE-03's resolvers map through + `IntoFieldError::into_field_error`, which is what makes the section 13 table + true (`NO_ACCESS`, `STALE_SERVICE_CONFIGURATION`, and `INTERNAL_ERROR` for + `EntityNotFound`/`DistributionPlatformNotAssignable`/`DatabaseError`). No + existing field's error shape is changed. +9. **`clippy::misnamed_getters`** is allowed with an explanatory comment on + `PublisherServiceConfiguration::updated_at`, because the field must resolve to + the configuration token and **not** to `publisher.updated_at` — the two are + deliberately different values (specification section 6.4 item 3). + +Deviations from the specification: NONE. + +Two specification statements could not be satisfied as literally written at the +previous head. **Both are now resolved by the CTO-authorized bounded +specification corrections of section 16**, and the specification text in this +pull request has been amended accordingly: + +- **Section 14.3 item 3 previously said "commit the regenerated + `thoth-client/assets/schema.graphql`".** That file is **generated and + gitignored** in this repository (`thoth-client/.gitignore:1:assets/schema.graphql`), + written by `thoth-client/build.rs` on every build, so the instruction was + unsatisfiable and merged repository evidence outranked it (root `AGENTS.md` + section 2 authority order). The amended §14.3 item 3 now requires regeneration + through the normal build path, the exact SDL diff against the authorized base, + a reproducible artifact identity, workspace-path client verification and the + backend head for APP-01 pinning — which is what sections 7.2, 7.4, 7.5 and 9.4 + record. Nothing is unsatisfiable and nothing is deviated from. +- **Section 18.4 previously required rejecting "a blank or whitespace-only + actor" while section 8.1 prescribed `CHECK (btrim(actor) <> '')`**, which + trims spaces only and therefore accepts a tab-only or newline-only actor. That + contradiction was the P1 authority conflict. The CTO resolved it in favour of + the invariant — an audit actor must contain at least one non-whitespace + character — and both sections now express it, with the predicate + `CHECK (actor ~ '[^[:space:]]')`. Evidence in section 9.1. + +**The amended specification text is a candidate in this pull request. It is not +yet reachable from `develop`**, which remains at +`3b6b3a31f9358011f0c998015dfd0c2508380e83`, and nothing here should be read as +claiming it is already merged. + +## 6. Database and migration effects + +Migration added: YES + +- migration files: `thoth-api/migrations/20260813_v1.7.0/up.sql` and + `down.sql`. + + **DATE override authorization.** `20260812` is already occupied by merged + BE-02 (`thoth-api/migrations/20260812_v1.7.0`), and `make migration` derives + the directory name from `date +"%Y%m%d"`, which on the implementation day would + have collided with it. The CTO explicitly authorized the repository-supported + command with an overridden date: + + ```bash + make migration DATE=20260813 + ``` + + It produced exactly `thoth-api/migrations/20260813_v1.7.0/up.sql` and + `thoth-api/migrations/20260813_v1.7.0/down.sql` and no other directory. The + version suffix `v1.7.0` is derived by the Makefile from the workspace version + `1.6.2`, unchanged. No existing migration was appended to, renamed or + rewritten, and no migration was hand-created. +- schema effect: one new column on `publisher`, one new enum type with exactly + two labels in order, one new table with one primary key, one `ON DELETE + CASCADE` foreign key, one named check constraint and one composite index. +- existing-data effect: every existing publisher receives + `service_configuration_updated_at` from the column default. No package value + changes, no assignment row changes, zero audit rows are created and zero job + tables exist. +- locking/downtime: `ADD COLUMN` takes a brief `ACCESS EXCLUSIVE` lock on + `publisher`; the new table's foreign key additionally takes + `SHARE ROW EXCLUSIVE`. Measured, not asserted — see section 9.1. +- empty database result: apply, revert, re-apply all clean — section 9.1. +- populated database result: `relfilenode` unchanged, packages unchanged, + BE-02 assignments byte-identical, zero audit rows — section 9.1. +- rollback/forward repair: section 12. +- idempotency: not applicable; a one-shot DDL migration. + +`thoth-api/src/schema.rs` was edited manually and atomically in this same pull +request under ADR-0003 Architecture A. `diesel print-schema` was not used, no +`diesel.toml` was introduced and no schema-synchronization subsystem was added. + +Not created, by design: `distribution_job`, `distribution_job_target`, +`distribution_job_attempt`, any worker-role persistence, any credential or +configuration-secret table, and **any capability column, table, override or +cache**. Verified by catalog assertion — section 9.1. + +## 7. API and compatibility effects + +### 7.1 Additive inventory + +Queries added: `publisherServiceConfiguration`, +`publisherServiceConfigurations`, `publisherServiceConfigurationCount`. +Mutations added: `replacePublisherServiceConfiguration`. +Object types added: `PublisherServiceConfiguration`, +`PublisherServiceConfigurationChange`, +`PublisherServiceConfigurationSummary`. +Input types added: `ReplacePublisherServiceConfigurationInput`. +Enums added: `PublisherServiceConfigurationSource`. +Enums becoming SDL-reachable: `ThothPackage`, `PublisherCapability`. + +Nothing else. No scalar, interface or union; no field on an existing type; no +change to any existing field's type, nullability, arguments, defaults or +description. + +### 7.2 Exact SDL diff + +Generation command (from the repository root, which runs +`thoth-client/build.rs`): + +```bash +cargo check --workspace +``` + +The base artifact was regenerated identically from a `git worktree` at +`3b6b3a31f9358011f0c998015dfd0c2508380e83` with a separate `CARGO_TARGET_DIR`, +and the two files compared. + +```text +added lines: 72 +removed lines: 0 +``` + +**Re-run after the complete remediation**, as required, even though no GraphQL +production shape was expected to change. Base artifact SHA-256 +`0ba96aa1aa15006e8bf8b9f4a711f9e493eec4ce51911eebc32fb99d1ba53a67`; head artifact +SHA-256 `25329c1687d8b4222638c2f673bd2751a13adeda8c6f181d4ac83e869abac479`. The +diff is unchanged at 72 added / 0 removed, and the head artifact is +**byte-identical to the pre-remediation head** — independent confirmation that +the SDL guard repair, the single-`UPDATE` correction and the documentation +corrections changed no production GraphQL contract. + +The complete diff, in file order: + +```diff ++"Capability that a subscription package may grant to a publisher. A capability permits a feature but does not configure or activate it" ++enum PublisherCapability { ++ "Publisher works may be considered for OAI-PMH after work-level open-licence and lifecycle checks" OAI_PMH ++ "Thoth-managed drivers may collect and retain canonical metrics when a source account and platform/measure configuration are enabled" METRICS_COLLECT ++ "Publisher users may submit approved publisher-controlled usage or sales reports" METRICS_IMPORT ++ "A Thoth-owned authenticated service may serve publisher dashboard metrics" METRICS_DASHBOARD ++ "A Thoth-owned authenticated service may serve bounded work-level widget metrics" METRICS_WIDGET ++ "Eligible finalized canonical metrics may create and deliver OPERAS export claims" METRICS_OPERAS_EXPORT ++} ++ ++"How a recorded service-configuration change entered the system" ++enum PublisherServiceConfigurationSource { ++ "A committed superuser replacePublisherServiceConfiguration call; the actor is the authenticated account identifier" SUPERUSER_API ++ "A separately approved controlled historical backfill; the actor is the authorized control identity" MIGRATION_BACKFILL ++} ++ ++"Subscription package determining which publisher services a publisher is entitled to" ++enum ThothPackage { ++ "Default package, with no publisher-service capabilities" OASIS ++ "Package permitting OAI-PMH eligibility and private managed metrics collection" OBELISK ++ "Package permitting OAI-PMH eligibility and all initial metrics capabilities" SPHINX ++ "Package permitting OAI-PMH eligibility and all initial metrics capabilities" PYRAMID ++} ++ ++"Complete desired service configuration to store for a publisher. This is a replace, not a patch: the platform list is the complete desired enabled set, and an empty list means no destination is enabled" ++input ReplacePublisherServiceConfigurationInput { ++ publisherId: Uuid! ++ subscriptionPackage: ThothPackage! ++ enabledDistributionPlatforms: [DistributionPlatform!]! ++ expectedUpdatedAt: Timestamp! ++} ++ ++ "Replace a publisher's complete desired service configuration under optimistic concurrency control. Superuser only. This stores desired configuration: it creates no distribution job and triggers no dissemination" ++ replacePublisherServiceConfiguration("Complete desired service configuration to store" data: ReplacePublisherServiceConfigurationInput!): PublisherServiceConfiguration! ++ ++"The desired service configuration of one publisher." ++type PublisherServiceConfiguration { ++ "The publisher this configuration belongs to" ++ publisher: Publisher! ++ "Subscription package currently assigned to the publisher" ++ subscriptionPackage: ThothPackage! ++ "Capabilities the current subscription package grants this publisher, in canonical capability order. Derived from the package; a capability permits a feature but does not configure or activate it" ++ effectiveCapabilities: [PublisherCapability!]! ++ "Distribution platforms currently enabled for the publisher, in canonical platform order" ++ enabledDistributionPlatforms: [PublisherDistributionPlatformAssignment!]! ++ "Version token of this configuration; supply it as expectedUpdatedAt to replace the configuration" ++ updatedAt: Timestamp! ++} ++ ++"Metadata of one recorded service-configuration change. The before and after states themselves are not exposed." ++type PublisherServiceConfigurationChange { ++ "When the change was committed" ++ changedAt: Timestamp! ++ "Identity that made the change: the account identifier for SUPERUSER_API, or the authorized control identity for a controlled backfill" ++ actor: String! ++ "How the change entered the system" ++ source: PublisherServiceConfigurationSource! ++} ++ ++"A publisher's service configuration together with its latest change metadata." ++type PublisherServiceConfigurationSummary { ++ "The publisher's desired service configuration" ++ configuration: PublisherServiceConfiguration! ++ "Metadata of the most recent recorded configuration change, or null if none has been recorded" ++ lastChange: PublisherServiceConfigurationChange ++} ++ ++ "Query the protected desired service configuration of one publisher. Readable only by a superuser or by a PUBLISHER_USER of that publisher" ++ publisherServiceConfiguration("Thoth publisher ID to search on" publisherId: Uuid!): PublisherServiceConfiguration! ++ "Query the protected desired service configuration of every publisher, with the metadata of its latest recorded change. Superuser only" ++ publisherServiceConfigurations(…): [PublisherServiceConfigurationSummary!]! ++ "Get the total number of publishers matching a protected service configuration report filter. Superuser only" ++ publisherServiceConfigurationCount(…): Int! +``` + +The two report lines are elided above only for width; their generated argument +lists are quoted verbatim below. + +### 7.3 List-argument nullability, quoted verbatim + +From the regenerated `thoth-client/assets/schema.graphql` at the implementation +head: + +```graphql +publisherServiceConfigurations("If set, only shows results for publishers with these IDs" publishers: [Uuid!] = [], "If set, only shows results for publishers with these subscription packages" packages: [ThothPackage!] = [], "If set, only shows results for publishers that have every one of these distribution platforms enabled. Multiple values narrow the results rather than widening them" enabledPlatforms: [DistributionPlatform!] = [], "The number of items to return" limit: Int = 100, "The number of items to skip" offset: Int = 0, "The order in which to sort the results. Results are always additionally sorted by publisher ID ascending, so pagination is deterministic" order: PublisherOrderBy = {direction: "ASC", field: "PUBLISHER_NAME"}): [PublisherServiceConfigurationSummary!]! +publisherServiceConfigurationCount("If set, only counts publishers with these IDs" publishers: [Uuid!] = [], "If set, only counts publishers with these subscription packages" packages: [ThothPackage!] = [], "If set, only counts publishers that have every one of these distribution platforms enabled. Multiple values narrow the results rather than widening them" enabledPlatforms: [DistributionPlatform!] = []): Int! +``` + +The three required fragments, verbatim: + +```graphql +publishers: [Uuid!] = [] +packages: [ThothPackage!] = [] +enabledPlatforms: [DistributionPlatform!] = [] +``` + +Compared explicitly against the merged siblings named by specification section +14.3 item 2, from the same artifact: + +| Merged sibling | Generated argument | +|---|---| +| `imprints` | `"If set, only shows results connected to publishers with these IDs" publishers: [Uuid!] = []` | +| `publishersByDistributionPlatform` | `platform: DistributionPlatform!, limit: Int = 100, offset: Int = 0, order: PublisherOrderBy = {direction: "ASC", field: "PUBLISHER_NAME"}` | +| `works` (merged enum-list precedent) | `"Specific types to filter by" workTypes: [WorkType!] = []` | + +All three BE-03 filters render as **nullable outer lists of non-null members +with a `[]` default**, matching the merged convention exactly. None renders as +`[T!]!`. + +### 7.4 Generated schema/client updates + +- `thoth-client/assets/schema.graphql` is **build-generated and gitignored** in + this repository, so it is regenerated rather than committed, exactly as the + amended specification §14.3 item 3 requires; see sections 5 and 16.2. Its + SHA-256 at the remediation head is + `25329c1687d8b4222638c2f673bd2751a13adeda8c6f181d4ac83e869abac479`. + `thoth-client/.gitignore` was not modified and the artifact was not + force-added: `git check-ignore -v thoth-client/assets/schema.graphql` reports + `thoth-client/.gitignore:1:assets/schema.graphql`, and + `git status --porcelain thoth-client/` is empty. +- `thoth-client/assets/queries.graphql`: **unchanged**, as a reviewed + conclusion, not an omission. BE-03 adds protected operations the internal + export client does not consume, and it changes no field the client already + selects. `git status --porcelain thoth-client/assets/queries.graphql` is + empty. +- No generated client enum conversion needed updating: the two newly reachable + enums appear only on protected fields the client does not select. + +### 7.5 APP-01 contract pinning + +The pinned contract is the exact generated schema produced at the reviewed +BE-03 head. Record, at pinning time, **both**: + +- the exact backend commit SHA of the reviewed BE-03 head (recorded on the pull + request; see section 3); +- the schema artifact regenerated at that same head, whose SHA-256 at the head + this report was written against is + `25329c1687d8b4222638c2f673bd2751a13adeda8c6f181d4ac83e869abac479`. + +It includes the corrected list-argument nullability of section 7.3 and the +`PublisherCapability` block of section 7.2. `thoth-app` is a **separate +repository** and was not modified; its codegen impact is additive-only (three +new queries, one new mutation, three new object types, one new input, three +newly reachable enums, no removal and no changed field), so an existing +`thoth-app` build against this contract continues to compile. + +### 7.6 Backwards compatibility + +Every existing public GraphQL surface is unchanged, including BE-02's four read +surfaces, their nullability, ordering and authorization — asserted verbatim by +the amended guard in `distribution_platform_tests.rs`. Deprecations: none. + +## 8. Authorization and security + +Authorization paths changed: three new protected operations. No existing +operation's authorization changed. No new role, ownership table, policy helper +or authorization framework was introduced. + +| Caller | Read protected configuration | Replace configuration | Report / count | +|---|---|---|---| +| `SUPERUSER` | ALLOW, any publisher | ALLOW, any publisher | ALLOW | +| `PUBLISHER_USER` for the target publisher | ALLOW | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| `PUBLISHER_USER` only for another publisher | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| `PUBLISHER_ADMIN` for the target, no `PUBLISHER_USER` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| `WORK_LIFECYCLE` for the target, no `PUBLISHER_USER` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| `CDN_WRITE` for the target, no `PUBLISHER_USER` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| all three of the above combined, no `PUBLISHER_USER` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| authenticated, no applicable role | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| anonymous | DENY `NO_ACCESS` | DENY `NO_ACCESS` | DENY `NO_ACCESS` | +| account with `PUBLISHER_USER` for two publishers | ALLOW for both, DENY for a third | — | — | +| target publisher with `NULL` `zitadel_id` | DENY for every non-superuser; ALLOW for superuser | — | — | +| unknown publisher, authenticated caller | `EntityNotFound` (`INTERNAL_ERROR`) | — | — | +| unknown publisher, anonymous caller | DENY `NO_ACCESS` before any load | — | — | + +Every row is asserted by a test in +`thoth-api/src/graphql/service_configuration_tests.rs`, each requesting +`effectiveCapabilities`, so capability exposure is proven to follow the same +single decision as the rest of the type. Adding `PUBLISHER_USER` to an otherwise +denied account opens the read, which proves the denial is the role check and not +an unrelated failure. Every mutation denial was taken before the database was +touched: the token is unchanged after the whole denied-caller sweep. + +Secret or personal-data handling: the audit `actor` is exactly +`IntrospectedUser.user_id`, the repository's established free-text actor +representation with no foreign key to any local account (there is no local +account table). No credential, token, endpoint, bucket, host, adapter identity +or deployment identity is stored, logged or returned. The audit +`before_state`/`after_state` JSON is never exposed through GraphQL. The stale +error message contains no SQL, table name, column name, driver text or the +current stored token — disclosing the current token to a caller that just failed +a version check would let it blind-write over a change it never read. + +Security limitations: recorded in section 13. + +## 9. Tests and checks + +All commands were run from the repository root against **disposable local +services** (a scratch PostgreSQL 17.10 cluster and a local Redis). Nothing was +pointed at production or at a shared service. + +### 9.1 Migration and schema evidence + +Empty-database apply, revert, re-apply (database `thoth_be03_empty`): + +```bash +DATABASE_URL="postgres://thoth:thoth@localhost/thoth_be03_empty" cargo run migrate +DATABASE_URL="postgres://thoth:thoth@localhost/thoth_be03_empty" cargo run migrate --revert +DATABASE_URL="postgres://thoth:thoth@localhost/thoth_be03_empty" cargo run migrate +``` + +```text +apply -> __diesel_schema_migrations head = 20260813 +revert -> 0 migrations remain; 0 publisher/audit tables; 0 source types +re-apply -> 20250000 20260417 20260429 20260504 20260805 20260812 20260813 + publisher_service_configuration_history rows = 0 +``` + +Catalog verification (same database): + +**Re-verified at the remediation head**, after the actor predicate correction, +on a fresh disposable database `thoth_be03_mig` (PostgreSQL 17.10 Homebrew; +CI uses `postgres:17`). Apply on empty → revert → re-apply, all clean through the +embedded runner (`cargo run migrate`, `cargo run migrate --revert`). Post-revert +the table, the enum type and the `publisher` column are all absent; post-re-apply +the catalog reports: + +```text +publisher_service_configuration_history_actor_check => CHECK ((actor ~ '[^[:space:]]'::text)) +enum labels: SUPERUSER_API,MIGRATION_BACKFILL +index: publisher_service_configuration_history_pkey +index: publisher_service_configuration_history_publisher_created_idx +publisher column: service_configuration_updated_at timestamp with time zone nullable=NO +``` + +**Whitespace rejection matrix**, executed as real `INSERT`s against that live +constraint (not simulated), each rejected specifically by +`publisher_service_configuration_history_actor_check`: + +| Actor | Result | +|---|---| +| `''` (empty) | REJECTED | +| `' '` | REJECTED | +| `' '` | REJECTED | +| `'\t'` | REJECTED | +| `'\n'` | REJECTED | +| `'\r'` | REJECTED | +| `'\v'` (`U+000B`) | REJECTED | +| `'\f'` (`U+000C`) | REJECTED | +| `' \t\n\r\v\f '` | REJECTED | +| `' real-actor-42 '` | ACCEPTED | +| `'\t\nreal-actor-42\r\n'` | ACCEPTED | + +The POSIX class was verified against the actual server before the predicate was +chosen: `SELECT ch ~ '[[:space:]]'` is true for all six required classes, and +`SELECT ch ~ '[^[:space:]]'` is false for each of them, whereas +`btrim(ch) <> ''` is **true** for tab, newline, carriage return, vertical tab and +form feed — which is precisely the defect the correction closes. + +Catalog verification of the original apply (all values unchanged except the +constraint predicate): + +```text +publisher_service_configuration_source: 1 SUPERUSER_API, 2 MIGRATION_BACKFILL +publisher.service_configuration_updated_at | timestamp with time zone | NOT NULL | default CURRENT_TIMESTAMP +publisher_service_configuration_history columns: history_id uuid NOT NULL default uuid_generate_v4(); + publisher_id uuid NOT NULL; actor text NOT NULL; source USER-DEFINED NOT NULL; + before_state jsonb NOT NULL; after_state jsonb NOT NULL; + created_at timestamptz NOT NULL default CURRENT_TIMESTAMP +constraints: + publisher_service_configuration_history_actor_check CHECK ((actor ~ '[^[:space:]]'::text)) + publisher_service_configuration_history_pkey PRIMARY KEY (publisher_service_configuration_history_id) + publisher_service_configuration_history_publisher_id_fkey FOREIGN KEY (publisher_id) + REFERENCES publisher(publisher_id) ON DELETE CASCADE +indexes: + publisher_service_configuration_history_pkey + publisher_service_configuration_history_publisher_created_idx + btree (publisher_id, created_at DESC, publisher_service_configuration_history_id DESC) +non-internal triggers on the audit table: 0 (append-only: no updated_at column) +job tables: 0 capability tables: 0 capability columns: 0 +``` + +Populated-database forward migration (database `thoth_be03_populated`: 500 +publishers with all four packages, 500 imprints, 2 000 works, 875 BE-02 +assignments including 375 normalized OAPEN/DOAB pairs and 125 retained disabled +rows). The database was first brought to the exact pre-BE-03 state by running +BE-03's own `down.sql` and deleting its `__diesel_schema_migrations` row, then +the **real runner** applied the migration forward: + +```bash +DATABASE_URL="postgres://thoth:thoth@localhost/thoth_be03_populated" ./target/debug/thoth migrate +``` + +```text + BEFORE AFTER +relfilenode = 668819 relfilenode = 668819 (unchanged: no table rewrite) +packages = OASIS:125,OBELISK:125, packages = identical + PYRAMID:125,SPHINX:125 +assignment_digest = 4fc7a9038cff481cc657d5bf assignment_digest = identical (md5 over every + eda1f39e assignment column, ordered) +assignments = 875 assignments = 875 +works = 2000 works = 2000 + audit_rows = 0 + publishers_with_token = 500 + distinct_token_values = 1 + job_tables = 0 +real 0.04 (whole `thoth migrate` process, /usr/bin/time -p) +``` + +`relfilenode` is **unchanged**, confirming the `STABLE` `CURRENT_TIMESTAMP` +default was stored as a fast default with no table rewrite. + +Observed locking, taken by replaying `up.sql` inside one explicit transaction on +a `createdb -T` copy and self-inspecting `pg_locks` before commit: + +```text +BEGIN 0.161 ms +ALTER TABLE 0.725 ms +CREATE TYPE 0.274 ms +CREATE TABLE 2.264 ms +CREATE INDEX 0.439 ms + +relname | mode | granted +publisher | AccessExclusiveLock | t +publisher | AccessShareLock | t +publisher | ShareRowExclusiveLock | t +publisher_service_configuration_history | AccessExclusiveLock | t +publisher_service_configuration_history | AccessShareLock | t +publisher_service_configuration_history | ShareLock | t +publisher_service_configuration_history | ShareRowExclusiveLock | t +publisher_service_configuration_history_publisher_created_idx | AccessExclusiveLock | t +COMMIT 0.220 ms +``` + +`schema.rs` matches the migrated database contract: asserted in-process by +`the_migrated_database_matches_the_schema_contract`, and structurally by the +whole `thoth-api` suite compiling and running every Diesel query in this report +against the migrated database. + +### 9.2 Formatting + +```bash +cargo fmt --all -- --check +``` + +```text +(no output; exit 0) +``` + +```bash +git diff --check +``` + +```text +(no output; exit 0) +``` + +### 9.3 Static analysis + +```bash +cargo check --workspace +``` + +```text +Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 44s +``` + +```bash +cargo clippy --all --all-targets --all-features -- -D warnings +``` + +```text +Finished; no error and no clippy warning. +One pre-existing dependency notice remains, unrelated to BE-03: +"the following packages contain code that will be rejected by a future version + of Rust: proc-macro-error2 v2.0.1" +``` + +### 9.4 Tests + +```bash +cargo test -p thoth-api --features backend +``` + +```text +lib: 1055 passed; 0 failed +graphql_permissions: 13 passed; 0 failed +doc-tests: 0 passed; 8 ignored +``` + +```bash +cargo test --workspace +``` + +```text +thoth (lib) 0 passed; 0 failed +thoth (bin) 14 passed; 0 failed +thoth_api (lib) 1055 passed; 0 failed +graphql_permissions 13 passed; 0 failed +thoth_api_server 3 passed; 0 failed +thoth_client 4 passed; 0 failed +thoth_errors 11 passed; 0 failed +thoth_export_server 144 passed; 0 failed +doc-tests 8 passed; 8 ignored (thoth_client 6, thoth_export_server 2) +TOTAL 1252 passed; 0 failed +``` + +BE-03 adds **80** tests: 45 in +`model::publisher_service_configuration::tests`, 25 in +`graphql::service_configuration_tests`, 6 in the new test-only +`graphql::sdl_support`, 2 connection-scoped primitive regressions in +`model::publisher_distribution_platform::tests`, and 2 replacing 1 in the +amended `graphql::tests` SDL guard. + +The remediation added **8** of those: the 6 `sdl_support` extractor tests +(section 16.3), `every_committed_change_issues_exactly_one_publisher_update` +(section 9.7) and +`the_assignment_loader_chunks_a_page_larger_than_the_maximum_batch` +(section 9.5). Two existing tests were strengthened rather than added — the +actor-constraint test now asserts the full whitespace matrix, and +`the_migrated_database_matches_the_schema_contract` now asserts the constraint's +catalog definition rather than only its name. No test's expectation was +weakened or removed. + +```bash +cargo test -p thoth-export-server +``` + +```text +144 passed; 0 failed + 2 doc-tests passed +``` + +**`cargo test -p thoth-client` and `cargo build -p thoth-client` cannot be run +standalone in this repository, and this is pre-existing rather than caused by +BE-03.** Both fail with 26 `cannot find 'graphql' in crate` errors because only +the workspace/dev-dependency edge enables `thoth-api`'s `backend` feature, and +single-package feature unification drops it. The identical failure was +reproduced at the authorized base `3b6b3a31` in a clean worktree. The client is +therefore built and tested through the workspace forms above, where its 4 unit +tests and 6 doc-tests pass and `build.rs` regenerates the SDL. + +### 9.5 Query-count evidence + +Measured with the existing observed-loader harness +(`SqlProbe` + `RequestLoaders::for_request_observed`), which exercises the +**production** batcher, driving the real GraphQL report and requesting +`enabledDistributionPlatforms` on every summary: + +| Page size | Publisher-page statements | Latest-change statements | Assignment statements | Loader dispatch chunks | +|---:|---:|---:|---:|---| +| 1 | 1 | 1 | 1 | `[1]` | +| 25 | 1 | 1 | 1 | `[25]` | +| 200 | 1 | 1 | 1 | `[200]` | +| 201 | 1 | 1 | **2** | `[200, 1]` | + +**The accurate statement shape is bounded, not constant.** The merged ADR-0007 +assignment DataLoader is configured with +`crate::graphql::dataloader::MAX_BATCH_SIZE = 200`, so it chunks larger key +sets. For a report page containing N publishers the request issues: + +```text +2 set-based report statements ++ ceil(N / MAX_BATCH_SIZE) set-based assignment-loader dispatches +``` + +with **no per-publisher SQL loop**. The earlier phrasing "the count does not +grow with N" was wrong without that qualification: it holds only while +N ≤ `MAX_BATCH_SIZE`, which is why 201 is asserted as a regression point +(`the_assignment_loader_chunks_a_page_larger_than_the_maximum_batch`) alongside +the required 1, 25 and 200 measurements. At 201 the observed chunks are +`[200, 1]` and the observed assignment dispatch count is exactly 2, matching +`ceil(201 / 200)`. + +The latest-change statement contains `DISTINCT ON` and `= ANY`; every assignment +dispatch contains `= ANY`. The single-publisher protected query issues exactly +one assignment statement, one loader dispatch of size 1, and **zero** history +statements. No second assignment loader was introduced: asserted by +`no_second_assignment_loader_was_introduced`, and the protected resolver is +loader-first and `try_load`-only, asserted by source inspection. + +### 9.6 Publisher and work trigger evidence + +Fixture, as specification section 18.4 requires: one target publisher, **two** +imprints belonging to it, **two** works distributed across those imprints, and a +**control work belonging to a different publisher**. Every value is read +directly from the database before and after, against a real disposable +PostgreSQL database with the migration applied, so both publisher triggers +actually execute. + +| Case | `service_configuration_updated_at` | `publisher.updated_at` | every target `work.updated_at_with_relations` | control work | +|---|---|---|---|---| +| committed package-only change | moves | moves | moves, all | unchanged | +| committed platform-only change | moves | moves | moves, all | unchanged | +| committed linked repair (membership unchanged) | moves | moves | moves, all | unchanged | +| true semantic no-op | unmoved | unmoved | unmoved | unchanged | +| stale request | unmoved | unmoved | unmoved | unchanged | +| injected pre-commit failure / rollback | unmoved | unmoved | unmoved | unchanged | + +The platform-only and repair rows are asserted deliberately, with a comment in +the test saying so, because **merged BE-02 alone would have moved neither**: +BE-02 does not `UPDATE publisher`, and `publisher_distribution_platform` carries +no work-freshness trigger. BE-03 moves them because the same transaction writes +the configuration token to the publisher row. + +**A committed configuration change refreshes `work.updated_at_with_relations` +across the whole of that publisher's catalogue. That is a public, +downstream-consumed freshness signal**: it is resolved on the public `Work` type, +it is a filter and ordering key on the anonymous work queries, and +`thoth-export-server` uses it to decide Redis cache freshness — so the change +will cause that publisher's cached metadata records to be treated as stale and +regenerated on next request, and will cause incremental consumers to re-select +that publisher's works. **It is not distribution activation**: no distribution +job, job target, job attempt, upload, feed, message or dissemination is created +by it. + +### 9.7 Catalogue-scale write-amplification and lock-footprint measurement + +Disposable-environment measurement only, driven through the real GraphQL +mutation on a materially larger catalogue. **This is empirical evidence about +the shape of the cost. It is not a production SLA, it is not extrapolated to +production, and no "safe" catalogue size is derived from it.** + +Re-measured at the remediation head, after the single-publisher-`UPDATE` +correction. The previous head's figures (24 statements, 86.35 ms) are superseded +and are **not** reused: + +```text +target works: 2000 +control works (a different publisher): 250 +SQL statements issued by the configuration operation: 23 +publisher UPDATE statements: 1 +work rows changed by the publisher trigger: 2000 +unrelated publisher work rows changed: 0 +request duration in this disposable environment: 39.55 ms +``` + +This request changes the package **and** the platforms — the case that +previously cost two publisher `UPDATE`s — and now issues exactly one, captured +verbatim as statement 16: + +```sql +UPDATE "publisher" SET "subscription_package" = $1, + "service_configuration_updated_at" = + GREATEST(CURRENT_TIMESTAMP, + service_configuration_updated_at + interval '1 microsecond') +WHERE ("publisher"."publisher_id" = $2) RETURNING ... +``` + +Of the 23 captured statements: 2 are pooled-connection health checks +(`SELECT 1`), `BEGIN`/`COMMIT` are 2, 3 are Diesel `pg_type` OID lookups, 1 is +the post-commit DataLoader read serving the mutation's own response, and the +remaining 15 are the coordinator's own reads and writes — the publisher lock +(taken 3 times: once by the coordinator and once inside each connection-scoped +primitive, harmless no-ops after the first), the publisher row read, the enabled +assignment reads, the linked-group member reads and transaction timestamps, +three assignment upserts, the **single** publisher `UPDATE` carrying both the +package and the token, the after-state assignment read and the audit `INSERT`. +**The count is bounded by the closed 17-value platform inventory and does not +grow with catalogue size.** + +**No per-work application loop exists**: the request issues **no statement at +all** against the `work` table, asserted by the test. The 2 000 work rows are +changed by the existing `AFTER UPDATE` trigger's single set-based +`UPDATE work ... FROM imprint WHERE imprint.publisher_id = NEW.publisher_id`, +which now runs **once** for every committed change shape. Asserted per shape by +`every_committed_change_issues_exactly_one_publisher_update`: + +| Committed change | Publisher `UPDATE`s | Work-freshness cascades | +|---|---:|---:| +| package-only | 1 | 1 | +| platform-only | 1 | 1 | +| linked repair | 1 | 1 | +| combined package + platform | 1 | 1 | +| true no-op | 0 | 0 | +| stale | 0 | 0 | + +The transaction's real footprint is therefore `one publisher row + bounded +configuration/audit rows + N related work rows` for **every** committed change +shape — no longer `2N` for a combined change. The work-row locks are held for +the remainder of the transaction, and the publisher `FOR UPDATE` lock is held +while the trigger's work executes, so concurrent configuration writers for the +same publisher serialize behind all of it. BE-04 inherits this with the +transaction boundary. + +**Stop-condition 19 reassessment, from the new evidence: NOT TRIGGERED.** At +2 000 works the whole request took 39.55 ms in this disposable environment — +against 86.35 ms for the same fixture before the correction — the statement +count is bounded at 23 and independent of catalogue size, the cascade is now +exactly one set-based statement per committed change rather than up to two, and +unrelated publishers are provably untouched. The implementing agent does **not** +judge this a material operational problem at realistic catalogue sizes. The +measurement is placed before the independent reviewer to assess independently. +No trigger, package location or token architecture was altered; the correction +removed a redundant `UPDATE` from BE-03's own coordinator only. + +### 9.8 Concurrency evidence + +All against a real database with two threads and separate pooled connections: + +- two clients holding one token: exactly one commits, the loser fails + `StalePublisherServiceConfiguration`, exactly one audit row exists, and the + final persisted state equals what the winner committed; +- the same with **linked OAPEN/DOAB** assignments: the loser leaves no partial or + one-sided pair; both rows end enabled with one shared activation and one + shared `enabled_at`; +- a stale request that would have been a **true no-op**: still fails, moves + nothing; +- a stale request that would have **repaired a split pair**: still fails, the + split pair survives byte-identically, no token movement, no audit row — + proving the version check precedes every lifecycle call; +- two concurrent membership-equal **repair** requests: one repairs, one is + stale, the final pair is normalized with one shared activation and exactly one + audit row exists; +- concurrent replacements on **different** publishers: both commit, one audit + row each, no contention; +- a replacement concurrent with a **direct BE-02** `disable`: both complete, + serialized on the same publisher row lock, **no deadlock**, and every + assignment row satisfies `enabled == (disabled_at IS NULL)` afterwards; +- **strict token monotonicity** per publisher across a sequence of committed + changes that includes a repair: every successive token is strictly greater and + all values are distinct. + +### 9.9 Audit evidence + +- exactly one audit row per committed change, including a change that updates + the package **and** several linked groups and singletons in one request; +- `before_state` and `after_state` key sets are **exactly** + `{configurationVersion, enabledDistributionPlatforms, subscriptionPackage}`; + the test fails if any key is ever added, and separately asserts the serialized + JSON contains no `activation`, `enabledAt`, `disabledAt`, `capabilit`, + `zitadel`, `publisherName`, `credential`, `token`, `endpoint` or `bucket`; +- platforms are serialized in canonical `DistributionPlatform::ALL` order — + observed audit value: `["OAPEN","DOAB","ZENODO"]` for a request naming + `[OAPEN, ZENODO]`; +- for a linked-state repair the two states are equal in `subscriptionPackage` + and `enabledDistributionPlatforms` and differ only in `configurationVersion`, + with `before_state.configurationVersion` equal to the superseded token and + `after_state.configurationVersion` equal to the new one; +- `actor` equals the caller's `PolicyContext::user_id()`; +- `source` is `SUPERUSER_API` for **every** row BE-03 writes, and no BE-03 path + writes `MIGRATION_BACKFILL` — asserted over a three-change sequence; +- the actor check enforces the non-whitespace invariant: it rejects `''`, `' '`, + `' '`, `'\t'`, `'\n'`, `'\r'`, `'\u{0b}'`, `'\u{0c}'` and a mixed + whitespace string, and accepts `' real-actor-42 '` and + `'\t\nreal-actor-42\r\n'` — asserted by + `the_database_rejects_an_actor_with_no_non_whitespace_character` against the + real database constraint (section 9.1 carries the catalog evidence); +- a committed configuration change writes **no** `publisher_history` row, + confirming the coordinator does not route the package update through + `Crud::update`; +- deleting a publisher cascades its audit rows away. + +**`publisher_history.data` additive-key consequence.** `publisher_history` and +`publisher_service_configuration_history` are **not the same thing**. +`publisher_history` is the pre-existing generic entity-history table written by +the shared `Crud::update` macro for publisher metadata edits, keyed by +`user_id`, storing a whole-entity snapshot with a legacy +`timestamp without time zone`; BE-03's mutation never writes it. Because +`service_configuration_updated_at` is appended to the `Publisher` struct that +`Crud::update` serializes, the new field **may appear as an additional key in +future `publisher_history.data` snapshots wherever that path runs** — for +example an ordinary `updatePublisher` metadata edit. That is an additive JSON key +in an untyped `jsonb` column: no existing key changes meaning, no existing row is +rewritten, and the table, its columns and its triggers are not modified. This is +observed and accepted, not a defect. + +### 9.10 Capability evidence + +- field name and type: `effectiveCapabilities: [PublisherCapability!]!` on + `PublisherServiceConfiguration`; +- for **every** `ThothPackage` the returned list equals + `ThothPackage::capabilities()` for that package **as an exact ordered + sequence**, not set equality, so a later sort, dedup or reorder fails the test; + repeated reads return an identical sequence; +- `OASIS` returns an empty list, never `null`; +- the package reported in the same response always agrees with the capabilities, + because both are read from the same locked publisher row — one derivation + site, no second mapping; +- a package **upgrade** (`OASIS -> SPHINX`) and **downgrade** + (`SPHINX -> OBELISK`) change the capabilities automatically, in the mutation's + own returned configuration **and** in a subsequent query, with no separate + capability write, migration, backfill or reconciliation; +- a platform-only change leaves the capabilities unchanged; +- **no capability state is persisted anywhere**: the migration creates no + capability column, table, override or index, asserted by catalog query; the + only durable input is `publisher.subscription_package`; +- `PublisherCapability` is SDL-reachable **only** through + `PublisherServiceConfiguration.effectiveCapabilities` — exactly one field in + the entire schema returns it — and no anonymous operation can select a + capability or package value; the public `Publisher` type contains no package, + capability or configuration-version field of any spelling. + +### 9.11 Linked-platform and independence evidence + +- requesting OAPEN alone, or DOAB alone, enables both with one shared activation + and one shared `enabled_at`; omitting both disables both, retaining the rows; +- **membership-equal repairs**, each executed with the requested enabled + membership already equal to the current membership so a membership-only diff + would be empty: + + | Seeded split state | Request | Result | + |---|---|---| + | both enabled, **different `activation_id`** | `OAPEN` | repaired to one shared activation and timestamp; token bumped; exactly 1 audit row; states differ only in `configurationVersion` | + | both enabled, same activation, **different `enabled_at`** | `DOAB` | as above | + | **one-sided** (OAPEN enabled, DOAB absent) | `OAPEN` | as above | + | **one-sided** (OAPEN enabled, DOAB absent) | `DOAB` | as above | + | fully normalized pair | `OAPEN` | **true no-op**: nothing written, rows byte-identical, token unmoved, no audit row | + +- a package-only change over already normalized state leaves every assignment + row **byte-identical** (`activation_id`, `enabled_at`, `disabled_at`, + `updated_at` all equal); +- a package change submitted together with a **split** requested group writes the + package **and** repairs the group, under one token bump and one audit row; +- a platform-only change leaves `subscription_package` unchanged; +- `OCLC_KB` and `EX_LIBRIS_KB` receive independent activations and are disabled + independently with no coupling; +- `JISC_NBK` fails with `DistributionPlatformNotAssignable`, writing nothing, + moving no token and creating no audit row — asserted **both** when the request + would otherwise have changed nothing **and** when it would otherwise have + changed the package and enabled a valid platform, proving whole-set + pre-validation precedes the first lifecycle call; +- duplicates are deduplicated with no error; an empty list disables everything + and is never read as "all"; +- a request naming several linked groups and several singletons writes one audit + row and bumps the token once. + +### 9.12 BE-02 regression evidence + +- BE-02's **40** existing lifecycle tests in + `model::publisher_distribution_platform::tests` pass **unchanged** against the + refactored connection-scoped functions; no behavioural expectation was edited; +- BE-02's four public read surfaces return identical results, ordering and + errors, and their generated SDL signatures are asserted **verbatim** by the + amended guard; +- the pool-level `enable`/`disable` behaviour is unchanged, including that a + non-assignable platform still fails **before** any connection is acquired or + transaction opened (the check is the first statement of `enable`, ahead of + `db.get()?`); +- the connection-scoped primitives' outcomes are asserted directly for every + transition: absent row enabled -> `Changed`; already-enabled singleton -> + `Unchanged`; enabled group disabled -> `Changed`; group with no enabled member + disabled -> `Unchanged`; disabled row re-enabled -> `Changed`; + already-normalized linked group -> `Unchanged`; split pair -> `Changed`; +- **direct `enable_on(JISC_NBK)` regression.** Called directly inside a + caller-owned transaction, bypassing both the pool-level wrapper and the + coordinator so nothing has pre-validated the platform, it returns + `ThothError::DistributionPlatformNotAssignable("JISC_NBK")`; **no** row is + created for `JISC_NBK` or any other platform; **no** existing assignment row + changes (enabled state, `activation_id`, `enabled_at`, `disabled_at` and + `updated_at` all byte-identical); the caller's transaction is then **both + rolled back and committed** in two separate runs with no hidden mutation in + either case, proving the primitive failed before any write rather than relying + on rollback; and `publisher.service_configuration_updated_at`, + `publisher.updated_at` and the audit table are all unaffected because this path + never reaches the coordinator's committed-change phase; +- the coordinator's own whole-set pre-validation is proven separately + (section 9.11), so the two checks are established independently rather than one + masking the other. + +### 9.13 Write-path containment evidence + +Searched paths, all verified present: + +```text +thoth-api/src thoth-api-server/src thoth-client/src +thoth-errors/src thoth-export-server/src src +thoth-api/migrations +``` + +Workspace-declaration check, so no local production crate was omitted from the +scope: + +```bash +grep -n "members" Cargo.toml +``` + +```text +15:members = ["thoth-api", "thoth-api-server", "thoth-client", "thoth-errors", "thoth-export-server"] +``` + +`thoth-app` is a **separate repository** and does not exist here +(`ls -d thoth-app` -> `No such file or directory`), so no `thoth-app/src` path +was searched and none is presented as evidence. + +**Call-site enumeration.** + +```bash +grep -rn 'replace_publisher_service_configuration' +``` + +| Location | Classification | +|---|---| +| `thoth-api/src/model/publisher_service_configuration/crud.rs:67` | definition | +| `thoth-api/src/graphql/mutation.rs:85` | **production — the only production caller** (inside `replace_service_configuration`, called by the resolver at `mutation.rs:111`) | +| `thoth-api/src/graphql/service_configuration_tests.rs:145` | test fixture | +| `thoth-api/src/model/publisher_service_configuration/tests.rs:64` | test helper | +| `crud.rs:6`, `mod.rs:9`, `mutation.rs:36`, `tests.rs:19`, `service_configuration_tests.rs:25` | doc comment / import | + +```bash +grep -rnE 'PublisherDistributionPlatform::(enable|disable)\(' +``` + +**61 hits, every one in a `tests.rs` file**, file by file: + +| File | Hits | Classification | +|---|---:|---| +| `thoth-api/src/model/publisher_distribution_platform/tests.rs` | 45 | test | +| `thoth-api/src/graphql/distribution_platform_tests.rs` | 14 | test | +| `thoth-api/src/model/publisher_service_configuration/tests.rs` | 2 | test (the direct-BE-02 concurrency test) | +| **Total** | **61** | **production: 0** | + +`grep -v 'tests\.rs:'` over the same result set returns nothing, so **BE-02's +pool-level lifecycle functions have zero production call sites**, exactly as +specification section 2.1 item 7 recorded. BE-03 therefore establishes the +single production write path without displacing any existing production caller. + +> **Correction.** The previous head's report stated "66 hits … 50 in +> `model/publisher_distribution_platform/tests.rs`". Both numbers were wrong and +> not reproducible. The figures above were produced by re-running the search at +> the remediation head with `/usr/bin/grep` and the seven explicit scope paths, +> and the per-file counts were derived from the same result set with +> `awk -F: '{c[$1]++}'` rather than counted by hand. The security-relevant +> invariant — zero production callers — was and remains unchanged. + +```bash +grep -rnE '(enable_on|disable_on)\(' +``` + +**15 hits**, file by file: `model/publisher_distribution_platform/crud.rs` 4, +`model/publisher_distribution_platform/tests.rs` 9, +`model/publisher_service_configuration/crud.rs` 2. + +| Location | Classification | +|---|---| +| `model/publisher_distribution_platform/crud.rs:103,175` | definitions | +| `model/publisher_distribution_platform/crud.rs:80,162` | production — BE-02's own pool-level wrappers delegating to them | +| `model/publisher_service_configuration/crud.rs:135,151` | **production — the coordinator, the only production configuration caller** | +| `model/publisher_distribution_platform/tests.rs` (9 hits) | tests | + +**Bypass search.** + +```bash +grep -rn 'subscription_package' | grep -v 'tests.rs:' +grep -rn 'service_configuration_updated_at' | grep -v 'tests.rs:' +grep -rn 'publisher_service_configuration_history' | grep -v 'tests.rs:' +grep -rnE 'diesel::(insert_into|update|delete)' thoth-api/src --include=*.rs \ + | grep -v 'tests.rs:' \ + | grep -E 'publisher::table|publisher_distribution_platform|publisher_service_configuration_history' +grep -rn 'sql_query' | grep -v 'tests.rs:' | grep -v 'fixture.rs' +grep -rn 'diesel(table_name = publisher' thoth-api/src --include=*.rs | grep -v tests.rs +``` + +Complete relevant production matches, with every write classified: + +| Write target | Production writer | Classification | +|---|---|---| +| `publisher.subscription_package` | `model/publisher_service_configuration/crud.rs:187` | the coordinator, step 10 — **the only writer** | +| `publisher.service_configuration_updated_at` | `model/publisher_service_configuration/crud.rs:187,195` | the coordinator, step 10 — **the only writer**; the two lines are the package-carrying and token-only branches of the **same single** `UPDATE` | +| `publisher_distribution_platform` (INSERT) | `model/publisher_distribution_platform/crud.rs:122` | inside `enable_on` | +| `publisher_distribution_platform` (UPDATE) | `model/publisher_distribution_platform/crud.rs:192` | inside `disable_on` | +| `publisher_service_configuration_history` (INSERT) | `model/publisher_service_configuration/crud.rs:223` | the coordinator, step 11 — **the only writer** | + +Exactly one of the two `crud.rs` publisher branches executes per committed +change, so a committed change is one `UPDATE`. `publisher::table` is written +inline in both branches rather than through a hoisted local, specifically so +both remain visible to this search. + +Every other production match of those identifiers is a `schema.rs` column +declaration, a struct field, a doc comment, a **read** (`crud.rs:436` package +filter, `crud.rs:443` assignment filter subquery, `crud.rs:480-493` latest-change +read, `graphql/model.rs:1399-1444` resolvers, `publisher/crud.rs:151,232` BE-02's +merged reverse-lookup joins) or a migration file. + +**No implicit writer exists through Diesel changesets.** The only +`AsChangeset`/`Insertable` types targeting `publisher` are `NewPublisher` and +`PatchPublisher`, and **neither declares `subscription_package` or +`service_configuration_updated_at`**: + +```rust +pub struct PatchPublisher { + pub publisher_id: Uuid, + pub publisher_name: String, + pub publisher_shortname: Option, + pub publisher_url: Option, + pub zitadel_id: Option, + pub accessibility_statement: Option, + pub accessibility_report_url: Option, +} +``` + +so `Publisher::update` (the `updatePublisher` mutation) writes the publisher row +without being able to touch either protected column, and `Publisher::create` +leaves both to their defaults. The only production raw SQL anywhere in scope is +`SET CONSTRAINTS ... DEFERRED` and `pg_advisory_xact_lock` in unrelated modules. + +**Conclusion.** The GraphQL mutation calls the coordinator; there is no second +production configuration writer; BE-02's pool-level lifecycle functions have no +production call site at all and are therefore not production +service-configuration entry points; the connection-scoped primitives are used +for configuration purposes only by the coordinator; and nothing outside the +coordinator writes configuration package, platform, token or audit state. + +**Token/audit coupling**, proven and honestly bounded: every committed change +made through the coordinator moves the token and writes exactly one audit row, +asserted across the committed-change, repair, multi-group, upgrade and downgrade +tests. Recorded as a limitation rather than an enforced property: the +lower-level primitives remain *capable* of writing assignments without the +token, so the single-writer invariant is enforced by this specification, this +review and the search evidence above — **not by the type system**. A future task +needing a second configuration writer must extend the coordinator rather than +bypass it. + +## 10. Manual verification + +Environment: local disposable PostgreSQL 17.10 (Homebrew, scratch cluster, trust +auth) and local Redis, both created for this task and holding no production +data. No production or shared service was contacted at any point. + +Steps and observed results are the measured evidence in sections 9.1–9.13; every +one is an automated assertion or a recorded command output rather than an +unverified claim. + +## 11. CI + +CI status: **PASSING** — normal `pull_request`-triggered CI on the draft PR. No +workflow file was changed and no workflow was manually dispatched. + +The durations below are from the run on the implementation commit. Every push to +this branch re-runs the same ten checks, and each run has passed; the live +per-head result is the pull request's own check record under ADR-0005. + +| Check | Result | Duration | +|---|---|---| +| `classify` (build/lint/test workflow) | pass | 8s | +| `classify` (migrations workflow) | pass | 5s | +| `classify` (staging image workflow) | pass | 9s | +| `check-changelog` | pass | 5s | +| `format_check` | pass | 6s | +| `build` | pass | 2m26s | +| `lint` | pass | 5m23s | +| `test` | pass | 6m5s | +| `run_migrations` | pass | 3m20s | +| `build_and_push_staging_docker_image` | pass | 9m11s | + +Failures or warnings: none. `run_migrations` independently applies the new +migration in CI's own PostgreSQL service, and `build` regenerates the SDL through +`thoth-client/build.rs`, so the generated-contract path is exercised there as +well. + +## 12. Rollout and rollback + +Initial state after merge: **repository history changes only.** No deployment +occurs, no migration is executed anywhere, no configuration changes and no API +behaviour becomes available to any client. + +Activation required: none, and none is authorized. Deployment and migration +execution remain separately gated by CG-13 and separate release authorization. + +Feature flag/configuration: none. A protected superuser-only mutation does not +require one, and no repository or programme authority establishes one. + +Migration sequence: `20260813_v1.7.0` applies after `20260812_v1.7.0`, in the +ordinary embedded-runner sequence. + +Monitoring required: none. BE-03 activates nothing to monitor and adds no log, +metric or alert. The mutation logs no audit JSON, actor identity, token or +credential. + +Rollback: + +- **before environment adoption** — no deployment, no migration executed, no + audit or configuration data exists: an ordinary code revert of the pull + request is possible under normal review; +- **after environment adoption** — retain the additive foundation; forward + repair is preferred. An incorrect publisher configuration is corrected through + a reviewed and audited configuration change, which itself writes an audit row, + never by editing or deleting history. **Do not destroy audit history as + routine rollback**: dropping a populated + `publisher_service_configuration_history` requires separate explicit + authorization. `down.sql` is reversibility **evidence**, not an automatic + production rollback procedure. + +Even after a separately authorized deployment and migration, this is **not** a +claim of zero observable downstream data change: changing protected desired +configuration creates no distribution job, performs no upload and triggers no +dissemination worker, but the existing `set_work_updated_at_with_relations` +trigger **will** refresh `work.updated_at_with_relations` across that publisher's +catalogue and invalidate that publisher's cached export records (section 9.6). +That freshness cascade is an existing catalogue-freshness mechanism reacting to a +real publisher-row change; it must **not** be described as distribution +activation, which remains separately gated and unauthorized. + +## 13. Known limitations and deferred work + +1. **The single-write-coordinator invariant is not type-enforced.** It is held by + specification, review and the section 9.13 search evidence. The lower-level + BE-02 primitives remain capable of writing assignments without the token. +2. ~~**`btrim(actor) <> ''` trims spaces only.**~~ **RESOLVED BY CTO-DIRECTED + SPECIFICATION CORRECTION.** See section 16.1. The authoritative invariant is + that an audit actor must contain at least one non-whitespace character; the + constraint is now `CHECK (actor ~ '[^[:space:]]')` under the same name, and + the whitespace matrix is proven rejected. This is no longer an accepted + limitation. +3. **`EntityNotFound` and `DistributionPlatformNotAssignable` still surface as + `INTERNAL_ERROR`.** Changing that would alter BE-02's merged contract and + normalize unrelated error families; recorded rather than silently changed. +4. **`ThothError::DatabaseError` still renders the underlying driver message.** + Pre-existing behaviour; BE-03 adds nothing to that exposure. +5. **A committed configuration change is publicly observable by timing.** The + protected *values* stay behind the section 8 read decision, but an observer + polling `updatedAtWithRelations` or watching export-cache regeneration can + infer that something changed for that publisher at that time. This is + acknowledged, not denied. +6. ~~**A package change updates the publisher row twice**~~ **RESOLVED BY + CTO-AUTHORIZED BOUNDED CORRECTION.** See section 16.1. Step 8 now compares + only and step 10 issues a single conditional `UPDATE`, so every committed + change shape costs exactly one publisher `UPDATE` and one work-freshness + cascade. Measured in section 9.7 and asserted per shape by + `every_committed_change_issues_exactly_one_publisher_update`. +7. **`thoth-client` cannot be built or tested as a single package** in this + repository. Pre-existing and reproduced at the authorized base; the workspace + forms are used instead. This is a build-topology property of the repository, + not a BE-03 limitation. +8. ~~**`thoth-client/assets/schema.graphql` is gitignored**, so the requirement to + commit it is unsatisfiable.~~ **RESOLVED BY CTO-AUTHORIZED SPECIFICATION + CORRECTION.** See section 16.1. The specification no longer requires + committing the generated artifact; it requires regeneration through the normal + build path plus the recorded diff, artifact hash, client verification and + backend head, which is what sections 7.2, 7.4 and 7.5 provide. Nothing here is + unsatisfiable. +9. Deferred by design and unauthorized here: BE-04 durable jobs, MIG-01 backfill + (`MIGRATION_BACKFILL` is defined and never written), APP-01, APP-02, and any + deployment, environment or production migration, assignment creation, + distribution activation or `OBSERVE`/`ENFORCE` transition. + +## 14. Unresolved issues + +NONE. + +## 15. Agent self-assessment + +The implementing agent may identify risks but may not approve the task. + +Suggested review focus: + +1. **The coordinator's transaction sequence** against specification section 7.3, + step by step — especially that staleness precedes validation and every + lifecycle call, that `enable_on` is invoked unconditionally for every desired + group, and that the token bump and single audit insert are gated on the + aggregated outcome. +2. **The write-path containment evidence** in section 9.13, since the + single-writer invariant is not type-enforced. Re-run the searches at the exact + head. +3. **The two authorized SDL guard amendments**: confirm the replacements preserve + and strengthen the original security intent rather than weakening it — the + public `Publisher` type, the BE-02 types, `PublisherCapability` reachability + through exactly one field, and BE-02's four verbatim read surfaces. +4. **The section 9.7 catalogue-scale measurement**, and whether the reviewer + agrees with the implementing agent's stop-condition 19 assessment that the + cascade is not a material operational problem. +5. **The remediation cycle of section 16**, in particular that the two + specification corrections stay within what the CTO authorized and widen + nothing else. +6. **The corrected actor constraint**: the catalog definition, the whitespace + rejection matrix, and that no audit architecture, column, constraint name or + source semantics moved with it. +7. **Migration reversibility and the populated-database evidence**, including + the unchanged `relfilenode` and the byte-identical assignment digest. +8. **The corrected numeric evidence** — the 61 pool-level hits, the + `ceil(N / MAX_BATCH_SIZE)` statement formula, and the re-measured + catalogue-scale figures — all of which should be reproduced independently at + the exact head rather than taken from this report. + +## 16. Remediation cycle + +### 16.1 Independent review and CTO decision + +The exact head `1315057983d389d1ef6b85bc4e69b81eda53aa79` received a fresh +independent HIGH-risk implementation review whose decision was **BLOCKED**, on +five findings: + +| # | Severity | Finding | +|---|---|---| +| 1 | P1 | SDL guard coverage defect | +| 2 | P1 | actor-check specification contradiction | +| 3 | P2 | double publisher `UPDATE` / doubled trigger cascade | +| 4 | P2 | DataLoader statement-count overclaim | +| 5 | P2 | lifecycle call-site count arithmetic | + +Finding 2 was an authority conflict — the approved specification's exact DDL +(`CHECK (btrim(actor) <> '')`) contradicted its own acceptance-test wording +(reject "a blank or whitespace-only actor") — and only the CTO could resolve it. +The CTO resolved it and authorized bounded corrections to all five findings plus +the generated-schema wording. This remediation implements exactly that +authorization and nothing wider. + +### 16.2 Material CTO-directed corrections + +1. **Actor invariant resolved toward non-whitespace.** The authoritative + invariant is that an audit actor must contain at least one non-whitespace + character. The specification's DDL and its §9.2, §15, §18.4 and §18.7 wording + now express that one invariant, and the unmerged migration's predicate is + `CHECK (actor ~ '[^[:space:]]')`. The table, the `actor text NOT NULL` column, + the constraint name `publisher_service_configuration_history_actor_check` and + the whole audit architecture are unchanged; only the predicate is + strengthened, and no second constraint was added. `source` semantics, + `SUPERUSER_API`, `MIGRATION_BACKFILL`, actor provenance, the audit JSON and + the history-table structure are untouched. +2. **Generated-schema contract aligned to established repository mechanics.** + `thoth-client/assets/schema.graphql` is build-generated and ignored under + existing repository authority (root `AGENTS.md` §12.2, `thoth-client/AGENTS.md` + §1). Specification §14.3 item 3 no longer requires committing it and instead + requires regeneration through the normal build path, the exact SDL diff + against the authorized base, a reproducible artifact identity, workspace-path + client verification and the backend head for APP-01 pinning. + `thoth-client/.gitignore` was not modified, the artifact was not force-added, + and `thoth-app` was not touched. +3. **Doubled publisher `UPDATE` removed.** Specification §7.3 steps 8 and 10 and + the coordinator now defer the package write into a single conditional + publisher `UPDATE` issued after the lifecycle outcomes are known. Sections + 9.7 and 9.5 carry the re-measured evidence. +4. **SDL guard coverage repaired.** Both authorized guards now extract the whole + generated type body. Section 16.3. +5. **Query-count wording corrected** to the bounded + `2 + ceil(N / MAX_BATCH_SIZE)` formula, with a 201-publisher regression. + Section 9.5. +6. **Bypass evidence corrected**: the pool-level enable/disable count is 61 at + this head, with per-file counts derived mechanically. Section 9.13. + +### 16.3 SDL guard extraction + +The previous `sdl_block()` helper took `.split_once('}')`, which stops at the +**first** closing brace after the declaration. In the real generated SDL the +public `Publisher` type declares +`order: ImprintOrderBy = {direction: "ASC", field: "IMPRINT_NAME"}` inside the +`imprints` field, so extraction stopped there and every field after `imprints` — +`contacts` and `distributionPlatforms` — was never inspected. A protected field +added after `imprints` would have passed both guards silently. + +Extraction now lives in one place, `thoth-api/src/graphql/sdl_support.rs`, and is +**brace-balanced and string-aware**. String awareness is required, not +defensive: the real SDL contains braces inside descriptions (`\d{4,9}` in the +`doi` description), escaped quotes (the `Timestamp` description) and `"""` block +strings inside a type body (`Imprint.crossmarkDoi`). + +Measured on the real generated SDL, and on a tampered copy with +`subscriptionPackage: ThothPackage!` inserted **after** `imprints`: + +| Extraction | `Publisher` block | contains `contacts(` | contains `distributionPlatforms:` | catches tampered field | +|---|---:|---|---|---| +| old `split_once('}')` | 1203 chars | no | no | **no** | +| new brace-balanced | 1733 chars | yes | yes | **yes** | + +Both guards now additionally assert those two post-`imprints` sentinels as a +coverage precondition, so a future truncation fails the guard rather than +silently shrinking it. `sdl_support` carries six of its own tests, including the +tampered-field regression and two `should_panic` cases. The real regenerated +`Publisher` type contains none of `subscriptionPackage`, `effectiveCapabilities`, +any capability field, `serviceConfiguration*` or `serviceConfigurationUpdatedAt`. +BE-02's four approved public read surfaces are asserted verbatim and unchanged. +The guards were strengthened, never weakened or deleted, and no production +GraphQL contract changed — the regenerated SDL is byte-identical to the previous +head (section 7.4). diff --git a/docs/engineering/ai-delivery/tasks/BE-03.md b/docs/engineering/ai-delivery/tasks/BE-03.md index bab33689..25f9f491 100644 --- a/docs/engineering/ai-delivery/tasks/BE-03.md +++ b/docs/engineering/ai-delivery/tasks/BE-03.md @@ -372,7 +372,10 @@ Re-derived from the merged code at the authoring base: `ThothError::DistributionPlatformNotAssignable` and `ThothError::EntityNotFound` already exist. 11. **Generated contract** is `thoth-client/assets/schema.graphql`, written by - `thoth-client/build.rs` from `thoth_api::graphql::create_schema()`. + `thoth-client/build.rs` from `thoth_api::graphql::create_schema()`. It is a + build product, ignored through `thoth-client/.gitignore`, so it is + regenerated rather than tracked; section 14.3 item 3 states how BE-03 + evidences it. The tracked client contract file is `assets/queries.graphql`. ## 3. Explicit scope @@ -821,11 +824,17 @@ one connection: `ThothError::DistributionPlatformNotAssignable` before the first lifecycle call, so a rejected request never depends on rollback to leave the configuration untouched. -8. Update `subscription_package` only if it differs. Record whether it changed. - The coordinator writes the column directly inside its own transaction; it does - **not** route the change through the shared `Crud::update` macro, so the - mutation writes no `publisher_history` row and the configuration audit of - section 8 remains the only history BE-03 writes (section 6.4). +8. Compare the requested `subscriptionPackage` with the package read under the + lock in step 3 and record whether it differs as `package_changed`. **Do not + update the publisher row here.** The package write is deferred to the single + publisher `UPDATE` of step 10, so a change to both the package and the + platform state still updates the publisher row exactly once and therefore + still fires the existing `AFTER UPDATE` work-freshness trigger exactly once. + When that update happens, the coordinator writes the column directly inside + its own transaction; it does **not** route the change through the shared + `Crud::update` macro, so the mutation writes no `publisher_history` row and + the configuration audit of section 8 remains the only history BE-03 writes + (section 6.4). 9. Apply the desired platform state **through BE-02's connection-scoped lifecycle primitives**, per section 7.7, never by writing `publisher_distribution_platform` rows directly, as follows: @@ -855,16 +864,34 @@ one connection: the report, not this mutation. That bound covers the statements BE-03 issues. It does **not** describe - everything the transaction writes: step 10's publisher `UPDATE` fires the - existing `AFTER UPDATE` trigger, which issues one further set-based statement - updating every work of that publisher (sections 2.1 item 8 and 6.4). The - transaction's total row-write footprint is therefore one publisher row, plus - the bounded configuration and audit rows, plus **N related work rows**, where - N is the number of works currently belonging to that publisher through its - imprints. N is a database-side effect of one set-based trigger statement, not - a per-work application loop, and BE-03 must not introduce one. -10. If and only if step 8 or step 9 changed something, set - `service_configuration_updated_at` per section 6.2. + everything the transaction writes: step 10's **single** publisher `UPDATE` + fires the existing `AFTER UPDATE` trigger, which issues one further set-based + statement updating every work of that publisher (sections 2.1 item 8 and + 6.4). The transaction's total row-write footprint for a committed change is + therefore one publisher row, plus the bounded configuration and audit rows, + plus **N related work rows**, where N is the number of works currently + belonging to that publisher through its imprints — and it is that same + footprint whether the change was package-only, platform-only, a linked + repair, or package and platform combined, because all four commit through one + publisher `UPDATE`. N is a database-side effect of one set-based trigger + statement, not a per-work application loop, and BE-03 must not introduce one. +10. If and only if step 8 or step 9 changed something, issue **exactly one** + publisher `UPDATE`, which in a single statement: + 1. writes `subscription_package` to the requested value when + `package_changed`, and leaves the column untouched otherwise; + 2. sets `service_configuration_updated_at` per section 6.2; + 3. returns the resulting publisher row, so the `after_state` of step 11 is + read from what was actually persisted. + + If neither step 8 nor step 9 changed anything, the request is the true no-op + of section 7.4 and issues **zero** publisher `UPDATE` statements. + + One publisher `UPDATE` per committed change is a required property, not an + incidental one: the publisher row carries the shared `AFTER UPDATE` + work-freshness trigger, so a second `UPDATE` in the same transaction would + run that trigger's set-based cascade a second time over the same N work rows + for no additional effect. A combined package-and-platform change must + therefore cost the same one cascade as a platform-only change. 11. If and only if step 8 or step 9 changed something, insert exactly one `publisher_service_configuration_history` row for the whole committed change, with the `source` and `actor` supplied by the caller's write @@ -1227,7 +1254,7 @@ CREATE TABLE public.publisher_service_configuration_history ( FOREIGN KEY (publisher_id) REFERENCES public.publisher(publisher_id) ON DELETE CASCADE, CONSTRAINT publisher_service_configuration_history_actor_check - CHECK (btrim(actor) <> '') + CHECK (actor ~ '[^[:space:]]') ); CREATE INDEX publisher_service_configuration_history_publisher_created_idx @@ -1249,11 +1276,19 @@ Notes: non-ZITADEL migration control identity, and a column named `user_id` would then assert something untrue. **No new identifier format, credential, secret or local account table is introduced.** -- the `CHECK (btrim(actor) <> '')` constraint makes "explicit, stable actor +- the `CHECK (actor ~ '[^[:space:]]')` constraint makes "explicit, stable actor identity" a database property rather than a convention: no write path, - present or future, can record an anonymous or blank actor. Section 18.7 - requires catalog verification of the constraint and a test proving a blank - actor is rejected. + present or future, can record an anonymous or blank actor. **The authoritative + invariant is that an audit actor must contain at least one non-whitespace + character**, and the predicate above is the exact expression of it: the POSIX + `[[:space:]]` class covers space, tab, newline, carriage return, vertical tab + and form feed, so `actor ~ '[^[:space:]]'` rejects the empty string and every + whitespace-only string built from those classes, while accepting any actor that + carries a real identifier even when it is surrounded by whitespace. A narrower + `btrim(actor) <> ''` predicate would satisfy only part of that invariant — it + trims spaces alone, so a tab-only or newline-only actor would pass — and is + therefore not used. Section 18.7 requires catalog verification of the + constraint and tests proving the whole whitespace matrix is rejected. - actor and source are explicit columns, not fields buried in free-form JSON, so they are indexable, typed and greppable. - `created_at timestamptz` follows BE-02. The legacy @@ -1363,8 +1398,9 @@ incoherent reserved value to avoid: coherently in one place rather than inheriting the misleading `user_id` name (section 8.1); 3. `source` disambiguates the namespace, so a `MIGRATION_BACKFILL` actor can - never be mistaken for a ZITADEL user id, and the `CHECK (btrim(actor) <> '')` - constraint makes an anonymous actor impossible for any write path; + never be mistaken for a ZITADEL user id, and the + `CHECK (actor ~ '[^[:space:]]')` constraint of section 8.1 makes an anonymous + actor impossible for any write path; 4. section 9.3 binds MIG-01 to the same coordinator and the same persistence invariants, so the value's semantics are fixed by BE-03 rather than left for a later task to improvise; @@ -1878,13 +1914,29 @@ protected by section 11.1. []` as the merged enum-list precedent. If the generated lines render as non-null lists, the implementation — not this specification — is wrong and must be corrected before the diff is accepted; -3. commit the regenerated `thoth-client/assets/schema.graphql`; +3. **do not hand-edit the generated schema, and do not commit it.** + `thoth-client/assets/schema.graphql` is build-generated by + `thoth-client/build.rs` and is ignored through `thoth-client/.gitignore` + under existing repository authority — the root `AGENTS.md` section 12.2 and + `thoth-client/AGENTS.md` section 1 both govern it. BE-03 must not hand-edit + it, must not newly commit it, must not force-add it and must not modify + `thoth-client/.gitignore` to make committing it possible. The evidence + obligation is discharged instead by recording, in the implementation report: + the exact generation command used through the repository's normal build path; + the exact additive/removal SDL diff against the authorized base, produced by + regenerating the base artifact the same way; and a reproducible identity for + the generated artifact — its SHA-256, or the repository-equivalent checksum — + at the exact head being reviewed; 4. update `thoth-client/assets/queries.graphql` and the client's generated types only if the client actually consumes a changed surface. BE-03 adds protected operations the internal export client does not use, so the expected outcome is **no query change**; if that holds it must be stated as a - reviewed conclusion, not an omission; -5. build and test `thoth-client`; + reviewed conclusion, not an omission. `assets/queries.graphql` **is** tracked + and is committed when it changes; only the generated schema artifact is + ignored; +5. verify `thoth-client` compatibility by building and testing it through the + repository-supported workspace path, recording the exact commands and + results; 6. assess `thoth-app` codegen compatibility as an additive-only change and record the conclusion. **BE-03 does not modify `thoth-app`**; 7. record the exact backend commit SHA of the reviewed BE-03 head in the @@ -1908,9 +1960,9 @@ Schema changes: - `CREATE TYPE public.publisher_service_configuration_source` with the two closed values of section 9.1; - `CREATE TABLE public.publisher_service_configuration_history` with its - `actor text NOT NULL` column, its named non-blank actor check constraint, its - primary key, its `ON DELETE CASCADE` foreign key and its composite index, per - section 8.1. + `actor text NOT NULL` column, its named non-whitespace actor check constraint + `CHECK (actor ~ '[^[:space:]]')`, its primary key, its `ON DELETE CASCADE` + foreign key and its composite index, per section 8.1. `down.sql` drops the table, the type and the column. @@ -2179,7 +2231,12 @@ Also required: `PolicyContext::user_id()`; - `source` is `SUPERUSER_API` for **every** row BE-03 writes, and no BE-03 path can write `MIGRATION_BACKFILL`; -- the non-blank actor check constraint rejects a blank or whitespace-only actor. +- the section 8.1 actor check constraint enforces the invariant that **an audit + actor must contain at least one non-whitespace character**. It must be proven + to reject the empty string and every whitespace-only actor, covering at least + spaces, tabs, newlines, carriage returns, vertical tabs, form feeds and a mixed + whitespace string, and to accept an actor carrying a real non-whitespace + identifier surrounded by whitespace. **Timestamp-movement evidence (section 6.4).** All six cases must be reproduced against a **real disposable PostgreSQL database** with the migration applied, so @@ -2241,7 +2298,12 @@ Also required: - a test asserting that a committed configuration change writes **no** `publisher_history` row, confirming the coordinator does not route the package - update through `Crud::update` (section 7.3 step 8); + update through `Crud::update` (section 7.3 step 10); +- tests asserting the committed write footprint of section 7.3 step 10: exactly + **one** publisher `UPDATE` for a package-only change, a platform-only change, a + linked repair and a combined package-and-platform change, and **zero** for a + true no-op and for a stale request, with no per-work application loop in any + case; - the implementation report must explicitly record the `publisher_history.data` consequence of section 6.4: because `service_configuration_updated_at` is appended to the `Publisher` struct that @@ -2369,8 +2431,9 @@ Also required: - `pg_class.relfilenode` for `publisher` compared before and after, with the observed lock and duration recorded; - catalog verification of the new type and its exact two labels in order, the - table, its primary key, its foreign key, the named non-blank actor check - constraint, the composite index and the new `publisher` column; + table, its primary key, its foreign key, the named non-whitespace actor check + constraint — whose catalog definition must be the section 8.1 predicate, not a + narrower `btrim` form — the composite index and the new `publisher` column; - `thoth-api/src/schema.rs` matches the migrated database contract. ### 18.8 Query-count evidence @@ -2555,8 +2618,12 @@ must additionally record: - the exact SDL diff, including the new `ThothPackage` and `PublisherCapability` enum blocks, and the three list-argument lines quoted **verbatim** and compared explicitly against the merged siblings named in section 14.3 item 2; -- the exact backend commit SHA for APP-01 contract pinning, and the schema - artifact that SHA pins (section 14.3 item 7); +- the exact backend commit SHA for APP-01 contract pinning, and the reproducible + identity (SHA-256 or repository-equivalent checksum) of the generated schema + artifact that SHA pins, regenerated through the normal build path at that exact + head rather than committed (section 14.3 items 3 and 7); +- the `thoth-client` compatibility result, with the exact + repository-supported workspace commands used (section 14.3 item 5); - the effective-capability evidence: the field's final name and type, that its value is `ThothPackage::capabilities()` for every package with the asserted canonical ordering, that no capability state is persisted anywhere, that a diff --git a/docs/publisher-services/task-status.md b/docs/publisher-services/task-status.md index f0652d2f..4f0aac90 100644 --- a/docs/publisher-services/task-status.md +++ b/docs/publisher-services/task-status.md @@ -4,7 +4,7 @@ Status: ACTIVE TRACKER Programme owner: CTO Master issue: [#765](https://github.com/thoth-pub/thoth/issues/765) Approved design: [private Google Doc](https://docs.google.com/document/d/1kr2Ft0Y4pxgcXGyFAKs_wfFx4I0jlxEvaceswE5Dus8/edit), Drive revision `3` -Last updated: 2026-08-12 (BE-02 implementation merged; BE-02 closed as an inactive foundation) +Last updated: 2026-08-13 (BE-03 specification repository-authoritative on `develop`; BE-03 implementation delivered as a draft pull request, independently reviewed at its previous exact head with a BLOCKED decision, CTO-authorized bounded remediation delivered, and the new exact head awaiting a fresh independent review) ## 1. Control rule @@ -25,7 +25,7 @@ No task moves to `READY` without an approved specification, architecture depende | LIC-02 Enforce supported licences | `thoth` | HIGH | BLOCKED | `develop` / `develop` | LIC-01 release; production licence audit plan | #765 | TBD | NOT STARTED | | [BE-01 Publisher package model](../engineering/ai-delivery/tasks/BE-01.md) | `thoth` | HIGH | CLOSED | `develop` at `37b802776ae6853affe19d90156f3c1e0654ebe3` (PR #778 merge commit, verified before any edit) / `develop` | None remaining for BE-01 itself: the separately authorized bounded implementation was delivered on `feature/publisher-services/be-01` under ADR-0003 Architecture A and merged into `develop` through implementation PR [#779](https://github.com/thoth-pub/thoth/pull/779) after fresh independent exact-head review and explicit CTO merge authorization, as required for every HIGH-risk merge. Production migration/release execution remains separately gated by open CG-13, and the MIG-01 commercial backfill remains a separately approved CRITICAL task. | [#765](https://github.com/thoth-pub/thoth/issues/765) | Specification [#774](https://github.com/thoth-pub/thoth/pull/774); implementation [#779](https://github.com/thoth-pub/thoth/pull/779) | CLOSED - INACTIVE FOUNDATION - all publishers `OASIS`; no consumer, package API, mutation, UI, distribution, OAI or Metrics behaviour activated; retained-foundation operational rollback applies; evidence in the [BE-01 implementation report](../engineering/ai-delivery/implementation-reports/BE-01-implementation-report.md) and the immutable exact-head comments on PR #779 | | [BE-02 Distribution platform model](../engineering/ai-delivery/tasks/BE-02.md) | `thoth` | HIGH | CLOSED | `develop` at `1c752a522f7048963efde00b50565379d7c14b4d` (PR #788 merge commit, verified before any edit) / `develop` | None remaining for BE-02 itself: ADR-01/the final inventory is satisfied through PR #783, ADR-0007 through PR #800, and the request-local non-cached DataLoader foundation through PR #802 (`8dcf031d`). The reconciled BE-02 specification was independently reviewed, CTO-approved and merged through PR #788, making it repository-authoritative at `1c752a52`; the CTO then separately authorized implementation against that exact `develop` SHA, and the bounded implementation was delivered on `feature/publisher-services/be-02` and merged into `develop` through implementation PR [#805](https://github.com/thoth-pub/thoth/pull/805) after fresh independent exact-head review and explicit CTO merge authorization, as required for every HIGH-risk merge. Deployment, environment migration execution, production migration, assignment creation/backfill and distribution activation remain separately gated and unauthorized. | #765 | Specification [#788](https://github.com/thoth-pub/thoth/pull/788); implementation [#805](https://github.com/thoth-pub/thoth/pull/805) | CLOSED - INACTIVE FOUNDATION - the 17-value `DistributionPlatform` enum, the `publisher_distribution_platform` migration and repository-authoritative `schema.rs`, the assignment lifecycle, linked OAPEN/DOAB normalization, four additive public GraphQL read surfaces and the first production ADR-0007 DataLoader adoption are merged, with evidence in the [BE-02 implementation report](../engineering/ai-delivery/implementation-reports/BE-02-implementation-report.md) and the immutable exact-head comments on PR #805. The migration creates zero assignment rows, no distribution behaviour is activated, and merge is not deployment, migration execution, backfill or activation authorization | -| [BE-03 Protected service configuration](../engineering/ai-delivery/tasks/BE-03.md) | `thoth` | HIGH | IMPLEMENTATION NOT AUTHORIZED | `develop` / `develop` | BE-01 (CLOSED, satisfied); BE-02 (CLOSED, satisfied). Remaining: the written implementation specification at [`tasks/BE-03.md`](../engineering/ai-delivery/tasks/BE-03.md) becomes repository-authoritative when its exact CTO-approved content is reachable from `develop`; the [BE-03/BE-04/APP-01 phase-boundary programme decision](decisions.md) is a specification candidate whose authority condition is that same event — explicit CTO specification approval of the exact content carrying it, plus that content reaching `develop` — and needs no separate lifecycle-status edit once both hold; and implementation additionally requires separate fresh-base explicit CTO authorization. The reserved implementation branch `feature/publisher-services/be-03` must not exist until then. | #765 | Specification [#808](https://github.com/thoth-pub/thoth/pull/808); implementation PR not applicable until implementation is authorized | NOT STARTED - no runtime, migration, schema or GraphQL change exists | +| [BE-03 Protected service configuration](../engineering/ai-delivery/tasks/BE-03.md) | `thoth` | HIGH | IMPLEMENTATION IN REVIEW | `develop` at `3b6b3a31f9358011f0c998015dfd0c2508380e83` (specification PR #808 merge commit, verified before any edit) / `develop` | BE-01 (CLOSED, satisfied); BE-02 (CLOSED, satisfied). The specification at [`tasks/BE-03.md`](../engineering/ai-delivery/tasks/BE-03.md) is repository-authoritative: its exact CTO-approved content merged into `develop` through PR [#808](https://github.com/thoth-pub/thoth/pull/808) as `3b6b3a31`, which also satisfies the authority condition of the [BE-03/BE-04/APP-01 phase-boundary programme decision](decisions.md) with no separate lifecycle-status edit required. The CTO then separately authorized implementation against that exact `develop` SHA, and the bounded implementation was delivered on `feature/publisher-services/be-03`. An independent HIGH-risk implementation review of the previous exact head `1315057983d389d1ef6b85bc4e69b81eda53aa79` returned BLOCKED on five findings; the CTO resolved the single blocking authority conflict (the actor check-constraint contradiction) and authorized bounded corrections, now delivered on the same branch. Remaining for BE-03 itself: a fresh independent implementation review of the **new** exact head, and explicit CTO merge authorization, as required for every HIGH-risk merge. Deployment, environment migration execution, production migration, assignment creation/backfill and distribution activation remain separately gated and unauthorized. | #765 | Specification [#808](https://github.com/thoth-pub/thoth/pull/808); implementation [#809](https://github.com/thoth-pub/thoth/pull/809) (draft) | IMPLEMENTATION DELIVERED, NOT MERGED - the additive migration (configuration token, closed audit-source type, append-only audit table), repository-authoritative `schema.rs`, the single service-configuration write coordinator, the protected owner-and-superuser read, the superuser-only staff report and replace mutation, derived effective capabilities and the connection-scoped `BE-02` lifecycle refactor are implemented with evidence in the [BE-03 implementation report](../engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md). The migration creates zero audit rows and changes no package or assignment; no distribution job, dissemination or activation exists; and no environment or production migration has been executed | | BE-04 Durable distribution jobs | `thoth` | HIGH | BLOCKED | `develop` / `develop` | BE-02 (CLOSED, satisfied); BE-03 | #765 | TBD | NOT STARTED | | MIG-01 Audit/production backfill | `thoth` + operations | CRITICAL | BLOCKED | dedicated task branch -> `develop`; separately approved production run | BE-01 and BE-02 (CLOSED, satisfied); BE-03; licence audit; dry run | #765 | TBD | NOT STARTED | | APP-01 Service configuration UI | `thoth-app` | MEDIUM | BLOCKED | current `dev` / `dev` pending BR-APP-01 or exception | BE-03 exposing the approved protected API; app readiness controls (BR-APP-01 or explicit CTO exception; the separately specified CG-11 CI closure task); generated API contract pinned to the exact BE-03 commit SHA per the reserved contract control; own approved bounded specification. Scope boundary: under the candidate [BE-03/BE-04/APP-01 phase boundary](decisions.md), BE-03 alone supports only APP-01's configuration scope — own-publisher reads of package, effective capability codes and enabled platforms; superuser read/edit; capability-driven UI affordances; backend-driven linked-platform behaviour; optimistic-concurrency handling; and server-normalized state. APP-01 elements rendering durable back-catalogue job status, attempt state, failure state or pending-onboarding state additionally depend on BE-04 and must not be planned against BE-03 alone | #765 | TBD | NOT STARTED | @@ -147,30 +147,48 @@ Each branch starts from the repository's verified development branch and targets integration only; deployment, environment migration execution, production migration, assignment creation or backfill and distribution activation remain separately gated and unauthorized. -10. BE-03's BE-01 and BE-02 dependencies are satisfied. The written - implementation specification is - [`tasks/BE-03.md`](../engineering/ai-delivery/tasks/BE-03.md), which becomes - repository-authoritative when its exact CTO-approved content is reachable from - `develop`. It carries one programme-decision candidate — the +10. BE-03's BE-01 and BE-02 dependencies are satisfied and its specification is + **repository-authoritative**: the exact CTO-approved content of + [`tasks/BE-03.md`](../engineering/ai-delivery/tasks/BE-03.md) merged into + `develop` through PR + [#808](https://github.com/thoth-pub/thoth/pull/808) as + `3b6b3a31f9358011f0c998015dfd0c2508380e83`. Both halves of the authority + condition therefore hold, so the BE-03/BE-04/APP-01 phase boundary is an + approved programme decision with no further status edit required. The CTO + separately authorized implementation against that exact freshly verified + `develop` head, and the bounded implementation was delivered on + `feature/publisher-services/be-03` as a **draft** pull request following + ADR-0003 Architecture A (direct `thoth-api/src/schema.rs` edit in the same + bounded PR as the migration, models, GraphQL contract and tests). Evidence is + recorded in the + [BE-03 implementation report](../engineering/ai-delivery/implementation-reports/BE-03-implementation-report.md); + live review, authorization and merge evidence lives only in that pull + request's record under ADR-0005 and is not restated here. An independent + HIGH-risk implementation review of the previous exact head returned + **BLOCKED** on five findings, one of which was an authority conflict between + the approved specification's exact actor DDL and its own acceptance-test + wording; the CTO resolved that conflict and authorized bounded corrections, + which have been delivered on the same branch without rebase or history + rewrite. BE-03 is **not merged**: the resulting new exact head requires a + **fresh** independent review — the earlier review applies only to the head it + examined — and explicit CTO merge authorization remains outstanding. Merge + would authorize repository integration only — deployment, environment + migration execution, production migration, assignment creation or backfill + and distribution activation remain separately gated and unauthorized. The + specification carried one programme-decision candidate — the [BE-03/BE-04/APP-01 phase boundary](decisions.md), under which BE-03 owns desired configuration only and creates no durable job, no placeholder job and no fabricated job status, and under which APP-01's job-aware elements depend - on BE-04 rather than on BE-03 alone. That candidate's authority condition is - explicit CTO specification approval of the exact content carrying it plus that - content reaching `develop`; once both hold it is an approved programme - decision with no further status edit required. The protected configuration - surface follows ADR-0001 section 4.4 and exposes the current package, the - effective capability codes derived from BE-01's code-owned + on BE-04 rather than on BE-03 alone. The protected configuration surface + follows ADR-0001 section 4.4 and exposes the current package, the effective + capability codes derived from BE-01's code-owned `ThothPackage::capabilities()` and stored nowhere, and the enabled - distribution platforms. The specification additionally fixes one authoritative + distribution platforms. The implementation fixes one authoritative service-configuration write coordinator, so the canonical configuration version token and the configuration audit history cannot be bypassed by a - production writer, and it states the protected-read authorization as a + production writer, and it implements the protected-read authorization as a least-privilege per-publisher `PUBLISHER_USER` (or superuser) check with no - role inheritance, covering package and capability codes alike. BE-03 - implementation is `NOT AUTHORIZED` and additionally requires separate explicit - authorization from a freshly verified `develop` head; the reserved branch - `feature/publisher-services/be-03` must not exist until then. + role inheritance, covering package and capability codes alike. 11. APP-01 remains blocked pending BE-03, app readiness controls (BR-APP-01 or an explicit CTO exception, and the separately specified CG-11 CI closure), the exact BE-03 SHA/schema-pinning contract control, and its own approved @@ -180,11 +198,13 @@ Each branch starts from the repository's verified development branch and targets state and pending-onboarding state require BE-04. 12. Beyond the delivered and merged documentation-only ADR-01 implementation, ADR-01-CLOSEOUT-01 control reconciliation, the shared DataLoader - foundation and the merged inactive BE-02 foundation, no BE-03, BE-04, - APP-01, OAI-PMH, deployment, release, production migration, assignment - creation or backfill, distribution activation, `OBSERVE`/`ENFORCE` - transition or PR #799 action is authorized; all licence, migration, app, - dissemination and operational tasks remain blocked under their recorded - dependencies. Repository authority and specification work do not - authorize runtime change, credential use, workflow dispatch or production - access. + foundation, the merged inactive BE-02 foundation and the separately + authorized, delivered-but-unmerged BE-03 implementation, no BE-04, + MIG-01, APP-01, APP-02, OAI-PMH, deployment, release, environment or + production migration, assignment creation or backfill, distribution + activation, `OBSERVE`/`ENFORCE` transition or PR #799 action is + authorized; all licence, migration, app, dissemination and operational + tasks remain blocked under their recorded dependencies. Repository + authority, specification work and BE-03 implementation authorization do + not authorize runtime change, credential use, workflow dispatch or + production access. diff --git a/thoth-api/migrations/20260813_v1.7.0/down.sql b/thoth-api/migrations/20260813_v1.7.0/down.sql new file mode 100644 index 00000000..3537caab --- /dev/null +++ b/thoth-api/migrations/20260813_v1.7.0/down.sql @@ -0,0 +1,6 @@ +DROP TABLE IF EXISTS public.publisher_service_configuration_history; + +DROP TYPE IF EXISTS public.publisher_service_configuration_source; + +ALTER TABLE public.publisher + DROP COLUMN IF EXISTS service_configuration_updated_at; diff --git a/thoth-api/migrations/20260813_v1.7.0/up.sql b/thoth-api/migrations/20260813_v1.7.0/up.sql new file mode 100644 index 00000000..6caa6c8d --- /dev/null +++ b/thoth-api/migrations/20260813_v1.7.0/up.sql @@ -0,0 +1,31 @@ +ALTER TABLE public.publisher + ADD COLUMN service_configuration_updated_at timestamp with time zone + DEFAULT CURRENT_TIMESTAMP NOT NULL; + +CREATE TYPE public.publisher_service_configuration_source AS ENUM ( + 'SUPERUSER_API', + 'MIGRATION_BACKFILL' +); + +CREATE TABLE public.publisher_service_configuration_history ( + publisher_service_configuration_history_id uuid + DEFAULT public.uuid_generate_v4() NOT NULL, + publisher_id uuid NOT NULL, + actor text NOT NULL, + source public.publisher_service_configuration_source NOT NULL, + before_state jsonb NOT NULL, + after_state jsonb NOT NULL, + created_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, + CONSTRAINT publisher_service_configuration_history_pkey + PRIMARY KEY (publisher_service_configuration_history_id), + CONSTRAINT publisher_service_configuration_history_publisher_id_fkey + FOREIGN KEY (publisher_id) + REFERENCES public.publisher(publisher_id) ON DELETE CASCADE, + CONSTRAINT publisher_service_configuration_history_actor_check + CHECK (actor ~ '[^[:space:]]') +); + +CREATE INDEX publisher_service_configuration_history_publisher_created_idx + ON public.publisher_service_configuration_history + USING btree (publisher_id, created_at DESC, + publisher_service_configuration_history_id DESC); diff --git a/thoth-api/src/graphql/distribution_platform_tests.rs b/thoth-api/src/graphql/distribution_platform_tests.rs index 282c8610..ad6a3d15 100644 --- a/thoth-api/src/graphql/distribution_platform_tests.rs +++ b/thoth-api/src/graphql/distribution_platform_tests.rs @@ -18,6 +18,7 @@ use uuid::Uuid; use super::dataloader::fixture::{BatchStats, SqlProbe}; use super::dataloader::RequestLoaders; +use super::sdl_support::sdl_block; use super::{create_schema, Context, GraphQLRequest, Schema}; use crate::db::PgPool; use crate::model::publisher_distribution_platform::{ @@ -450,13 +451,7 @@ fn sdl_adds_exactly_the_approved_public_inventory() { } // Exactly 17 enum values, and the linked-group and behaviour vocabularies. - let platform_enum = sdl - .split_once("enum DistributionPlatform {") - .expect("DistributionPlatform enum") - .1 - .split_once('}') - .expect("enum body") - .0; + let platform_enum = sdl_block(&sdl, "enum DistributionPlatform {"); for code in [ "INTERNET_ARCHIVE", "OAPEN", @@ -481,22 +476,10 @@ fn sdl_adds_exactly_the_approved_public_inventory() { assert!(!platform_enum.contains("OTHER")); assert!(!platform_enum.contains("UNKNOWN")); - let group_enum = sdl - .split_once("enum DistributionPlatformGroup {") - .expect("group enum") - .1 - .split_once('}') - .expect("enum body") - .0; + let group_enum = sdl_block(&sdl, "enum DistributionPlatformGroup {"); assert!(group_enum.contains("OAPEN_DOAB")); - let behaviour_enum = sdl - .split_once("enum BackCatalogueBehaviour {") - .expect("behaviour enum") - .1 - .split_once('}') - .expect("enum body") - .0; + let behaviour_enum = sdl_block(&sdl, "enum BackCatalogueBehaviour {"); for value in ["AUTOMATIC_PUSH", "PULL_FEED", "MANUAL"] { assert!(behaviour_enum.contains(value)); } @@ -521,13 +504,7 @@ fn sdl_exposes_no_internal_or_protected_distribution_state() { // Activation identity, retained history, package/capability state and any // endpoint or credential identity stay out of the assignment type. - let assignment_type = sdl - .split_once("type PublisherDistributionPlatformAssignment {") - .expect("assignment type") - .1 - .split_once('}') - .expect("type body") - .0; + let assignment_type = sdl_block(&sdl, "type PublisherDistributionPlatformAssignment {"); for forbidden in [ "activationId", "disabledAt", @@ -545,13 +522,7 @@ fn sdl_exposes_no_internal_or_protected_distribution_state() { assert!(assignment_type.contains("platform: DistributionPlatform!")); assert!(assignment_type.contains("enabledAt: Timestamp!")); - let option_type = sdl - .split_once("type DistributionPlatformOption {") - .expect("option type") - .1 - .split_once('}') - .expect("type body") - .0; + let option_type = sdl_block(&sdl, "type DistributionPlatformOption {"); for forbidden in [ "adapterProfile", "mechanismReadiness", @@ -568,17 +539,75 @@ fn sdl_exposes_no_internal_or_protected_distribution_state() { ); } - // BE-02 exposes no package/capability state and no protected BE-03 surface. - for forbidden in [ - "subscriptionPackage", - "PublisherServiceConfiguration", - "replacePublisherServiceConfiguration", + // BE-02's own surfaces expose no package, capability or protected + // service-configuration state. + // + // BE-03 adds `PublisherServiceConfiguration`, + // `replacePublisherServiceConfiguration` and a protected + // `subscriptionPackage` field, so the previous whole-document string + // prohibition is intentionally false from BE-03 onwards. Its security + // intent — that none of that state is reachable from a BE-02 surface — + // is preserved here as per-type and per-field assertions, which are + // stricter than the string search they replace: they would still fail if a + // package, capability or configuration field were added to any BE-02 type + // or to the public `Publisher`. + for be02_type in [ + "type DistributionPlatformOption {", + "type PublisherDistributionPlatformAssignment {", + "type Publisher {", ] { + let block = sdl_block(&sdl, be02_type); + for forbidden in [ + "subscriptionPackage", + "ThothPackage", + "apabilit", + "PublisherServiceConfiguration", + "serviceConfiguration", + ] { + assert!( + !block.contains(forbidden), + "`{be02_type}` must not expose `{forbidden}`" + ); + } + } + + // Coverage precondition for the public `Publisher` prohibitions above. Both + // fields are declared after `imprints`, whose nested object default ended + // the previous `split_once('}')` extraction, so this asserts the guard saw + // the whole declaration rather than a truncated prefix of it. + let publisher_type = sdl_block(&sdl, "type Publisher {"); + for post_imprints_sentinel in ["contacts(", "distributionPlatforms:"] { assert!( - !sdl.contains(forbidden), - "SDL must not expose `{forbidden}`" + publisher_type.contains(post_imprints_sentinel), + "guard coverage is incomplete: `{post_imprints_sentinel}` is declared after \ + `imprints` but was not extracted: {publisher_type}" + ); + } + + // BE-02's four public read surfaces keep their exact merged signatures: the + // protected BE-03 additions change none of them. + for merged_surface in [ + "distributionPlatformOptions: [DistributionPlatformOption!]!", + "publishersByDistributionPlatform(\"Distribution platform to search on\" platform: DistributionPlatform!, \"The number of items to return\" limit: Int = 100, \"The number of items to skip\" offset: Int = 0, \"The order in which to sort the results. Results are always additionally sorted by publisher ID ascending, so pagination is deterministic\" order: PublisherOrderBy = {direction: \"ASC\", field: \"PUBLISHER_NAME\"}): [Publisher!]!", + "publisherCountByDistributionPlatform(\"Distribution platform to search on\" platform: DistributionPlatform!): Int!", + "distributionPlatforms: [PublisherDistributionPlatformAssignment!]!", + ] { + assert_eq!( + sdl.matches(merged_surface).count(), + 1, + "BE-02 surface changed: `{merged_surface}`" ); } + + // The protected BE-03 configuration is reachable only through its own + // protected operations, never through a BE-02 read surface. + assert_eq!(sdl.matches("): PublisherServiceConfiguration!").count(), 2); + for protected_operation in [ + "publisherServiceConfiguration(\"Thoth publisher ID to search on\" publisherId: Uuid!): PublisherServiceConfiguration!", + "replacePublisherServiceConfiguration(\"Complete desired service configuration to store\" data: ReplacePublisherServiceConfigurationInput!): PublisherServiceConfiguration!", + ] { + assert_eq!(sdl.matches(protected_operation).count(), 1); + } } #[test] diff --git a/thoth-api/src/graphql/mod.rs b/thoth-api/src/graphql/mod.rs index 7f5d7c74..74ba2511 100644 --- a/thoth-api/src/graphql/mod.rs +++ b/thoth-api/src/graphql/mod.rs @@ -9,6 +9,10 @@ mod mutation_guard; #[cfg(test)] mod mutation_guard_tests; mod query; +#[cfg(all(test, feature = "backend"))] +pub(crate) mod sdl_support; +#[cfg(test)] +mod service_configuration_tests; pub use juniper::http::GraphQLRequest; diff --git a/thoth-api/src/graphql/model.rs b/thoth-api/src/graphql/model.rs index a5b94340..064a8deb 100644 --- a/thoth-api/src/graphql/model.rs +++ b/thoth-api/src/graphql/model.rs @@ -35,11 +35,15 @@ use crate::model::{ AccessibilityException, AccessibilityStandard, Publication, PublicationOrderBy, PublicationType, }, - publisher::Publisher, + publisher::{Publisher, PublisherCapability, ThothPackage}, publisher_distribution_platform::{ BackCatalogueBehaviour, DistributionPlatform, DistributionPlatformGroup, DistributionPlatformOption, PublisherDistributionPlatformAssignment, }, + publisher_service_configuration::{ + PublisherServiceConfiguration, PublisherServiceConfigurationChange, + PublisherServiceConfigurationSource, PublisherServiceConfigurationSummary, + }, r#abstract::{Abstract, AbstractOrderBy, AbstractType}, reference::{Reference, ReferenceOrderBy}, series::{Series, SeriesType}, @@ -1381,6 +1385,107 @@ impl PublisherDistributionPlatformAssignment { } } +#[juniper::graphql_object( + Context = Context, + description = "The desired service configuration of one publisher." +)] +impl PublisherServiceConfiguration { + #[graphql(description = "The publisher this configuration belongs to")] + pub fn publisher(&self) -> &Publisher { + &self.publisher + } + + #[graphql(description = "Subscription package currently assigned to the publisher")] + pub fn subscription_package(&self) -> ThothPackage { + self.publisher.subscription_package + } + + #[graphql( + description = "Capabilities the current subscription package grants this publisher, in canonical capability order. Derived from the package; a capability permits a feature but does not configure or activate it" + )] + pub fn effective_capabilities(&self) -> Vec { + // Exactly `BE-01`'s code-owned mapping for the package on the same row + // that `subscriptionPackage` reports, in that `&'static` slice's order. + // No sort, no dedup, no filter, no second mapping, no persistence. + self.publisher.subscription_package.capabilities().to_vec() + } + + #[graphql( + description = "Distribution platforms currently enabled for the publisher, in canonical platform order" + )] + pub async fn enabled_distribution_platforms( + &self, + context: &Context, + ) -> FieldResult> { + // Loader-first (`ADR-0007` section 4.5), reusing `BE-02`'s existing + // request-local `publisher_distribution_platforms` loader rather than + // introducing a second assignment loader: the publisher ID is already + // available on `self`, so the key is registered at resolver entry with + // no unrelated awaited work before `try_load`. + unpack_assignments( + context + .loaders + .publisher_distribution_platforms + .try_load(self.publisher_id()) + .await, + ) + } + + #[graphql( + description = "Version token of this configuration; supply it as expectedUpdatedAt to replace the configuration" + )] + // The GraphQL field is `updatedAt` on the configuration type and must + // resolve to the **configuration** token, not to `publisher.updated_at`. + // Those two values are deliberately different and are not interchangeable + // in either direction (specification section 6.4 item 3), so clippy's + // getter-name heuristic is wrong here. + #[allow(clippy::misnamed_getters)] + pub fn updated_at(&self) -> Timestamp { + self.publisher.service_configuration_updated_at + } +} + +#[juniper::graphql_object( + Context = Context, + description = "A publisher's service configuration together with its latest change metadata." +)] +impl PublisherServiceConfigurationSummary { + #[graphql(description = "The publisher's desired service configuration")] + pub fn configuration(&self) -> &PublisherServiceConfiguration { + &self.configuration + } + + #[graphql( + description = "Metadata of the most recent recorded configuration change, or null if none has been recorded" + )] + pub fn last_change(&self) -> Option<&PublisherServiceConfigurationChange> { + self.last_change.as_ref() + } +} + +#[juniper::graphql_object( + Context = Context, + description = "Metadata of one recorded service-configuration change. The before and after states themselves are not exposed." +)] +impl PublisherServiceConfigurationChange { + #[graphql(description = "When the change was committed")] + pub fn changed_at(&self) -> Timestamp { + self.changed_at + } + + #[graphql( + description = "Identity that made the change: the account identifier for SUPERUSER_API, or the authorized control identity for a controlled backfill" + )] + pub fn actor(&self) -> &String { + &self.actor + } + + #[graphql(description = "How the change entered the system")] + pub fn source(&self) -> PublisherServiceConfigurationSource { + self.source + } +} + #[juniper::graphql_object(Context = Context, description = "The brand under which a publisher issues works.")] impl Imprint { #[graphql(description = "Thoth ID of the imprint")] diff --git a/thoth-api/src/graphql/mutation.rs b/thoth-api/src/graphql/mutation.rs index 1b6cfc88..fe60ea6a 100644 --- a/thoth-api/src/graphql/mutation.rs +++ b/thoth-api/src/graphql/mutation.rs @@ -32,6 +32,11 @@ use crate::model::{ NewPublication, PatchPublication, Publication, PublicationPolicy, PublicationProperties, }, publisher::{NewPublisher, PatchPublisher, Publisher, PublisherPolicy}, + publisher_service_configuration::{ + crud::replace_publisher_service_configuration, PublisherServiceConfiguration, + PublisherServiceConfigurationSource, ReplacePublisherServiceConfigurationInput, + ServiceConfigurationWriteContext, + }, r#abstract::{Abstract, AbstractPolicy, NewAbstract, PatchAbstract}, reference::{NewReference, PatchReference, Reference, ReferencePolicy}, series::{NewSeries, PatchSeries, Series, SeriesPolicy}, @@ -53,10 +58,33 @@ use crate::storage::{ run_cleanup_plan_sync, temp_key, work_cleanup_plan, work_featured_video_cleanup_plan, StorageConfig, }; -use thoth_errors::ThothError; +use juniper::IntoFieldError; +use thoth_errors::{ThothError, ThothResult}; pub struct MutationRoot; +/// Authorize a service-configuration replacement, build its write context and +/// delegate to the canonical coordinator. +/// +/// The resolver's responsibilities are exactly these: authorize, build the write +/// context, call the coordinator and map the result. Every read, validation, +/// lock, write, version bump and audit insert belongs to the coordinator's +/// single transaction. +fn replace_service_configuration( + context: &Context, + data: &ReplacePublisherServiceConfigurationInput, +) -> ThothResult { + // Superuser only, denied before the database is touched. There is no + // self-service configuration path in BE-03: every non-superuser is denied + // whatever publisher-scoped roles it holds for the target publisher. + context.require_superuser()?; + let write_context = ServiceConfigurationWriteContext { + source: PublisherServiceConfigurationSource::SuperuserApi, + actor: context.user_id()?, + }; + replace_publisher_service_configuration(&context.db, &write_context, data) +} + #[juniper::graphql_object(Context = Context)] impl MutationRoot { #[graphql(description = "Create a new work with the specified values")] @@ -77,6 +105,17 @@ impl MutationRoot { Publisher::create(&context.db, &data).map_err(Into::into) } + #[graphql( + description = "Replace a publisher's complete desired service configuration under optimistic concurrency control. Superuser only. This stores desired configuration: it creates no distribution job and triggers no dissemination" + )] + fn replace_publisher_service_configuration( + context: &Context, + #[graphql(description = "Complete desired service configuration to store")] + data: ReplacePublisherServiceConfigurationInput, + ) -> FieldResult { + replace_service_configuration(context, &data).map_err(IntoFieldError::into_field_error) + } + #[graphql(description = "Create a new imprint with the specified values")] fn create_imprint( context: &Context, diff --git a/thoth-api/src/graphql/query.rs b/thoth-api/src/graphql/query.rs index f664b443..fa23d82d 100644 --- a/thoth-api/src/graphql/query.rs +++ b/thoth-api/src/graphql/query.rs @@ -28,8 +28,11 @@ use crate::model::{ location::{Location, LocationOrderBy, LocationPlatform}, price::{CurrencyCode, Price}, publication::{Publication, PublicationOrderBy, PublicationType}, - publisher::{Publisher, PublisherOrderBy}, + publisher::{Publisher, PublisherOrderBy, ThothPackage}, publisher_distribution_platform::{DistributionPlatform, DistributionPlatformOption}, + publisher_service_configuration::{ + PublisherServiceConfiguration, PublisherServiceConfigurationSummary, + }, r#abstract::{Abstract, AbstractOrderBy}, reference::{Reference, ReferenceOrderBy}, series::{Series, SeriesOrderBy, SeriesType}, @@ -40,10 +43,31 @@ use crate::model::{ Crud, Doi, }; use crate::policy::PolicyContext; -use thoth_errors::ThothError; +use juniper::IntoFieldError; +use thoth_errors::{ThothError, ThothResult}; pub struct QueryRoot; +/// Load one publisher's protected service configuration under the section 11.1 +/// read matrix. +/// +/// Anonymous callers are rejected **before** any publisher load. For an +/// authenticated caller the publisher is loaded first, so an unknown +/// `publisherId` returns `EntityNotFound`; publisher existence is already public +/// through the anonymous `publisher`/`publishers` queries, so this discloses +/// nothing new. The role check is then exactly +/// `PolicyContext::require_publisher_for`: superuser, or `PUBLISHER_USER` for +/// that publisher's organisation. There is **one** authorization decision for +/// the whole type, including `effectiveCapabilities`. +fn load_protected_configuration( + context: &Context, + publisher_id: Uuid, +) -> ThothResult { + let publisher: Publisher = context.load_current(&publisher_id)?; + context.require_publisher_for(&publisher)?; + Ok(PublisherServiceConfiguration::new(publisher)) +} + #[juniper::graphql_object(Context = Context)] impl QueryRoot { #[allow(clippy::too_many_arguments)] @@ -633,6 +657,95 @@ impl QueryRoot { Publisher::count_by_distribution_platform(&context.db, platform).map_err(Into::into) } + #[graphql( + description = "Query the protected desired service configuration of one publisher. Readable only by a superuser or by a PUBLISHER_USER of that publisher" + )] + fn publisher_service_configuration( + context: &Context, + #[graphql(description = "Thoth publisher ID to search on")] publisher_id: Uuid, + ) -> FieldResult { + load_protected_configuration(context, publisher_id) + .map_err(IntoFieldError::into_field_error) + } + + #[graphql( + description = "Query the protected desired service configuration of every publisher, with the metadata of its latest recorded change. Superuser only" + )] + fn publisher_service_configurations( + context: &Context, + #[graphql( + default = vec![], + description = "If set, only shows results for publishers with these IDs" + )] + publishers: Option>, + #[graphql( + default = vec![], + description = "If set, only shows results for publishers with these subscription packages" + )] + packages: Option>, + #[graphql( + default = vec![], + description = "If set, only shows results for publishers that have every one of these distribution platforms enabled. Multiple values narrow the results rather than widening them" + )] + enabled_platforms: Option>, + #[graphql(default = 100, description = "The number of items to return")] limit: Option, + #[graphql(default = 0, description = "The number of items to skip")] offset: Option, + #[graphql( + default = PublisherOrderBy::default(), + description = "The order in which to sort the results. Results are always additionally sorted by publisher ID ascending, so pagination is deterministic" + )] + order: Option, + ) -> FieldResult> { + context + .require_superuser() + .and_then(|_| { + PublisherServiceConfiguration::all_summaries( + &context.db, + limit.unwrap_or_default(), + offset.unwrap_or_default(), + order.unwrap_or_default(), + publishers.unwrap_or_default(), + packages.unwrap_or_default(), + enabled_platforms.unwrap_or_default(), + ) + }) + .map_err(IntoFieldError::into_field_error) + } + + #[graphql( + description = "Get the total number of publishers matching a protected service configuration report filter. Superuser only" + )] + fn publisher_service_configuration_count( + context: &Context, + #[graphql( + default = vec![], + description = "If set, only counts publishers with these IDs" + )] + publishers: Option>, + #[graphql( + default = vec![], + description = "If set, only counts publishers with these subscription packages" + )] + packages: Option>, + #[graphql( + default = vec![], + description = "If set, only counts publishers that have every one of these distribution platforms enabled. Multiple values narrow the results rather than widening them" + )] + enabled_platforms: Option>, + ) -> FieldResult { + context + .require_superuser() + .and_then(|_| { + PublisherServiceConfiguration::count( + &context.db, + publishers.unwrap_or_default(), + packages.unwrap_or_default(), + enabled_platforms.unwrap_or_default(), + ) + }) + .map_err(IntoFieldError::into_field_error) + } + #[graphql(description = "Query the full list of imprints")] fn imprints( context: &Context, diff --git a/thoth-api/src/graphql/sdl_support.rs b/thoth-api/src/graphql/sdl_support.rs new file mode 100644 index 00000000..64ca39d2 --- /dev/null +++ b/thoth-api/src/graphql/sdl_support.rs @@ -0,0 +1,169 @@ +//! Shared SDL extraction for the generated-schema guards. +//! +//! The `BE-02` and `BE-03` guards assert that a given generated type exposes no +//! forbidden field. Those assertions are only as strong as the block they +//! inspect, so the extraction below is the security-relevant part of them: a +//! guard that silently inspects half a type body still passes while the field it +//! was written to catch sits in the half it never looked at. + +/// The complete body of one SDL type, input or enum declaration. +/// +/// `declaration` must include the opening `{` (for example `"type Publisher {"`). +/// The returned slice is everything between that brace and its **matching** +/// closing brace. +/// +/// Extraction is brace-balanced and string-aware. Both properties are required +/// by the real generated schema: +/// +/// - a field argument may carry a nested object default, as +/// `Publisher.imprints` does with +/// `order: ImprintOrderBy = {direction: "ASC", field: "IMPRINT_NAME"}`; +/// - a description may contain a brace, as the `doi` description does with +/// `\d{4,9}`, or an escaped quote, as the `Timestamp` description does; +/// - a description may be a `"""` block string, as `Imprint.crossmarkDoi` is. +/// +/// A naive `split_once('}')` stops at the first closing brace of any kind. On +/// `type Publisher` that is the brace closing the `imprints` order default, so +/// everything declared after `imprints` — `contacts` and +/// `distributionPlatforms` — would never be inspected, and a protected field +/// added there would pass the guard unnoticed. +pub(crate) fn sdl_block<'a>(sdl: &'a str, declaration: &str) -> &'a str { + let body = sdl + .split_once(declaration) + .unwrap_or_else(|| panic!("SDL must declare `{declaration}`")) + .1; + + let bytes = body.as_bytes(); + let mut index = 0; + // The declaration consumed the opening brace, so the body starts one deep. + let mut depth = 1usize; + + while index < bytes.len() { + // Only ASCII bytes are matched below, and every byte of a multi-byte + // UTF-8 sequence is >= 0x80, so `index` is always a char boundary. + if bytes[index..].starts_with(br#"""""#) { + index = skip_block_string(bytes, index); + } else if bytes[index] == b'"' { + index = skip_quoted_string(bytes, index); + } else { + match bytes[index] { + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return &body[..index]; + } + } + _ => {} + } + index += 1; + } + } + + panic!("`{declaration}` body is not brace-balanced") +} + +/// The index just past the `"""` block string opening at `start`. +fn skip_block_string(bytes: &[u8], start: usize) -> usize { + let mut index = start + 3; + while index < bytes.len() { + if bytes[index..].starts_with(br#"""""#) { + return index + 3; + } + index += 1; + } + bytes.len() +} + +/// The index just past the `"` string opening at `start`, honouring `\` escapes. +fn skip_quoted_string(bytes: &[u8], start: usize) -> usize { + let mut index = start + 1; + while index < bytes.len() { + match bytes[index] { + b'\\' => index += 2, + b'"' => return index + 1, + _ => index += 1, + } + } + bytes.len() +} + +#[cfg(test)] +mod tests { + use super::sdl_block; + use crate::graphql::create_schema; + + #[test] + fn extraction_spans_a_nested_object_default() { + let sdl = "type Publisher {\n imprints(order: X = {direction: \"ASC\"}): [Imprint!]!\n \ + distributionPlatforms: [A!]!\n}\n\ntype Next {\n other: Int!\n}\n"; + let block = sdl_block(sdl, "type Publisher {"); + + assert!(block.contains("imprints(")); + assert!( + block.contains("distributionPlatforms"), + "extraction truncated at the nested default: {block}" + ); + // The following declaration must not bleed in. + assert!(!block.contains("other: Int!")); + } + + #[test] + fn extraction_ignores_braces_and_quotes_inside_descriptions() { + let sdl = "type T {\n \"Expressed as `\\\\d{4,9}` and \\\"quoted\\\"\"\n a: Int!\n \ + \"\"\"\n A block } description {\n \"\"\"\n b: Int!\n}\n"; + let block = sdl_block(sdl, "type T {"); + + assert!(block.contains("a: Int!")); + assert!( + block.contains("b: Int!"), + "a brace inside a description truncated the body: {block}" + ); + } + + #[test] + fn extraction_covers_the_whole_real_publisher_declaration() { + let sdl = create_schema().as_sdl(); + let block = sdl_block(&sdl, "type Publisher {"); + + // `distributionPlatforms` is declared after `imprints`, whose nested + // order default is exactly what truncated the previous extraction. + for post_imprints_sentinel in ["contacts(", "distributionPlatforms:"] { + assert!( + block.contains(post_imprints_sentinel), + "extraction missed `{post_imprints_sentinel}`, declared after `imprints`: {block}" + ); + } + // The block stops at its own closing brace. + assert!(!block.contains("type PublisherContext")); + } + + #[test] + fn a_forbidden_field_inserted_after_imprints_is_caught() { + // The regression the previous extraction could not catch: a protected + // field smuggled in *after* the nested order default. + let sdl = create_schema().as_sdl(); + let (head, tail) = sdl + .split_once(" \"Get contacts linked to this publisher\"") + .expect("the real Publisher type declares `contacts` after `imprints`"); + let tampered = format!("{head} subscriptionPackage: ThothPackage!\n \"Get contacts linked to this publisher\"{tail}"); + + let block = sdl_block(&tampered, "type Publisher {"); + assert!( + block.contains("subscriptionPackage"), + "the guard would not see a protected field inserted after `imprints`" + ); + } + + #[test] + #[should_panic(expected = "not brace-balanced")] + fn an_unbalanced_declaration_fails_loudly() { + sdl_block("type Broken {\n a: Int!\n", "type Broken {"); + } + + #[test] + #[should_panic(expected = "SDL must declare")] + fn a_missing_declaration_fails_loudly() { + sdl_block("type Other {\n}\n", "type Absent {"); + } +} diff --git a/thoth-api/src/graphql/service_configuration_tests.rs b/thoth-api/src/graphql/service_configuration_tests.rs new file mode 100644 index 00000000..932c7425 --- /dev/null +++ b/thoth-api/src/graphql/service_configuration_tests.rs @@ -0,0 +1,1298 @@ +//! `BE-03` protected-surface authorization, capability-exposure, error-shape +//! and query-count evidence. +//! +//! These tests exercise the real production schema, the real resolvers and the +//! real `RequestLoaders` bundle against a disposable database. Coordinator, +//! audit, concurrency, linked-platform and trigger-cascade evidence lives in +//! `crate::model::publisher_service_configuration::tests`. + +#![cfg(all(test, feature = "backend"))] + +use std::collections::HashMap; +use std::sync::Arc; + +use diesel::{sql_query, RunQueryDsl}; +use serde_json::{json, Value as JsonValue}; +use uuid::Uuid; +use zitadel::actix::introspection::IntrospectedUser; + +use super::dataloader::fixture::{BatchStats, SqlProbe}; +use super::dataloader::RequestLoaders; +use super::{create_schema, Context, GraphQLRequest, Schema}; +use crate::db::PgPool; +use crate::model::publisher::{Publisher, PublisherCapability, ThothPackage}; +use crate::model::publisher_distribution_platform::DistributionPlatform; +use crate::model::publisher_service_configuration::crud::replace_publisher_service_configuration; +use crate::model::publisher_service_configuration::{ + PublisherServiceConfigurationSource, ReplacePublisherServiceConfigurationInput, + ServiceConfigurationWriteContext, +}; +use crate::model::tests::db as test_db; +use crate::model::{Crud, Timestamp}; +use crate::policy::Role; + +// -------------------------------------------------------------------------- +// Execution helpers +// -------------------------------------------------------------------------- + +fn request(query: &str) -> GraphQLRequest { + serde_json::from_value(json!({ "query": query })).expect("build GraphQL request") +} + +async fn run(schema: &Schema, context: &Context, query: &str) -> JsonValue { + serde_json::to_value(request(query).execute(schema, context).await) + .expect("serialize GraphQL response") +} + +fn data<'a>(response: &'a JsonValue, field: &str) -> &'a JsonValue { + assert!( + response.get("errors").is_none() + || response["errors"].as_array().is_some_and(Vec::is_empty), + "unexpected GraphQL errors: {response}" + ); + &response["data"][field] +} + +/// The single error object of a denied or failed response, with its machine +/// readable `extensions.type`. +fn only_error(response: &JsonValue) -> (String, String) { + let errors = response["errors"].as_array().expect("errors array"); + assert_eq!(errors.len(), 1, "expected exactly one error: {response}"); + ( + errors[0]["message"].as_str().expect("message").to_string(), + errors[0]["extensions"]["type"] + .as_str() + .expect("extensions.type") + .to_string(), + ) +} + +fn assert_unauthorized(response: &JsonValue) { + let (message, kind) = only_error(response); + assert_eq!(kind, "NO_ACCESS", "expected a fail-closed denial"); + assert_eq!(message, "Unauthorized"); +} + +/// A user holding one scoped role for several organisations, or several scoped +/// roles for one organisation. +fn user_with(user_id: &str, roles: &[(Role, &str)]) -> IntrospectedUser { + let mut project_roles: HashMap> = HashMap::new(); + for (role, org_id) in roles { + project_roles + .entry(role.as_ref().to_string()) + .or_default() + .insert((*org_id).to_string(), "role".to_string()); + } + IntrospectedUser { + user_id: user_id.to_string(), + username: None, + name: None, + given_name: None, + family_name: None, + preferred_username: None, + email: None, + email_verified: None, + locale: None, + project_roles: Some(project_roles), + metadata: None, + } +} + +fn org_of(publisher: &Publisher) -> String { + publisher.zitadel_id.clone().expect("publisher zitadel id") +} + +fn read_query(publisher_id: Uuid) -> String { + format!( + "{{ publisherServiceConfiguration(publisherId: \"{publisher_id}\") \ + {{ publisher {{ publisherId }} subscriptionPackage effectiveCapabilities \ + enabledDistributionPlatforms {{ platform }} updatedAt }} }}" + ) +} + +fn mutation( + publisher_id: Uuid, + package: &str, + platforms: &str, + expected_updated_at: &Timestamp, +) -> String { + format!( + "mutation {{ replacePublisherServiceConfiguration(data: {{ \ + publisherId: \"{publisher_id}\", subscriptionPackage: {package}, \ + enabledDistributionPlatforms: [{platforms}], \ + expectedUpdatedAt: \"{}\" }}) \ + {{ subscriptionPackage effectiveCapabilities updatedAt \ + enabledDistributionPlatforms {{ platform }} }} }}", + expected_updated_at.to_rfc3339() + ) +} + +fn token(pool: &PgPool, publisher_id: Uuid) -> Timestamp { + Publisher::from_id(pool, &publisher_id) + .expect("publisher") + .service_configuration_updated_at +} + +/// Commit a configuration change through the canonical coordinator, so a +/// GraphQL-level test can establish a fixture without asserting the write path +/// twice. +fn seed_configuration( + pool: &PgPool, + publisher_id: Uuid, + package: ThothPackage, + platforms: &[DistributionPlatform], +) { + replace_publisher_service_configuration( + pool, + &ServiceConfigurationWriteContext { + source: PublisherServiceConfigurationSource::SuperuserApi, + actor: "fixture-superuser", + }, + &ReplacePublisherServiceConfigurationInput { + publisher_id, + subscription_package: package, + enabled_distribution_platforms: platforms.to_vec(), + expected_updated_at: token(pool, publisher_id), + }, + ) + .expect("seed configuration"); +} + +fn capabilities_of(value: &JsonValue) -> Vec { + value["effectiveCapabilities"] + .as_array() + .expect("capability array") + .iter() + .map(|code| code.as_str().expect("capability code").to_string()) + .collect() +} + +fn expected_capabilities(package: ThothPackage) -> Vec { + package + .capabilities() + .iter() + .map(PublisherCapability::to_string) + .collect() +} + +// -------------------------------------------------------------------------- +// Read authorization: every row of specification section 11.1 +// -------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn anonymous_callers_cannot_read_the_protected_configuration() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + seed_configuration( + &pool, + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen], + ); + let schema = create_schema(); + let context = test_db::test_context_anonymous(Arc::clone(&pool)); + + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert_unauthorized(&response); + + // Rejected before any publisher load: an unknown publisher is equally + // unauthorized for an anonymous caller, never `EntityNotFound`. + let unknown = run(&schema, &context, &read_query(Uuid::new_v4())).await; + assert_unauthorized(&unknown); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn an_authenticated_caller_with_no_applicable_role_cannot_read_it() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let schema = create_schema(); + let context = test_db::test_context(Arc::clone(&pool), "no-roles"); + + assert_unauthorized(&run(&schema, &context, &read_query(publisher.publisher_id)).await); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_publisher_user_of_the_target_publisher_reads_it_including_its_capabilities() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + seed_configuration( + &pool, + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Oapen], + ); + let schema = create_schema(); + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with("owner", &[(Role::PublisherUser, &org_of(&publisher))]), + ); + + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + let configuration = data(&response, "publisherServiceConfiguration"); + assert_eq!(configuration["subscriptionPackage"], "OBELISK"); + assert_eq!( + capabilities_of(configuration), + expected_capabilities(ThothPackage::Obelisk) + ); + assert_eq!( + configuration["enabledDistributionPlatforms"] + .as_array() + .expect("platforms") + .len(), + 2 + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_publisher_user_of_another_publisher_cannot_read_it() { + let (_guard, pool) = test_db::setup_test_db(); + let target = test_db::create_publisher(&pool); + let other = test_db::create_publisher(&pool); + seed_configuration( + &pool, + target.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen], + ); + let schema = create_schema(); + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with("outsider", &[(Role::PublisherUser, &org_of(&other))]), + ); + + let response = run(&schema, &context, &read_query(target.publisher_id)).await; + assert_unauthorized(&response); + // Specifically: the other publisher's capability codes are not readable. + assert!(response["data"]["publisherServiceConfiguration"].is_null()); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn no_other_scoped_role_implies_publisher_user_for_the_protected_read() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let org = org_of(&publisher); + let schema = create_schema(); + + // PUBLISHER_ADMIN, WORK_LIFECYCLE and CDN_WRITE for the target publisher, + // each without PUBLISHER_USER, alone and in combination. + let denied = [ + vec![(Role::PublisherAdmin, org.as_str())], + vec![(Role::WorkLifecycle, org.as_str())], + vec![(Role::CdnWrite, org.as_str())], + vec![ + (Role::PublisherAdmin, org.as_str()), + (Role::WorkLifecycle, org.as_str()), + (Role::CdnWrite, org.as_str()), + ], + ]; + for roles in denied { + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with("scoped-but-not-publisher-user", &roles), + ); + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert_unauthorized(&response); + } + + // Adding PUBLISHER_USER, and only that, opens the read. + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with( + "publisher-user", + &[ + (Role::PublisherAdmin, org.as_str()), + (Role::PublisherUser, org.as_str()), + ], + ), + ); + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert!(!data(&response, "publisherServiceConfiguration").is_null()); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_superuser_reads_any_publishers_configuration_and_capabilities() { + let (_guard, pool) = test_db::setup_test_db(); + let first = test_db::create_publisher(&pool); + let second = test_db::create_publisher(&pool); + seed_configuration(&pool, first.publisher_id, ThothPackage::Pyramid, &[]); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-read")); + + for (publisher_id, package) in [ + (first.publisher_id, ThothPackage::Pyramid), + (second.publisher_id, ThothPackage::Oasis), + ] { + let response = run(&schema, &context, &read_query(publisher_id)).await; + let configuration = data(&response, "publisherServiceConfiguration"); + assert_eq!( + capabilities_of(configuration), + expected_capabilities(package) + ); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn an_account_scoped_to_two_publishers_reads_both_and_no_third() { + let (_guard, pool) = test_db::setup_test_db(); + let first = test_db::create_publisher(&pool); + let second = test_db::create_publisher(&pool); + let third = test_db::create_publisher(&pool); + let schema = create_schema(); + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with( + "multi-publisher", + &[ + (Role::PublisherUser, &org_of(&first)), + (Role::PublisherUser, &org_of(&second)), + ], + ), + ); + + for publisher_id in [first.publisher_id, second.publisher_id] { + let response = run(&schema, &context, &read_query(publisher_id)).await; + assert!(!data(&response, "publisherServiceConfiguration").is_null()); + } + assert_unauthorized(&run(&schema, &context, &read_query(third.publisher_id)).await); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_publisher_with_a_null_zitadel_id_fails_closed_for_every_non_superuser() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let linked = test_db::create_publisher(&pool); + { + let mut connection = pool.get().expect("connection"); + sql_query(format!( + "UPDATE publisher SET zitadel_id = NULL WHERE publisher_id = '{}'", + publisher.publisher_id + )) + .execute(&mut connection) + .expect("unlink publisher"); + } + let schema = create_schema(); + + // A caller holding PUBLISHER_USER for a real organisation still cannot read + // an unlinked publisher, because the publisher resolves to no organisation. + let context = test_db::test_context_with_user( + Arc::clone(&pool), + user_with( + "linked-elsewhere", + &[(Role::PublisherUser, &org_of(&linked))], + ), + ); + assert_unauthorized(&run(&schema, &context, &read_query(publisher.publisher_id)).await); + + let superuser = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-null")); + let response = run(&schema, &superuser, &read_query(publisher.publisher_id)).await; + assert!(!data(&response, "publisherServiceConfiguration").is_null()); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn an_unknown_publisher_is_not_found_for_an_authenticated_caller() { + let (_guard, pool) = test_db::setup_test_db(); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-missing")); + + let response = run(&schema, &context, &read_query(Uuid::new_v4())).await; + let (message, kind) = only_error(&response); + assert_eq!(kind, "INTERNAL_ERROR", "EntityNotFound keeps its mapping"); + assert_eq!(message, "No record was found for the given ID."); +} + +// -------------------------------------------------------------------------- +// Write and report authorization +// -------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn only_a_superuser_may_replace_the_configuration() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let other = test_db::create_publisher(&pool); + let org = org_of(&publisher); + let schema = create_schema(); + let current = token(&pool, publisher.publisher_id); + let query = mutation(publisher.publisher_id, "SPHINX", "ZENODO", ¤t); + + let denied_contexts = vec![ + test_db::test_context_anonymous(Arc::clone(&pool)), + test_db::test_context(Arc::clone(&pool), "authenticated-no-roles"), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("owner", &[(Role::PublisherUser, org.as_str())]), + ), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("admin", &[(Role::PublisherAdmin, org.as_str())]), + ), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("lifecycle", &[(Role::WorkLifecycle, org.as_str())]), + ), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("cdn", &[(Role::CdnWrite, org.as_str())]), + ), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("other-publisher", &[(Role::PublisherUser, &org_of(&other))]), + ), + ]; + for context in &denied_contexts { + assert_unauthorized(&run(&schema, context, &query).await); + } + // Every denial was decided before the database was touched. + assert_eq!(token(&pool, publisher.publisher_id), current); + + let superuser = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-write")); + let response = run(&schema, &superuser, &query).await; + let configuration = data(&response, "replacePublisherServiceConfiguration"); + assert_eq!(configuration["subscriptionPackage"], "SPHINX"); + assert!(token(&pool, publisher.publisher_id) > current); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn the_staff_report_and_its_count_are_superuser_only() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let other = test_db::create_publisher(&pool); + let schema = create_schema(); + + let report = "{ publisherServiceConfigurations { configuration { subscriptionPackage } \ + lastChange { actor source changedAt } } }"; + let count = "{ publisherServiceConfigurationCount }"; + + let denied = vec![ + test_db::test_context_anonymous(Arc::clone(&pool)), + test_db::test_context(Arc::clone(&pool), "authenticated"), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("owner", &[(Role::PublisherUser, &org_of(&publisher))]), + ), + test_db::test_context_with_user( + Arc::clone(&pool), + user_with("other", &[(Role::PublisherUser, &org_of(&other))]), + ), + ]; + for context in &denied { + assert_unauthorized(&run(&schema, context, report).await); + assert_unauthorized(&run(&schema, context, count).await); + } + + let superuser = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-report")); + assert_eq!( + data( + &run(&schema, &superuser, report).await, + "publisherServiceConfigurations" + ) + .as_array() + .expect("report") + .len(), + 2 + ); + assert_eq!( + data( + &run(&schema, &superuser, count).await, + "publisherServiceConfigurationCount" + ), + 2 + ); +} + +// -------------------------------------------------------------------------- +// Effective capabilities +// -------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn every_package_reports_exactly_its_canonical_capability_sequence() { + let (_guard, pool) = test_db::setup_test_db(); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-caps")); + + for package in [ + ThothPackage::Oasis, + ThothPackage::Obelisk, + ThothPackage::Sphinx, + ThothPackage::Pyramid, + ] { + let publisher = test_db::create_publisher(&pool); + seed_configuration(&pool, publisher.publisher_id, package, &[]); + + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + let configuration = data(&response, "publisherServiceConfiguration"); + + // Exact sequence, not set equality: a later sort, dedup or reorder + // fails this. + assert_eq!( + capabilities_of(configuration), + expected_capabilities(package), + "capabilities for {package}" + ); + // The package reported in the same response agrees with them, because + // both are read from the same publisher row. + assert_eq!(configuration["subscriptionPackage"], package.to_string()); + + // Repeated reads return an identical sequence. + let again = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert_eq!( + capabilities_of(data(&again, "publisherServiceConfiguration")), + capabilities_of(configuration) + ); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn oasis_reports_an_empty_capability_list_never_null() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-oasis")); + + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + let configuration = data(&response, "publisherServiceConfiguration"); + assert_eq!(configuration["subscriptionPackage"], "OASIS"); + assert!(configuration["effectiveCapabilities"].is_array()); + assert!(capabilities_of(configuration).is_empty()); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_package_upgrade_and_downgrade_change_capabilities_with_no_separate_write() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-upgrade")); + + // Upgrade OASIS -> SPHINX. + let upgrade = run( + &schema, + &context, + &mutation( + publisher.publisher_id, + "SPHINX", + "", + &token(&pool, publisher.publisher_id), + ), + ) + .await; + let returned = data(&upgrade, "replacePublisherServiceConfiguration"); + assert_eq!( + capabilities_of(returned), + expected_capabilities(ThothPackage::Sphinx), + "the mutation's own returned configuration reflects the new package" + ); + let queried = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert_eq!( + capabilities_of(data(&queried, "publisherServiceConfiguration")), + expected_capabilities(ThothPackage::Sphinx) + ); + + // Downgrade SPHINX -> OBELISK. + let downgrade = run( + &schema, + &context, + &mutation( + publisher.publisher_id, + "OBELISK", + "", + &token(&pool, publisher.publisher_id), + ), + ) + .await; + assert_eq!( + capabilities_of(data(&downgrade, "replacePublisherServiceConfiguration")), + expected_capabilities(ThothPackage::Obelisk) + ); + let queried = run(&schema, &context, &read_query(publisher.publisher_id)).await; + assert_eq!( + capabilities_of(data(&queried, "publisherServiceConfiguration")), + expected_capabilities(ThothPackage::Obelisk) + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_platform_only_change_leaves_effective_capabilities_unchanged() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + seed_configuration(&pool, publisher.publisher_id, ThothPackage::Obelisk, &[]); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-platform")); + + let response = run( + &schema, + &context, + &mutation( + publisher.publisher_id, + "OBELISK", + "OAPEN", + &token(&pool, publisher.publisher_id), + ), + ) + .await; + let returned = data(&response, "replacePublisherServiceConfiguration"); + assert_eq!( + capabilities_of(returned), + expected_capabilities(ThothPackage::Obelisk) + ); + assert_eq!( + returned["enabledDistributionPlatforms"] + .as_array() + .expect("platforms") + .len(), + 2 + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn no_anonymous_operation_can_select_a_capability_or_package_value() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + seed_configuration(&pool, publisher.publisher_id, ThothPackage::Pyramid, &[]); + let schema = create_schema(); + let anonymous = test_db::test_context_anonymous(Arc::clone(&pool)); + + // Schema validation rejects the fields outright on the public type. + for query in [ + "{ publishers(limit: 10) { publisherId effectiveCapabilities } }", + "{ publishers(limit: 10) { publisherId subscriptionPackage } }", + "{ publisher(publisherId: \"00000000-0000-0000-0000-000000000001\") { capabilities } }", + ] { + let response = run(&schema, &anonymous, query).await; + assert!( + response.get("errors").is_some(), + "public Publisher must not resolve `{query}`" + ); + } + + // And the protected operation itself is denied. + assert_unauthorized(&run(&schema, &anonymous, &read_query(publisher.publisher_id)).await); +} + +// -------------------------------------------------------------------------- +// Error shape +// -------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_stale_replacement_returns_the_distinct_machine_readable_error() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let stale = token(&pool, publisher.publisher_id); + seed_configuration( + &pool, + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + ); + let committed = token(&pool, publisher.publisher_id); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-stale")); + + let response = run( + &schema, + &context, + &mutation(publisher.publisher_id, "SPHINX", "CROSSREF", &stale), + ) + .await; + + let (message, kind) = only_error(&response); + assert_eq!(kind, "STALE_SERVICE_CONFIGURATION"); + assert!(message.contains("changed since it was read")); + // The current token is deliberately not disclosed to a caller that just + // failed a version check. + assert!(!message.contains(&committed.to_rfc3339())); + // No SQL, table name, column name or driver text. + for leak in [ + "SELECT", + "UPDATE", + "service_configuration_updated_at", + "publisher_service_configuration_history", + "publisher_distribution_platform", + "ERROR:", + "expected_updated_at", + ] { + assert!( + !message.contains(leak), + "the stale message must not leak `{leak}`: {message}" + ); + } + assert_eq!(token(&pool, publisher.publisher_id), committed); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_non_assignable_platform_keeps_its_merged_error_mapping() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let schema = create_schema(); + let context = + test_db::test_context_with_user(Arc::clone(&pool), test_db::test_superuser("su-jisc")); + + let response = run( + &schema, + &context, + &mutation( + publisher.publisher_id, + "OASIS", + "JISC_NBK", + &token(&pool, publisher.publisher_id), + ), + ) + .await; + + let (message, kind) = only_error(&response); + assert_eq!(kind, "INTERNAL_ERROR", "unchanged BE-02 mapping"); + assert!(message.contains("JISC_NBK")); +} + +// -------------------------------------------------------------------------- +// Query efficiency (specification sections 12.3 and 18.8) +// -------------------------------------------------------------------------- + +/// The assignment statements issued by the DataLoader, as opposed to the +/// report's own publisher-page and latest-change SQL. +fn assignment_statements(captured: &[String]) -> Vec { + captured + .iter() + .filter(|sql| sql.contains("FROM \"publisher_distribution_platform\"")) + .cloned() + .collect() +} + +fn history_statements(captured: &[String]) -> Vec { + captured + .iter() + .filter(|sql| sql.contains("FROM \"publisher_service_configuration_history\"")) + .cloned() + .collect() +} + +fn publisher_page_statements(captured: &[String]) -> Vec { + captured + .iter() + .filter(|sql| sql.contains("FROM \"publisher\"") && sql.contains("LIMIT")) + .cloned() + .collect() +} + +/// The application-issued `UPDATE` statements against the `publisher` table. +/// +/// The publisher row carries the shared `AFTER UPDATE` work-freshness trigger, +/// so this count **is** the number of times that trigger's set-based cascade +/// runs over the publisher's whole catalogue. The trigger's own +/// `UPDATE work ... FROM imprint` is server-side and never appears here, which +/// is why the application-level count is the thing worth asserting. +fn publisher_update_statements(captured: &[String]) -> Vec { + captured + .iter() + .filter(|sql| sql.contains("UPDATE \"publisher\"")) + .cloned() + .collect() +} + +fn seed_publishers_with_assignment(pool: &PgPool, count: usize) { + let mut connection = pool.get().expect("connection"); + sql_query(format!( + "INSERT INTO publisher (publisher_id, publisher_name) \ + SELECT gen_random_uuid(), 'Report Press ' || lpad(i::text, 5, '0') \ + FROM generate_series(1, {count}) AS i" + )) + .execute(&mut connection) + .expect("seed publishers"); + sql_query( + "INSERT INTO publisher_distribution_platform \ + (publisher_id, platform, enabled, activation_id, enabled_at) \ + SELECT publisher_id, 'OAPEN', true, gen_random_uuid(), now() FROM publisher", + ) + .execute(&mut connection) + .expect("seed assignments"); +} + +async fn measure_report(page_size: usize) -> (Vec, Vec) { + let (_guard, ordinary_pool) = test_db::setup_test_db(); + seed_publishers_with_assignment(&ordinary_pool, page_size); + + let probe = SqlProbe::install(&test_db::test_db_url()); + let stats = Arc::new(BatchStats::default()); + let mut context = test_db::test_context_with_user( + Arc::clone(&probe.pool), + test_db::test_superuser("su-measure"), + ); + context.loaders = + RequestLoaders::for_request_observed(Arc::clone(&probe.pool), Arc::clone(&stats)); + let schema = create_schema(); + + probe.start(); + let response = run( + &schema, + &context, + &format!( + "{{ publisherServiceConfigurations(limit: {page_size}) {{ \ + configuration {{ subscriptionPackage effectiveCapabilities updatedAt \ + enabledDistributionPlatforms {{ platform }} }} \ + lastChange {{ actor source }} }} }}" + ), + ) + .await; + let captured = probe.captured_statements(); + + let rows = data(&response, "publisherServiceConfigurations") + .as_array() + .expect("report") + .clone(); + assert_eq!(rows.len(), page_size); + for row in &rows { + assert_eq!( + row["configuration"]["enabledDistributionPlatforms"] + .as_array() + .expect("platforms") + .len(), + 1, + "every summary loads its own assignments" + ); + assert!(row["lastChange"].is_null()); + } + + (stats.batch_sizes(), captured) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn the_report_statement_count_is_bounded_and_does_not_grow_with_the_page() { + // The report always issues two set-based statements — one publisher page, + // one latest-change — whatever the page size. The existing BE-02 assignment + // loader adds `ceil(N / MAX_BATCH_SIZE)` set-based dispatches, because it is + // configured with a maximum batch size and chunks larger key sets. Nothing + // is per-publisher in either part. + // + // At 1, 25 and 200 the page fits one loader chunk, so the whole request is + // three statements. 201 is the first page size that does not, and is + // asserted separately below. + for page_size in [1, 25, 200] { + assert!(page_size <= crate::graphql::dataloader::MAX_BATCH_SIZE); + let (chunks, captured) = measure_report(page_size).await; + + assert_eq!( + publisher_page_statements(&captured).len(), + 1, + "one set-based publisher-page statement at page size {page_size}" + ); + assert_eq!( + history_statements(&captured).len(), + 1, + "one set-based latest-change statement at page size {page_size}" + ); + let history = &history_statements(&captured)[0]; + assert!(history.contains("DISTINCT ON"), "{history}"); + assert!(history.contains("= ANY"), "{history}"); + + let assignment_sql = assignment_statements(&captured); + assert_eq!( + assignment_sql.len(), + 1, + "one set-based assignment statement at page size {page_size}: {assignment_sql:?}" + ); + assert!(assignment_sql[0].contains("= ANY")); + assert_eq!(chunks, vec![page_size], "one dispatch chunk"); + } +} + +/// The first page size that exceeds one loader batch. +/// +/// The accurate statement shape is **two** report statements plus +/// `ceil(page publisher count / MAX_BATCH_SIZE)` assignment-loader dispatches — +/// not a count that is flatly independent of N. At 201 that is `[200, 1]` and +/// therefore two assignment statements, still with no per-publisher SQL loop. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn the_assignment_loader_chunks_a_page_larger_than_the_maximum_batch() { + let max = crate::graphql::dataloader::MAX_BATCH_SIZE; + let page_size = max + 1; + let (chunks, captured) = measure_report(page_size).await; + + // The report's own two statements do not change. + assert_eq!(publisher_page_statements(&captured).len(), 1); + assert_eq!(history_statements(&captured).len(), 1); + + assert_eq!(chunks, vec![max, 1], "expected loader chunks [{max}, 1]"); + + let assignment_sql = assignment_statements(&captured); + let expected_dispatches = page_size.div_ceil(max); + assert_eq!(expected_dispatches, 2); + assert_eq!( + assignment_sql.len(), + expected_dispatches, + "expected ceil({page_size} / {max}) = {expected_dispatches} assignment statements: \ + {assignment_sql:?}" + ); + for sql in &assignment_sql { + assert!(sql.contains("= ANY"), "each dispatch is set-based: {sql}"); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn the_single_publisher_query_issues_one_assignment_statement() { + let (_guard, ordinary_pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&ordinary_pool); + seed_configuration( + &ordinary_pool, + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen], + ); + + let probe = SqlProbe::install(&test_db::test_db_url()); + let stats = Arc::new(BatchStats::default()); + let mut context = test_db::test_context_with_user( + Arc::clone(&probe.pool), + test_db::test_superuser("su-single"), + ); + context.loaders = + RequestLoaders::for_request_observed(Arc::clone(&probe.pool), Arc::clone(&stats)); + let schema = create_schema(); + + probe.start(); + let response = run(&schema, &context, &read_query(publisher.publisher_id)).await; + let captured = probe.captured_statements(); + + assert!(!data(&response, "publisherServiceConfiguration").is_null()); + assert_eq!(assignment_statements(&captured).len(), 1); + assert_eq!(stats.batch_sizes(), vec![1]); + // The protected read consults no configuration-history statement. + assert!(history_statements(&captured).is_empty()); +} + +/// Every committed configuration change costs **exactly one** publisher +/// `UPDATE`, and every uncommitted one costs zero (specification section 7.3 +/// steps 8 and 10). +/// +/// This is a cascade-amplification regression, not a style assertion. The +/// publisher row carries the shared `AFTER UPDATE` work-freshness trigger, so +/// each extra publisher `UPDATE` re-runs a set-based cascade across that +/// publisher's entire catalogue. A combined package-and-platform change must +/// therefore cost the same single cascade as a platform-only change. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn every_committed_change_issues_exactly_one_publisher_update() { + // (case, package, platforms, expected publisher UPDATEs) + let cases: [(&str, &str, &str, usize); 6] = [ + ("package-only", "SPHINX", "OAPEN, DOAB", 1), + ("platform-only", "OASIS", "OAPEN, DOAB, ZENODO", 1), + ("linked repair", "OASIS", "OAPEN, DOAB", 1), + ("combined package and platform", "PYRAMID", "ZENODO", 1), + ("true no-op", "OASIS", "OAPEN, DOAB", 0), + ("stale", "SPHINX", "ZENODO", 0), + ]; + + for (case, package, platforms, expected_updates) in cases { + let (_guard, ordinary_pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&ordinary_pool); + // Captured before the fixture commits, so it is a token this publisher + // genuinely once had and has since superseded. + let superseded = token(&ordinary_pool, publisher.publisher_id); + seed_configuration( + &ordinary_pool, + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + ); + + // The linked-repair case splits the OAPEN/DOAB group behind the + // coordinator's back, so the request is a membership no-op that the + // BE-02 primitive still repairs. + if case == "linked repair" { + let mut connection = ordinary_pool.get().expect("connection"); + sql_query(format!( + "UPDATE publisher_distribution_platform SET enabled = false, \ + disabled_at = now() WHERE publisher_id = '{}' AND platform = 'DOAB'", + publisher.publisher_id + )) + .execute(&mut connection) + .expect("split the linked group"); + } + + let probe = SqlProbe::install(&test_db::test_db_url()); + let context = test_db::test_context_with_user( + Arc::clone(&probe.pool), + test_db::test_superuser("su-update-count"), + ); + let schema = create_schema(); + + let supplied = if case == "stale" { + superseded + } else { + token(&probe.pool, publisher.publisher_id) + }; + + probe.start(); + let response = run( + &schema, + &context, + &mutation(publisher.publisher_id, package, platforms, &supplied), + ) + .await; + let captured = probe.captured_statements(); + + if case == "stale" { + let (_, kind) = only_error(&response); + assert_eq!(kind, "STALE_SERVICE_CONFIGURATION", "{case}"); + } else { + assert!( + !data(&response, "replacePublisherServiceConfiguration").is_null(), + "{case}" + ); + } + + let updates = publisher_update_statements(&captured); + assert_eq!( + updates.len(), + expected_updates, + "{case}: expected {expected_updates} publisher UPDATE(s), got {}: {updates:?}", + updates.len() + ); + + // No per-work application loop in any case. + let work_statements: Vec<&String> = captured + .iter() + .filter(|sql| sql.contains("FROM \"work\"") || sql.contains("UPDATE \"work\"")) + .collect(); + assert!( + work_statements.is_empty(), + "{case}: no application-level work statement may exist: {work_statements:?}" + ); + } +} + +/// Disposable-environment write-amplification and lock-footprint measurement +/// (specification section 18.4), driven through the real GraphQL mutation. +/// +/// This is **empirical evidence about the shape of the cost, not a production +/// SLA**. It is deliberately not extrapolated to production and no "safe" +/// catalogue size is derived from it. The measured numbers are printed so the +/// implementation report can quote them exactly; run with `--nocapture`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn catalogue_scale_write_amplification_is_measured_in_a_disposable_environment() { + use crate::schema::{imprint, work}; + use diesel::{ExpressionMethods, QueryDsl}; + use std::time::Instant; + + const TARGET_WORKS: usize = 2_000; + const CONTROL_WORKS: usize = 250; + + let (_guard, ordinary_pool) = test_db::setup_test_db(); + + // One target publisher with a materially larger catalogue spread across two + // imprints, plus an unrelated publisher's catalogue as a control. + let target = test_db::create_publisher(&ordinary_pool); + let first_imprint = test_db::create_imprint(&ordinary_pool, &target); + let second_imprint = test_db::create_imprint(&ordinary_pool, &target); + let template = test_db::create_work(&ordinary_pool, &first_imprint); + + let control_publisher = test_db::create_publisher(&ordinary_pool); + let control_imprint = test_db::create_imprint(&ordinary_pool, &control_publisher); + let control_template = test_db::create_work(&ordinary_pool, &control_imprint); + + let clone_works = |count: usize, imprints: Vec, template_id: Uuid| { + let mut connection = ordinary_pool.get().expect("connection"); + let cases: Vec = imprints + .iter() + .enumerate() + .map(|(index, imprint_id)| { + format!( + "WHEN i % {} = {index} THEN '{imprint_id}'::uuid", + imprints.len() + ) + }) + .collect(); + sql_query(format!( + "INSERT INTO work (work_type, work_status, edition, imprint_id) \ + SELECT t.work_type, t.work_status, t.edition, CASE {} END \ + FROM work t CROSS JOIN generate_series(1, {count}) AS i \ + WHERE t.work_id = '{template_id}'", + cases.join(" ") + )) + .execute(&mut connection) + .expect("clone works"); + }; + clone_works( + TARGET_WORKS - 1, + vec![first_imprint.imprint_id, second_imprint.imprint_id], + template.work_id, + ); + clone_works( + CONTROL_WORKS - 1, + vec![control_imprint.imprint_id], + control_template.work_id, + ); + + let count_works = |publisher_id: Uuid| -> i64 { + let mut connection = ordinary_pool.get().expect("connection"); + work::table + .inner_join(imprint::table) + .filter(imprint::publisher_id.eq(publisher_id)) + .count() + .get_result::(&mut connection) + .expect("work count") + }; + let max_freshness = |publisher_id: Uuid| -> Timestamp { + let mut connection = ordinary_pool.get().expect("connection"); + work::table + .inner_join(imprint::table) + .filter(imprint::publisher_id.eq(publisher_id)) + .select(work::updated_at_with_relations) + .order(work::updated_at_with_relations.desc()) + .first::(&mut connection) + .expect("max freshness") + }; + let moved_since = |publisher_id: Uuid, threshold: Timestamp| -> i64 { + let mut connection = ordinary_pool.get().expect("connection"); + work::table + .inner_join(imprint::table) + .filter(imprint::publisher_id.eq(publisher_id)) + .filter(work::updated_at_with_relations.gt(threshold)) + .count() + .get_result::(&mut connection) + .expect("moved count") + }; + + let target_work_count = count_works(target.publisher_id); + let control_work_count = count_works(control_publisher.publisher_id); + assert_eq!(target_work_count, TARGET_WORKS as i64); + assert_eq!(control_work_count, CONTROL_WORKS as i64); + let target_before = max_freshness(target.publisher_id); + let control_before = max_freshness(control_publisher.publisher_id); + + let probe = SqlProbe::install(&test_db::test_db_url()); + let context = test_db::test_context_with_user( + Arc::clone(&probe.pool), + test_db::test_superuser("su-catalogue"), + ); + let schema = create_schema(); + let current = token(&probe.pool, target.publisher_id); + + probe.start(); + let started = Instant::now(); + let response = run( + &schema, + &context, + &mutation(target.publisher_id, "SPHINX", "OAPEN, ZENODO", ¤t), + ) + .await; + let elapsed = started.elapsed(); + let captured = probe.captured_statements(); + assert!(!data(&response, "replacePublisherServiceConfiguration").is_null()); + + let target_moved = moved_since(target.publisher_id, target_before); + let control_moved = moved_since(control_publisher.publisher_id, control_before); + + // The publisher trigger issues one set-based `UPDATE work ... FROM imprint` + // per publisher row `UPDATE`. This request changes both the package and the + // platforms, and step 10 commits both in a **single** publisher `UPDATE`, so + // the cascade runs exactly once over the target's catalogue. Every target + // work is refreshed; no unrelated work is. + let publisher_updates = publisher_update_statements(&captured); + assert_eq!( + publisher_updates.len(), + 1, + "a combined package-and-platform change must issue exactly one publisher \ + UPDATE, so the work-freshness cascade runs once: {publisher_updates:?}" + ); + assert_eq!(target_moved, target_work_count); + assert_eq!(control_moved, 0); + assert_eq!( + max_freshness(control_publisher.publisher_id), + control_before + ); + + // No per-work application loop: the request issues no statement against the + // `work` table at all. The work rows are changed by the trigger's own + // set-based statement, which is server-side and never appears here. + let work_statements: Vec<&String> = captured + .iter() + .filter(|sql| sql.contains("FROM \"work\"") || sql.contains("UPDATE \"work\"")) + .collect(); + assert!( + work_statements.is_empty(), + "no application-level work statement may exist: {work_statements:?}" + ); + assert!( + captured.len() < 32, + "the statement count must be small and bounded, got {}: {captured:?}", + captured.len() + ); + + println!( + "BE-03 catalogue-scale measurement (disposable environment only):\n \ + target works: {target_work_count}\n \ + control works (other publisher): {control_work_count}\n \ + SQL statements issued by the configuration operation: {}\n \ + work rows changed by the publisher trigger: {target_moved}\n \ + unrelated publisher work rows changed: {control_moved}\n \ + request duration in this disposable environment: {elapsed:?}\n \ + statements:\n{}", + captured.len(), + captured + .iter() + .enumerate() + .map(|(index, sql)| format!(" {}. {sql}", index + 1)) + .collect::>() + .join("\n") + ); +} + +#[test] +fn the_protected_assignment_resolver_is_loader_first_and_uses_try_load_only() { + let source = include_str!("model.rs"); + let body = source + .split_once("pub async fn enabled_distribution_platforms(") + .expect("protected assignment resolver") + .1 + .split_once("\n }\n") + .expect("resolver body") + .0; + + assert!( + body.contains("publisher_distribution_platforms"), + "the protected field must reuse BE-02's existing assignment loader" + ); + assert!( + body.contains(".try_load("), + "`try_load` is the only approved API" + ); + assert!( + !body.contains(".load("), + "`Loader::load` panics on a missing key" + ); + assert!( + !body.contains(".await;\n") || body.matches(".await").count() == 1, + "no unrelated awaited work may precede the loader key registration" + ); +} + +#[test] +fn no_second_assignment_loader_was_introduced() { + let source = include_str!("dataloader.rs"); + assert_eq!( + source.matches("pub(crate) struct").count() + - source.matches("pub(crate) struct SharedBatchError").count() + - source.matches("pub(crate) struct LoaderConfig").count() + - source.matches("pub(crate) struct RequestLoaders").count(), + 1, + "exactly one batcher struct exists" + ); + assert_eq!( + source + .matches("pub(crate) publisher_distribution_platforms:") + .count(), + 1, + "exactly one assignment loader field exists on the request bundle" + ); + assert!( + !source.contains("ServiceConfigurationLoader"), + "BE-03 introduces no loader of its own" + ); +} diff --git a/thoth-api/src/graphql/tests.rs b/thoth-api/src/graphql/tests.rs index 19ac07f2..5ac25aa5 100644 --- a/thoth-api/src/graphql/tests.rs +++ b/thoth-api/src/graphql/tests.rs @@ -3801,32 +3801,118 @@ fn graphql_mutations_cover_all() { // BE-01: the publisher package foundation must expose no public GraphQL // surface. See docs/engineering/ai-delivery/tasks/BE-01.md section 6.6. +// +// BE-03 deliberately makes `ThothPackage` and `PublisherCapability` +// SDL-reachable for the first time, and only through the protected +// `PublisherServiceConfiguration` type (BE-03 section 14.2). A blanket +// "these strings must not appear anywhere in the SDL" assertion is therefore +// intentionally false from BE-03 onwards. The security intent it encoded — that +// no package or capability value is readable on the public `Publisher` type or +// by an anonymous caller — is preserved below, and strengthened from a +// whole-document string search into per-type assertions plus a reachability +// assertion, because type reachability in the SDL is not value exposure. + +// The extraction these guards depend on is brace-balanced and string-aware, so +// each guard inspects the **whole** declaration. See +// `crate::graphql::sdl_support::sdl_block` for why a `split_once('}')` +// extraction silently truncated the public `Publisher` type at `imprints`. +use crate::graphql::sdl_support::sdl_block; #[test] -fn generated_schema_exposes_no_package_or_capability_surface() { +fn the_public_publisher_type_exposes_no_package_capability_or_configuration_field() { let schema = create_schema(); let sdl = schema.as_sdl(); + let publisher_type = sdl_block(&sdl, "type Publisher {"); + + // Coverage precondition. `imprints` carries a nested object default, which + // the previous extraction treated as the end of the type; these two fields + // are declared after it. Asserting them here means the prohibitions below + // are known to have been applied to the whole declaration rather than to a + // truncated prefix of it. + for post_imprints_sentinel in ["contacts(", "distributionPlatforms:"] { + assert!( + publisher_type.contains(post_imprints_sentinel), + "guard coverage is incomplete: `{post_imprints_sentinel}` is declared after \ + `imprints` but was not extracted: {publisher_type}" + ); + } + for forbidden in [ "subscriptionPackage", "ThothPackage", - "PublisherCapability", + "effectiveCapabilities", "capabilities", - "OASIS", - "OBELISK", - "SPHINX", - "PYRAMID", - "OAI_PMH", - "METRICS_COLLECT", - "METRICS_IMPORT", - "METRICS_DASHBOARD", - "METRICS_WIDGET", - "METRICS_OPERAS_EXPORT", + "PublisherCapability", + // No configuration-version field of any spelling. + "serviceConfiguration", + "configurationVersion", + "expectedUpdatedAt", + "serviceConfigurationUpdatedAt", ] { assert!( - !sdl.contains(forbidden), - "Generated GraphQL SDL unexpectedly contains {forbidden}" + !publisher_type.contains(forbidden), + "the public Publisher type must not expose `{forbidden}`: {publisher_type}" + ); + } + + // The publisher input types stay package-free too. + for input in ["input NewPublisher {", "input PatchPublisher {"] { + let block = sdl_block(&sdl, input); + assert!(!block.contains("subscriptionPackage")); + assert!(!block.contains("apabilit")); + } +} + +#[test] +fn package_and_capability_enums_are_sdl_reachable_only_through_protected_configuration() { + let schema = create_schema(); + let sdl = schema.as_sdl(); + + // Both enums are reachable, exactly as BE-03 section 14.2 requires. + assert_eq!(sdl.matches("enum ThothPackage {").count(), 1); + assert_eq!(sdl.matches("enum PublisherCapability {").count(), 1); + + // `PublisherCapability` is returned by exactly one field in the whole + // schema, and that field is on the protected configuration type. + let capability_fields: Vec<&str> = sdl + .lines() + .filter(|line| { + line.contains("PublisherCapability!]!") || line.contains(": PublisherCapability") + }) + .collect(); + assert_eq!( + capability_fields.len(), + 1, + "exactly one field may return PublisherCapability, got: {capability_fields:?}" + ); + assert!(capability_fields[0].contains("effectiveCapabilities: [PublisherCapability!]!")); + assert!(sdl_block(&sdl, "type PublisherServiceConfiguration {") + .contains("effectiveCapabilities: [PublisherCapability!]!")); + + // Every `ThothPackage` reference is on the protected configuration type, in + // the superuser-only mutation input, or in a superuser-only report + // argument. Nothing else in the schema mentions the package. + assert!(sdl_block(&sdl, "type PublisherServiceConfiguration {") + .contains("subscriptionPackage: ThothPackage!")); + assert!( + sdl_block(&sdl, "input ReplacePublisherServiceConfigurationInput {") + .contains("subscriptionPackage: ThothPackage!") + ); + for line in sdl.lines().filter(|line| line.contains("ThothPackage")) { + let is_enum_declaration = line.contains("enum ThothPackage {"); + let is_protected_field = line.trim() == "subscriptionPackage: ThothPackage!"; + let is_protected_argument = line.contains("publisherServiceConfigurations(") + || line.contains("publisherServiceConfigurationCount("); + assert!( + is_enum_declaration || is_protected_field || is_protected_argument, + "unexpected ThothPackage reference in the SDL: {line}" ); } + assert_eq!( + sdl.matches("subscriptionPackage: ThothPackage!").count(), + 2, + "exactly the protected type field and the superuser input field" + ); } #[test] diff --git a/thoth-api/src/model/mod.rs b/thoth-api/src/model/mod.rs index d4be6429..ffb62781 100644 --- a/thoth-api/src/model/mod.rs +++ b/thoth-api/src/model/mod.rs @@ -900,6 +900,7 @@ pub mod price; pub mod publication; pub mod publisher; pub mod publisher_distribution_platform; +pub mod publisher_service_configuration; pub mod reference; pub mod series; pub mod subject; diff --git a/thoth-api/src/model/publisher/mod.rs b/thoth-api/src/model/publisher/mod.rs index 12235404..cdc7180e 100644 --- a/thoth-api/src/model/publisher/mod.rs +++ b/thoth-api/src/model/publisher/mod.rs @@ -188,6 +188,10 @@ pub struct Publisher { pub subscription_package: ThothPackage, pub created_at: Timestamp, pub updated_at: Timestamp, + // Snapshots and API payloads serialized before BE-03 lack this field; + // deserialization must not require it, so the documented default applies. + #[serde(default)] + pub service_configuration_updated_at: Timestamp, } #[cfg_attr( diff --git a/thoth-api/src/model/publisher_distribution_platform/crud.rs b/thoth-api/src/model/publisher_distribution_platform/crud.rs index 929a3f66..6d8dc8db 100644 --- a/thoth-api/src/model/publisher_distribution_platform/crud.rs +++ b/thoth-api/src/model/publisher_distribution_platform/crud.rs @@ -19,6 +19,33 @@ use crate::schema::{publisher, publisher_distribution_platform}; /// The columns of the public assignment projection, in canonical order. type AssignmentRow = (Uuid, DistributionPlatform, Timestamp); +/// Whether one connection-scoped lifecycle call wrote persisted assignment +/// state. +/// +/// `BE-02`'s pool-level `enable`/`disable` return `ThothResult<()>`, so a caller +/// cannot distinguish an idempotent no-op from a linked-state repair. The +/// connection-scoped primitives report this instead, which is what lets the +/// `BE-03` service-configuration write coordinator decide whether a request was +/// a true no-op (specification section 7.7 item 3). +/// +/// It is deliberately the minimum `BE-03` needs — whether persisted state +/// changed — and is not a job-oriented change description. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[must_use] +pub(crate) enum AssignmentLifecycleOutcome { + /// Nothing was written and no timestamp moved. + Unchanged, + /// Persisted assignment state was written: an insert, a re-enable, a + /// disable, or a linked-state repair. + Changed, +} + +impl AssignmentLifecycleOutcome { + pub(crate) fn changed(self) -> bool { + self == AssignmentLifecycleOutcome::Changed + } +} + impl PublisherDistributionPlatform { /// Enable `platform` for `publisher_id`, normalizing its linked group. /// @@ -41,48 +68,82 @@ impl PublisherDistributionPlatform { publisher_id: Uuid, platform: DistributionPlatform, ) -> ThothResult<()> { + // Kept exactly as merged: the non-assignable check runs **before** a + // connection is acquired or a transaction opened. if !platform.is_assignable() { return Err(ThothError::DistributionPlatformNotAssignable( platform.to_string(), )); } - let members = platform.linked_members(); let mut connection = db.get()?; connection.transaction(|connection| { - lock_publisher(connection, publisher_id)?; - let existing = member_rows(connection, publisher_id, &members)?; - if is_normalized_fully_enabled(&existing, &members) { - return Ok(()); - } - let activation_id = Uuid::new_v4(); - let transition_at = transaction_timestamp(connection)?; - for member in &members { - diesel::insert_into(publisher_distribution_platform::table) - .values(( - publisher_distribution_platform::publisher_id.eq(publisher_id), - publisher_distribution_platform::platform.eq(*member), - publisher_distribution_platform::enabled.eq(true), - publisher_distribution_platform::activation_id.eq(activation_id), - publisher_distribution_platform::enabled_at.eq(transition_at), - publisher_distribution_platform::disabled_at.eq(None::), - )) - .on_conflict(( - publisher_distribution_platform::publisher_id, - publisher_distribution_platform::platform, - )) - .do_update() - .set(( - publisher_distribution_platform::enabled.eq(true), - publisher_distribution_platform::activation_id.eq(activation_id), - publisher_distribution_platform::enabled_at.eq(transition_at), - publisher_distribution_platform::disabled_at.eq(None::), - )) - .execute(connection)?; - } + let _outcome = Self::enable_on(connection, publisher_id, platform)?; Ok(()) }) } + /// The connection-scoped enable primitive, assuming a caller-owned + /// transaction. + /// + /// This is the single implementation of [`Self::enable`]'s lifecycle: the + /// pool-level function is a wrapper that acquires a connection, opens a + /// transaction, delegates here and discards the outcome. There is exactly + /// one linked-platform algorithm in the repository and it is this one. + /// + /// The non-assignable check is performed **here as well**, independently of + /// anything the caller did and before any write, so `BE-02`'s fail-closed + /// semantics hold for every internal caller that composes this primitive + /// into its own transaction (specification section 7.7 item 2). It calls the + /// same [`DistributionPlatform::is_assignable`] predicate and returns the + /// same [`ThothError::DistributionPlatformNotAssignable`] as the wrapper, so + /// this is one rule called from two places, not a second rule. + /// + /// Re-taking the publisher row lock is a harmless no-op when the caller's + /// transaction already holds it, so the lock statement is unconditional. + pub(crate) fn enable_on( + connection: &mut PgConnection, + publisher_id: Uuid, + platform: DistributionPlatform, + ) -> ThothResult { + if !platform.is_assignable() { + return Err(ThothError::DistributionPlatformNotAssignable( + platform.to_string(), + )); + } + let members = platform.linked_members(); + lock_publisher(connection, publisher_id)?; + let existing = member_rows(connection, publisher_id, &members)?; + if is_normalized_fully_enabled(&existing, &members) { + return Ok(AssignmentLifecycleOutcome::Unchanged); + } + let activation_id = Uuid::new_v4(); + let transition_at = transaction_timestamp(connection)?; + for member in &members { + diesel::insert_into(publisher_distribution_platform::table) + .values(( + publisher_distribution_platform::publisher_id.eq(publisher_id), + publisher_distribution_platform::platform.eq(*member), + publisher_distribution_platform::enabled.eq(true), + publisher_distribution_platform::activation_id.eq(activation_id), + publisher_distribution_platform::enabled_at.eq(transition_at), + publisher_distribution_platform::disabled_at.eq(None::), + )) + .on_conflict(( + publisher_distribution_platform::publisher_id, + publisher_distribution_platform::platform, + )) + .do_update() + .set(( + publisher_distribution_platform::enabled.eq(true), + publisher_distribution_platform::activation_id.eq(activation_id), + publisher_distribution_platform::enabled_at.eq(transition_at), + publisher_distribution_platform::disabled_at.eq(None::), + )) + .execute(connection)?; + } + Ok(AssignmentLifecycleOutcome::Changed) + } + /// Disable `platform` for `publisher_id`, and every member of its linked /// group. /// @@ -96,34 +157,51 @@ impl PublisherDistributionPlatform { publisher_id: Uuid, platform: DistributionPlatform, ) -> ThothResult<()> { - let members = platform.linked_members(); let mut connection = db.get()?; connection.transaction(|connection| { - lock_publisher(connection, publisher_id)?; - let existing = member_rows(connection, publisher_id, &members)?; - if !existing.iter().any(|row| row.enabled) { - return Ok(()); - } - let transition_at = transaction_timestamp(connection)?; - let enabled_members: Vec = existing - .iter() - .filter(|row| row.enabled) - .map(|row| row.platform) - .collect(); - diesel::update( - publisher_distribution_platform::table - .filter(publisher_distribution_platform::publisher_id.eq(publisher_id)) - .filter(publisher_distribution_platform::platform.eq_any(&enabled_members)), - ) - .set(( - publisher_distribution_platform::enabled.eq(false), - publisher_distribution_platform::disabled_at.eq(Some(transition_at)), - )) - .execute(connection)?; + let _outcome = Self::disable_on(connection, publisher_id, platform)?; Ok(()) }) } + /// The connection-scoped disable primitive, assuming a caller-owned + /// transaction. + /// + /// This is the single implementation of [`Self::disable`]'s lifecycle, and + /// reports whether it wrote a disable transition. When no member of the + /// linked group is currently enabled — including when no row exists at all — + /// it writes nothing and returns + /// [`AssignmentLifecycleOutcome::Unchanged`]. + pub(crate) fn disable_on( + connection: &mut PgConnection, + publisher_id: Uuid, + platform: DistributionPlatform, + ) -> ThothResult { + let members = platform.linked_members(); + lock_publisher(connection, publisher_id)?; + let existing = member_rows(connection, publisher_id, &members)?; + if !existing.iter().any(|row| row.enabled) { + return Ok(AssignmentLifecycleOutcome::Unchanged); + } + let transition_at = transaction_timestamp(connection)?; + let enabled_members: Vec = existing + .iter() + .filter(|row| row.enabled) + .map(|row| row.platform) + .collect(); + diesel::update( + publisher_distribution_platform::table + .filter(publisher_distribution_platform::publisher_id.eq(publisher_id)) + .filter(publisher_distribution_platform::platform.eq_any(&enabled_members)), + ) + .set(( + publisher_distribution_platform::enabled.eq(false), + publisher_distribution_platform::disabled_at.eq(Some(transition_at)), + )) + .execute(connection)?; + Ok(AssignmentLifecycleOutcome::Changed) + } + /// Every persisted assignment row for one publisher, in canonical /// destination order, including retained disabled rows. /// @@ -201,7 +279,12 @@ pub(crate) fn enabled_assignment_rows( /// Reads used to decide a transition run after this lock and inside the same /// transaction, so concurrent transitions serialize rather than racing. /// Different publishers never contend on the same lock. -fn lock_publisher(connection: &mut PgConnection, publisher_id: Uuid) -> ThothResult<()> { +/// +/// `BE-03`'s service-configuration write coordinator takes the **same** lock, on +/// the same row, as the first statement of its own transaction, so the +/// application-level lock order is `publisher` row first, everything else after +/// (specification section 7.8). +pub(crate) fn lock_publisher(connection: &mut PgConnection, publisher_id: Uuid) -> ThothResult<()> { publisher::table .filter(publisher::publisher_id.eq(publisher_id)) .select(publisher::publisher_id) diff --git a/thoth-api/src/model/publisher_distribution_platform/tests.rs b/thoth-api/src/model/publisher_distribution_platform/tests.rs index 84a4bc86..f56dfa0c 100644 --- a/thoth-api/src/model/publisher_distribution_platform/tests.rs +++ b/thoth-api/src/model/publisher_distribution_platform/tests.rs @@ -2,13 +2,15 @@ use std::str::FromStr; use std::sync::Arc; use diesel::sql_query; -use diesel::RunQueryDsl; +use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; use uuid::Uuid; use super::*; use crate::db::PgPool; +use crate::model::publisher::Publisher; use crate::model::tests::db as test_db; -use thoth_errors::ThothError; +use crate::model::Crud; +use thoth_errors::{ThothError, ThothResult}; // -------------------------------------------------------------------------- // Inventory and representation @@ -1166,6 +1168,185 @@ fn enabling_jisc_nbk_fails_closed_before_any_write() { ); } +// -------------------------------------------------------------------------- +// Connection-scoped lifecycle primitives (`BE-03` specification section 7.7) +// -------------------------------------------------------------------------- + +/// Run `body` inside a caller-owned transaction, exactly as the `BE-03` +/// coordinator composes these primitives. +fn in_transaction( + pool: &PgPool, + body: impl FnOnce(&mut diesel::pg::PgConnection) -> ThothResult, +) -> ThothResult { + use diesel::Connection; + let mut connection = pool.get().expect("connection"); + connection.transaction(|connection| body(connection)) +} + +#[test] +fn the_connection_scoped_primitives_report_every_transition_outcome() { + use crate::model::publisher_distribution_platform::crud::AssignmentLifecycleOutcome::{ + Changed, Unchanged, + }; + + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let publisher_id = publisher.publisher_id; + let singleton = DistributionPlatform::Zenodo; + + // Absent row enabled. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on(connection, publisher_id, singleton) + }) + .expect("enable"), + Changed + ); + // Already-enabled singleton. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on(connection, publisher_id, singleton) + }) + .expect("enable"), + Unchanged + ); + // Enabled group disabled. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::disable_on(connection, publisher_id, singleton) + }) + .expect("disable"), + Changed + ); + // Group with no enabled member disabled. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::disable_on(connection, publisher_id, singleton) + }) + .expect("disable"), + Unchanged + ); + // Disabled row re-enabled. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on(connection, publisher_id, singleton) + }) + .expect("enable"), + Changed + ); + + // Already-normalized linked group. + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on( + connection, + publisher_id, + DistributionPlatform::Oapen, + ) + }) + .expect("enable"), + Changed + ); + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on( + connection, + publisher_id, + DistributionPlatform::Doab, + ) + }) + .expect("enable"), + Unchanged + ); + + // Split pair: membership is unchanged, but the group is not normalized. + write_raw_assignment( + &pool, + publisher_id, + "DOAB", + Uuid::new_v4(), + "now() - interval '1 hour'", + ); + assert_eq!( + in_transaction(&pool, |connection| { + PublisherDistributionPlatform::enable_on( + connection, + publisher_id, + DistributionPlatform::Oapen, + ) + }) + .expect("repair"), + Changed + ); +} + +#[test] +fn the_connection_scoped_enable_rejects_jisc_nbk_itself_before_any_write() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let publisher_id = publisher.publisher_id; + PublisherDistributionPlatform::enable(&pool, publisher_id, DistributionPlatform::Zenodo) + .expect("seed enable"); + let before = + PublisherDistributionPlatform::all_for_publisher(&pool, publisher_id).expect("load before"); + let publisher_before = Publisher::from_id(&pool, &publisher_id).expect("publisher before"); + + // Called **directly** inside a caller-owned transaction, bypassing both the + // pool-level wrapper and the BE-03 coordinator, so nothing has pre-validated + // the platform. + for commit in [false, true] { + let mut connection = pool.get().expect("connection"); + let observed: ThothResult<()> = { + use diesel::Connection; + connection.transaction(|connection| { + let error = PublisherDistributionPlatform::enable_on( + connection, + publisher_id, + DistributionPlatform::JiscNbk, + ) + .expect_err("JISC NBK is not assignable"); + assert_eq!( + error, + ThothError::DistributionPlatformNotAssignable("JISC_NBK".to_string()) + ); + // The primitive failed before any write, so the caller's own + // transaction may be committed **or** rolled back with no hidden + // mutation either way. + if commit { + Ok(()) + } else { + Err(ThothError::EntityNotFound) + } + }) + }; + assert_eq!(observed.is_ok(), commit); + } + + let after = + PublisherDistributionPlatform::all_for_publisher(&pool, publisher_id).expect("load after"); + assert_eq!( + after, before, + "no row may be created or changed by a rejected connection-scoped enable" + ); + assert!(after + .iter() + .all(|row| row.platform != DistributionPlatform::JiscNbk)); + assert_eq!( + Publisher::from_id(&pool, &publisher_id).expect("publisher after"), + publisher_before, + "this path never reaches the coordinator's committed-change phase" + ); + let mut connection = pool.get().expect("connection"); + let audit: i64 = crate::schema::publisher_service_configuration_history::table + .filter( + crate::schema::publisher_service_configuration_history::publisher_id.eq(publisher_id), + ) + .count() + .get_result(&mut connection) + .expect("audit count"); + assert_eq!(audit, 0); +} + // -------------------------------------------------------------------------- // Read paths // -------------------------------------------------------------------------- diff --git a/thoth-api/src/model/publisher_service_configuration/crud.rs b/thoth-api/src/model/publisher_service_configuration/crud.rs new file mode 100644 index 00000000..acbb0dbe --- /dev/null +++ b/thoth-api/src/model/publisher_service_configuration/crud.rs @@ -0,0 +1,525 @@ +//! The canonical service-configuration write coordinator and the superuser +//! staff-report queries (`BE-03`). +//! +//! `Crud` is deliberately **not** implemented for service configuration: there +//! is no generic create/update/delete surface for it. The one supported +//! production write is [`replace_publisher_service_configuration`], and it is +//! the only place in the repository that commits a change to a publisher's +//! desired package, desired enabled platform state, canonical configuration +//! version token or configuration audit history. + +use std::collections::{HashMap, HashSet}; + +use diesel::pg::PgConnection; +use diesel::{Connection, ExpressionMethods, QueryDsl, RunQueryDsl}; +use thoth_errors::{ThothError, ThothResult}; +use uuid::Uuid; + +use super::{ + CanonicalServiceConfigurationState, NewPublisherServiceConfigurationHistory, + PublisherServiceConfiguration, PublisherServiceConfigurationChange, + PublisherServiceConfigurationSource, PublisherServiceConfigurationSummary, + ReplacePublisherServiceConfigurationInput, ServiceConfigurationWriteContext, +}; +use crate::db::PgPool; +use crate::model::publisher::{Publisher, PublisherField, PublisherOrderBy, ThothPackage}; +use crate::model::publisher_distribution_platform::crud::{ + enabled_assignment_rows, lock_publisher, +}; +use crate::model::publisher_distribution_platform::{ + DistributionPlatform, PublisherDistributionPlatform, +}; +use crate::model::Timestamp; +use crate::schema::{ + publisher, publisher_distribution_platform, publisher_service_configuration_history, +}; + +/// The latest-change columns the staff report needs, in canonical order. +type LatestChangeRow = (Uuid, Timestamp, String, PublisherServiceConfigurationSource); + +/// Replace one publisher's desired service configuration atomically. +/// +/// **This is the single authoritative production write path for desired service +/// configuration** (specification section 7.6). It owns every committed write +/// across all four of: `publisher.subscription_package`, the publisher's enabled +/// distribution-platform desired state, the canonical version token +/// `publisher.service_configuration_updated_at`, and +/// `publisher_service_configuration_history`. +/// +/// It executes exactly **one** transaction on **one** connection and performs +/// the whole of specification section 7.3 steps 2 to 12 inside it. `BE-04` will +/// later extend that same transaction between steps 9 and 12 to create durable +/// job rows atomically with the desired-state change; `BE-03` adds no job, hook, +/// callback, event or placeholder for it. +/// +/// It makes **no authorization decision of its own**. Authorization is the +/// caller's responsibility (specification sections 7.2 and 11.1), and the caller +/// supplies the audit provenance explicitly as a +/// [`ServiceConfigurationWriteContext`]. +/// +/// All platform-assignment writes go through `BE-02`'s connection-scoped +/// lifecycle primitives; this function never writes +/// `publisher_distribution_platform` directly and never re-implements the +/// linked-group normalization, the normalized-state predicate, the activation +/// and timestamp semantics or the non-assignable rule. +/// +/// It creates no distribution job and triggers no dissemination. +pub(crate) fn replace_publisher_service_configuration( + db: &PgPool, + write_context: &ServiceConfigurationWriteContext<'_>, + data: &ReplacePublisherServiceConfigurationInput, +) -> ThothResult { + let mut connection = db.get()?; + connection.transaction(|connection| replace_in_transaction(connection, write_context, data)) +} + +/// Specification section 7.3 steps 2 to 12, inside the coordinator's single +/// transaction. +/// +/// This is deliberately private to the coordinator's module: it is the +/// coordinator's transaction body, not a second write entry point. +fn replace_in_transaction( + connection: &mut PgConnection, + write_context: &ServiceConfigurationWriteContext<'_>, + data: &ReplacePublisherServiceConfigurationInput, +) -> ThothResult { + // Step 2. The publisher row lock is the first statement of the transaction, + // so every read and write below happens under it. An absent row ends the + // transaction with no write. + lock_publisher(connection, data.publisher_id)?; + + // Step 3. The canonical current configuration, read under that lock. + let current = publisher_row(connection, data.publisher_id)?; + let current_enabled = enabled_platforms(connection, data.publisher_id)?; + let previous_token = current.service_configuration_updated_at; + + // Steps 4 and 5. Staleness precedes validation and every lifecycle call, so + // a stale request writes nothing even when it would otherwise have been a + // true no-op or would otherwise have repaired a split linked group. + if previous_token != data.expected_updated_at { + return Err(ThothError::StalePublisherServiceConfiguration); + } + + // Step 6. Deduplicate the requested set and close it under linked + // membership, so naming either OAPEN or DOAB enables both and naming + // neither disables both. + let desired = normalize_requested_platforms(&data.enabled_distribution_platforms); + + // Step 7. Validate the **whole** normalized desired set before any write, so + // a rejected request never depends on rollback and fails before the first + // lifecycle call. + for platform in &desired { + if !platform.is_assignable() { + return Err(ThothError::DistributionPlatformNotAssignable( + platform.to_string(), + )); + } + } + + // Step 8. Compare only: the package write is deferred to the single + // publisher UPDATE of step 10. Writing it here as well would update the + // publisher row twice for a combined package-and-platform change, and the + // publisher row carries the shared AFTER UPDATE work-freshness trigger, so + // the second write would re-run that trigger's set-based cascade over the + // same N work rows for no additional effect. + let package_changed = current.subscription_package != data.subscription_package; + + // Step 9. Desired state is applied only through BE-02's connection-scoped + // primitives, one canonical representative per linked group. + // + // Every desired group is enabled **unconditionally**: the call is not gated + // on a membership diff, because membership equality does not imply the group + // is normalized. The primitive alone decides no-op versus repair. + let mut lifecycle_changed = false; + for representative in group_representatives(&desired) { + let outcome = PublisherDistributionPlatform::enable_on( + connection, + data.publisher_id, + representative, + )?; + lifecycle_changed |= outcome.changed(); + } + + // Closure under linked membership guarantees a group is either wholly + // desired or wholly undesired, so no group receives both calls. + let undesired_enabled: Vec = current_enabled + .iter() + .copied() + .filter(|platform| !desired.contains(platform)) + .collect(); + for representative in group_representatives(&undesired_enabled) { + let outcome = PublisherDistributionPlatform::disable_on( + connection, + data.publisher_id, + representative, + )?; + lifecycle_changed |= outcome.changed(); + } + + // A true no-op: the package was unchanged and every lifecycle call reported + // `Unchanged`. No publisher UPDATE, no token movement, no audit row, and + // therefore neither publisher trigger fires. + if !package_changed && !lifecycle_changed { + return Ok(PublisherServiceConfiguration::new(current)); + } + + // Step 10. **Exactly one** publisher UPDATE for the whole committed change. + // It carries the package only when the package changed, and always carries + // the token. Because the publisher row carries the shared AFTER UPDATE + // work-freshness trigger, this single statement means the cascade runs once + // per committed change — the same cost for a combined package-and-platform + // change as for a platform-only change or a linked repair. + // + // `GREATEST` makes the token strictly increasing per publisher. + // `CURRENT_TIMESTAMP` is `transaction_timestamp()`, so a transaction that + // started earlier but blocked on the row lock would otherwise be able to + // store a value equal to a token some client still holds. + // The two branches differ only in whether the package travels with the + // token. `publisher::table` is repeated rather than hoisted into a local so + // that both writes remain visible to the specification's write-path + // containment search for `diesel::update` against `publisher::table`. + let next_token = || { + diesel::dsl::sql::( + "GREATEST(CURRENT_TIMESTAMP, service_configuration_updated_at + interval '1 microsecond')", + ) + }; + let updated: Publisher = if package_changed { + diesel::update(publisher::table.filter(publisher::publisher_id.eq(data.publisher_id))) + .set(( + publisher::subscription_package.eq(data.subscription_package), + publisher::service_configuration_updated_at.eq(next_token()), + )) + .returning(publisher::all_columns) + .get_result(connection)? + } else { + diesel::update(publisher::table.filter(publisher::publisher_id.eq(data.publisher_id))) + .set(publisher::service_configuration_updated_at.eq(next_token())) + .returning(publisher::all_columns) + .get_result(connection)? + }; + + // Step 11. Exactly one audit row for the whole committed change, with the + // caller-supplied source and actor. The after state is read back from the + // database rather than assumed from the request, so the row records what was + // actually persisted. + let after_enabled = enabled_platforms(connection, data.publisher_id)?; + let before_state = CanonicalServiceConfigurationState { + subscription_package: current.subscription_package, + enabled_distribution_platforms: current_enabled, + configuration_version: previous_token, + }; + let after_state = CanonicalServiceConfigurationState { + subscription_package: updated.subscription_package, + enabled_distribution_platforms: after_enabled, + configuration_version: updated.service_configuration_updated_at, + }; + let audit = NewPublisherServiceConfigurationHistory { + publisher_id: data.publisher_id, + actor: write_context.actor.to_string(), + source: write_context.source, + before_state: serde_json::to_value(&before_state)?, + after_state: serde_json::to_value(&after_state)?, + }; + diesel::insert_into(publisher_service_configuration_history::table) + .values(&audit) + .execute(connection)?; + + // Step 12. + Ok(PublisherServiceConfiguration::new(updated)) +} + +/// The requested platform set, deduplicated and closed under linked membership, +/// in canonical [`DistributionPlatform::ALL`] order. +/// +/// Duplicates are deduplicated rather than rejected: the argument is a set. +pub(crate) fn normalize_requested_platforms( + requested: &[DistributionPlatform], +) -> Vec { + let mut closed: HashSet = HashSet::new(); + for platform in requested { + for member in platform.linked_members() { + closed.insert(member); + } + } + DistributionPlatform::ALL + .into_iter() + .filter(|platform| closed.contains(platform)) + .collect() +} + +/// One canonical representative per linked group: the earliest member of the +/// group present in `platforms`, which must already be in canonical order. +fn group_representatives(platforms: &[DistributionPlatform]) -> Vec { + let mut covered: HashSet = HashSet::new(); + let mut representatives: Vec = Vec::new(); + for platform in platforms { + if covered.contains(platform) { + continue; + } + for member in platform.linked_members() { + covered.insert(member); + } + representatives.push(*platform); + } + representatives +} + +/// One publisher row, read on the caller's connection. +fn publisher_row(connection: &mut PgConnection, publisher_id: Uuid) -> ThothResult { + publisher::table + .filter(publisher::publisher_id.eq(publisher_id)) + .first::(connection) + .map_err(Into::into) +} + +/// The publisher's currently enabled platforms, in canonical order. +/// +/// This reuses `BE-02`'s existing set-based assignment statement rather than +/// introducing a second one. +fn enabled_platforms( + connection: &mut PgConnection, + publisher_id: Uuid, +) -> ThothResult> { + Ok(enabled_assignment_rows(connection, &[publisher_id])? + .into_iter() + .map(|(_, platform, _)| platform) + .collect()) +} + +impl PublisherServiceConfiguration { + /// The superuser staff report: one page of publisher configurations with + /// their latest change metadata. + /// + /// Two set-based statements for a page of N publishers — one for the + /// filtered, ordered, paginated publisher page and one for the latest change + /// per publisher in that page — and no per-publisher loop. The protected + /// `enabledDistributionPlatforms` field resolves separately through `BE-02`'s + /// existing request-local assignment DataLoader. + pub(crate) fn all_summaries( + db: &PgPool, + limit: i32, + offset: i32, + order: PublisherOrderBy, + publishers: Vec, + packages: Vec, + enabled_platforms: Vec, + ) -> ThothResult> { + let mut connection = db.get()?; + let query = filtered_publishers(publishers, packages, enabled_platforms); + // The requested order plus a mandatory `publisher_id ASC` tie-breaker, + // exactly as `publishersByDistributionPlatform` does, so offset + // pagination is deterministic. + let query = match order.field { + PublisherField::PublisherId => { + apply_directional_order!( + query, + order.direction, + order, + publisher::publisher_id, + publisher::publisher_id + ) + } + PublisherField::PublisherName => { + apply_directional_order!( + query, + order.direction, + order, + publisher::publisher_name, + publisher::publisher_id + ) + } + PublisherField::PublisherShortname => { + apply_directional_order!( + query, + order.direction, + order, + publisher::publisher_shortname, + publisher::publisher_id + ) + } + PublisherField::PublisherUrl => { + apply_directional_order!( + query, + order.direction, + order, + publisher::publisher_url, + publisher::publisher_id + ) + } + PublisherField::ZitadelId => { + apply_directional_order!( + query, + order.direction, + order, + publisher::zitadel_id, + publisher::publisher_id + ) + } + PublisherField::AccessibilityStatement => { + apply_directional_order!( + query, + order.direction, + order, + publisher::accessibility_statement, + publisher::publisher_id + ) + } + PublisherField::AccessibilityReportUrl => { + apply_directional_order!( + query, + order.direction, + order, + publisher::accessibility_report_url, + publisher::publisher_id + ) + } + PublisherField::CreatedAt => { + apply_directional_order!( + query, + order.direction, + order, + publisher::created_at, + publisher::publisher_id + ) + } + PublisherField::UpdatedAt => { + apply_directional_order!( + query, + order.direction, + order, + publisher::updated_at, + publisher::publisher_id + ) + } + }; + + let page: Vec = query + .limit(limit.into()) + .offset(offset.into()) + .load::(&mut connection)?; + + let publisher_ids: Vec = page.iter().map(|row| row.publisher_id).collect(); + let mut changes = latest_changes(&mut connection, &publisher_ids)?; + + Ok(page + .into_iter() + .map(|row| PublisherServiceConfigurationSummary { + last_change: changes.remove(&row.publisher_id), + configuration: PublisherServiceConfiguration::new(row), + }) + .collect()) + } + + /// The number of publishers the staff report matches before pagination. + /// + /// This applies exactly the same filter predicates as + /// [`Self::all_summaries`]. + pub(crate) fn count( + db: &PgPool, + publishers: Vec, + packages: Vec, + enabled_platforms: Vec, + ) -> ThothResult { + let mut connection = db.get()?; + // See the `Crud::count` note on the i64 -> i32 conversion. + filtered_publishers(publishers, packages, enabled_platforms) + .count() + .get_result::(&mut connection) + .map(|total| total.to_string().parse::().unwrap()) + .map_err(Into::into) + } +} + +/// The staff report's filter predicates, shared by the list and count queries so +/// they cannot diverge. +/// +/// `enabled_platforms` narrows with **AND** semantics: a publisher matches only +/// if it has an enabled assignment for **every** requested platform. The +/// assignment primary key is `(publisher_id, platform)`, so a grouped count of +/// the matching enabled rows is exactly the number of distinct requested +/// platforms the publisher has enabled. +fn filtered_publishers<'a>( + publishers: Vec, + packages: Vec, + enabled_platforms: Vec, +) -> publisher::BoxedQuery<'a, diesel::pg::Pg> { + let mut query = publisher::table.into_boxed(); + if !publishers.is_empty() { + query = query.filter(publisher::publisher_id.eq_any(publishers)); + } + if !packages.is_empty() { + query = query.filter(publisher::subscription_package.eq_any(packages)); + } + let required: Vec = deduplicate_platforms(&enabled_platforms); + if !required.is_empty() { + let required_count = required.len() as i64; + query = query.filter( + publisher::publisher_id.eq_any( + publisher_distribution_platform::table + .filter(publisher_distribution_platform::enabled.eq(true)) + .filter(publisher_distribution_platform::platform.eq_any(required)) + .group_by(publisher_distribution_platform::publisher_id) + .having(diesel::dsl::count_star().eq(required_count)) + .select(publisher_distribution_platform::publisher_id), + ), + ); + } + query +} + +/// The requested platforms deduplicated, in canonical order, with **no** linked +/// closure: a report filter selects publishers, it does not assert desired +/// state. +fn deduplicate_platforms(requested: &[DistributionPlatform]) -> Vec { + let requested: HashSet = requested.iter().copied().collect(); + DistributionPlatform::ALL + .into_iter() + .filter(|platform| requested.contains(platform)) + .collect() +} + +/// The latest recorded change for each publisher in the page, in one set-based +/// statement. +/// +/// The `DISTINCT ON` order is a **total** order — `publisher_id`, then +/// `created_at DESC`, then the history id `DESC` — so the selected row is +/// deterministic even for two rows sharing a `created_at`. The composite index +/// created by the migration supports exactly this lookup. +fn latest_changes( + connection: &mut PgConnection, + publisher_ids: &[Uuid], +) -> ThothResult> { + if publisher_ids.is_empty() { + return Ok(HashMap::new()); + } + let rows: Vec = publisher_service_configuration_history::table + .filter(publisher_service_configuration_history::publisher_id.eq_any(publisher_ids)) + .distinct_on(publisher_service_configuration_history::publisher_id) + .order(( + publisher_service_configuration_history::publisher_id.asc(), + publisher_service_configuration_history::created_at.desc(), + publisher_service_configuration_history::publisher_service_configuration_history_id + .desc(), + )) + .select(( + publisher_service_configuration_history::publisher_id, + publisher_service_configuration_history::created_at, + publisher_service_configuration_history::actor, + publisher_service_configuration_history::source, + )) + .load::(connection)?; + + Ok(rows + .into_iter() + .map(|(publisher_id, changed_at, actor, source)| { + ( + publisher_id, + PublisherServiceConfigurationChange { + changed_at, + actor, + source, + }, + ) + }) + .collect()) +} diff --git a/thoth-api/src/model/publisher_service_configuration/mod.rs b/thoth-api/src/model/publisher_service_configuration/mod.rs new file mode 100644 index 00000000..110665ee --- /dev/null +++ b/thoth-api/src/model/publisher_service_configuration/mod.rs @@ -0,0 +1,197 @@ +//! Protected publisher service configuration (`BE-03`). +//! +//! This module owns the protected representation of one publisher's **desired** +//! service configuration — current subscription package, effective capability +//! codes and enabled distribution platforms — the canonical optimistic +//! concurrency token that versions it, the append-only configuration audit +//! record, and the single authoritative write coordinator through which every +//! committed production configuration change passes +//! ([`crud::replace_publisher_service_configuration`]). +//! +//! `BE-03` owns desired configuration only. It creates no distribution job, no +//! job target and no job attempt; it performs no dissemination; and it activates +//! no destination. +//! +//! Effective capabilities are **derived on read** from the canonical +//! `publisher.subscription_package` through `BE-01`'s code-owned +//! [`ThothPackage::capabilities`] and are persisted nowhere: there is no +//! capability column, table, override or cache, so no package/capability +//! inconsistency is representable (`ADR-0001` sections 4.1, 4.2 and 4.4). + +use serde::{Deserialize, Serialize}; +use strum::Display; +use strum::EnumString; +use uuid::Uuid; + +use crate::model::publisher::{Publisher, ThothPackage}; +use crate::model::publisher_distribution_platform::DistributionPlatform; +use crate::model::Timestamp; +#[cfg(feature = "backend")] +use crate::schema::publisher_service_configuration_history; + +/// How a recorded service-configuration change entered the system. +/// +/// The inventory is closed. There is deliberately no `OTHER`, no `UNKNOWN` and +/// no `Default`: an unrecognised value must fail rather than resolve to a +/// nearest source. +/// +/// `source` and `actor` form **one contract**: `source` fixes the namespace and +/// the required provenance of `actor` (specification section 9.1). +/// +/// `BE-03` writes only [`Self::SuperuserApi`]. [`Self::MigrationBackfill`] is +/// defined here so the separately approved and separately specified `MIG-01` +/// controlled historical backfill has a coherent value to write, and no `BE-03` +/// execution path emits it. +#[cfg_attr( + feature = "backend", + derive(diesel_derive_enum::DbEnum, juniper::GraphQLEnum), + graphql(description = "How a recorded service-configuration change entered the system"), + ExistingTypePath = "crate::schema::sql_types::PublisherServiceConfigurationSource" +)] +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash, EnumString, Display)] +#[serde(rename_all = "SCREAMING_SNAKE_CASE")] +#[strum(serialize_all = "SCREAMING_SNAKE_CASE")] +pub enum PublisherServiceConfigurationSource { + #[cfg_attr( + feature = "backend", + db_rename = "SUPERUSER_API", + graphql( + description = "A committed superuser replacePublisherServiceConfiguration call; the actor is the authenticated account identifier" + ) + )] + SuperuserApi, + #[cfg_attr( + feature = "backend", + db_rename = "MIGRATION_BACKFILL", + graphql( + description = "A separately approved controlled historical backfill; the actor is the authorized control identity" + ) + )] + MigrationBackfill, +} + +/// The desired service configuration of one publisher. +/// +/// The publisher row is the single source of truth for every value this type +/// exposes: the package and the configuration version token are columns on it, +/// and the effective capabilities are computed from that same package. A +/// response can therefore never report a package and a capability set that +/// disagree. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PublisherServiceConfiguration { + pub publisher: Publisher, +} + +impl PublisherServiceConfiguration { + pub fn new(publisher: Publisher) -> Self { + Self { publisher } + } + + pub fn publisher_id(&self) -> Uuid { + self.publisher.publisher_id + } +} + +// The `subscriptionPackage`, `effectiveCapabilities` and `updatedAt` accessors +// are the GraphQL resolvers in `crate::graphql::model`, which read them from +// this same `publisher` row. They are deliberately not duplicated here: one +// definition means a response can never report a package and a capability set +// that disagree, and the derivation has exactly one site. + +/// Metadata of one recorded service-configuration change. +/// +/// The audit `before_state`/`after_state` JSON is deliberately absent: it is +/// never exposed through any GraphQL surface. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PublisherServiceConfigurationChange { + pub changed_at: Timestamp, + pub actor: String, + pub source: PublisherServiceConfigurationSource, +} + +/// One publisher's service configuration together with its latest change +/// metadata, as returned by the superuser-only staff report. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PublisherServiceConfigurationSummary { + pub configuration: PublisherServiceConfiguration, + pub last_change: Option, +} + +/// One persisted configuration audit row. +/// +/// The table is append-only: it has no `updated_at` column and therefore no +/// `diesel_manage_updated_at` trigger, and nothing rewrites or deletes a row. +#[cfg_attr(feature = "backend", derive(diesel::Queryable))] +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PublisherServiceConfigurationHistory { + pub publisher_service_configuration_history_id: Uuid, + pub publisher_id: Uuid, + pub actor: String, + pub source: PublisherServiceConfigurationSource, + pub before_state: serde_json::Value, + pub after_state: serde_json::Value, + pub created_at: Timestamp, +} + +#[cfg_attr( + feature = "backend", + derive(diesel::Insertable), + diesel(table_name = publisher_service_configuration_history) +)] +pub struct NewPublisherServiceConfigurationHistory { + pub publisher_id: Uuid, + pub actor: String, + pub source: PublisherServiceConfigurationSource, + pub before_state: serde_json::Value, + pub after_state: serde_json::Value, +} + +/// The bounded canonical service-configuration state recorded in an audit row. +/// +/// The key set is exactly these three keys and must not be widened. Activation +/// identifiers, per-row `enabled_at`/`disabled_at` timestamps, disabled-platform +/// history, effective capabilities, credentials, endpoints and unrelated +/// publisher metadata are all deliberately absent (specification section 8.2). +/// A linked-state repair is already distinguishable through the differing +/// `configurationVersion` values. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct CanonicalServiceConfigurationState { + pub subscription_package: ThothPackage, + /// Serialized in canonical `DistributionPlatform::ALL` declaration order, so + /// two equal sets always serialize identically. + pub enabled_distribution_platforms: Vec, + pub configuration_version: Timestamp, +} + +/// The audit provenance the caller supplies to the write coordinator. +/// +/// This is a parameter, never inferred from ambient state and never a GraphQL +/// input. The coordinator makes no authentication or authorization decision of +/// its own; its callers authorize first and then supply this context +/// (specification section 7.6.2). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ServiceConfigurationWriteContext<'a> { + pub source: PublisherServiceConfigurationSource, + pub actor: &'a str, +} + +#[cfg_attr( + feature = "backend", + derive(juniper::GraphQLInputObject), + graphql( + description = "Complete desired service configuration to store for a publisher. This is a replace, not a patch: the platform list is the complete desired enabled set, and an empty list means no destination is enabled" + ) +)] +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReplacePublisherServiceConfigurationInput { + pub publisher_id: Uuid, + pub subscription_package: ThothPackage, + pub enabled_distribution_platforms: Vec, + pub expected_updated_at: Timestamp, +} + +#[cfg(feature = "backend")] +pub mod crud; +#[cfg(all(test, feature = "backend"))] +mod tests; diff --git a/thoth-api/src/model/publisher_service_configuration/tests.rs b/thoth-api/src/model/publisher_service_configuration/tests.rs new file mode 100644 index 00000000..fd9b5229 --- /dev/null +++ b/thoth-api/src/model/publisher_service_configuration/tests.rs @@ -0,0 +1,2139 @@ +//! `BE-03` coordinator, audit, concurrency, linked-platform, trigger-cascade +//! and staff-report evidence. +//! +//! Every test here runs against a real disposable PostgreSQL database with the +//! migration applied, so both publisher `UPDATE` triggers actually execute and +//! the database's own constraints are exercised rather than assumed. +//! +//! GraphQL-level authorization, capability exposure, SDL and query-count +//! evidence lives in `crate::graphql::service_configuration_tests`. + +use std::collections::HashSet; +use std::sync::Arc; + +use diesel::sql_query; +use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; +use serde_json::Value as JsonValue; +use uuid::Uuid; + +use super::crud::{normalize_requested_platforms, replace_publisher_service_configuration}; +use super::*; +use crate::db::PgPool; +use crate::model::publisher::{Publisher, PublisherField, PublisherOrderBy, ThothPackage}; +use crate::model::publisher_distribution_platform::{ + DistributionPlatform, PublisherDistributionPlatform, +}; +use crate::model::tests::db as test_db; +use crate::model::{Crud, Timestamp}; +use crate::schema::{publisher, publisher_history, publisher_service_configuration_history, work}; +use thoth_errors::{ThothError, ThothResult}; + +// -------------------------------------------------------------------------- +// Helpers +// -------------------------------------------------------------------------- + +const ACTOR: &str = "zitadel-superuser-1"; + +fn superuser_context() -> ServiceConfigurationWriteContext<'static> { + ServiceConfigurationWriteContext { + source: PublisherServiceConfigurationSource::SuperuserApi, + actor: ACTOR, + } +} + +fn input( + publisher_id: Uuid, + package: ThothPackage, + platforms: &[DistributionPlatform], + expected_updated_at: Timestamp, +) -> ReplacePublisherServiceConfigurationInput { + ReplacePublisherServiceConfigurationInput { + publisher_id, + subscription_package: package, + enabled_distribution_platforms: platforms.to_vec(), + expected_updated_at, + } +} + +/// Call the canonical coordinator exactly as the GraphQL mutation does, with a +/// superuser write context. +fn replace( + pool: &PgPool, + data: &ReplacePublisherServiceConfigurationInput, +) -> ThothResult { + replace_publisher_service_configuration(pool, &superuser_context(), data) +} + +fn publisher_row(pool: &PgPool, publisher_id: Uuid) -> Publisher { + Publisher::from_id(pool, &publisher_id).expect("publisher row") +} + +fn token(pool: &PgPool, publisher_id: Uuid) -> Timestamp { + publisher_row(pool, publisher_id).service_configuration_updated_at +} + +fn enabled(pool: &PgPool, publisher_id: Uuid) -> Vec { + PublisherDistributionPlatform::enabled_assignments(pool, publisher_id) + .expect("enabled assignments") + .into_iter() + .map(|assignment| assignment.platform) + .collect() +} + +fn audit_rows(pool: &PgPool, publisher_id: Uuid) -> Vec { + let mut connection = pool.get().expect("connection"); + publisher_service_configuration_history::table + .filter(publisher_service_configuration_history::publisher_id.eq(publisher_id)) + .order(publisher_service_configuration_history::created_at.asc()) + .load::(&mut connection) + .expect("audit rows") +} + +fn only_audit_row(pool: &PgPool, publisher_id: Uuid) -> PublisherServiceConfigurationHistory { + let mut rows = audit_rows(pool, publisher_id); + assert_eq!(rows.len(), 1, "expected exactly one audit row"); + rows.pop().expect("audit row") +} + +fn publisher_history_count(pool: &PgPool, publisher_id: Uuid) -> i64 { + let mut connection = pool.get().expect("connection"); + publisher_history::table + .filter(publisher_history::publisher_id.eq(publisher_id)) + .count() + .get_result::(&mut connection) + .expect("publisher history count") +} + +fn all_rows(pool: &PgPool, publisher_id: Uuid) -> Vec { + PublisherDistributionPlatform::all_for_publisher(pool, publisher_id).expect("assignment rows") +} + +/// Force a linked state the supported domain path cannot produce, so repair +/// behaviour can be proven. This mirrors `BE-02`'s own raw-write fixture. +fn write_raw_assignment( + pool: &PgPool, + publisher_id: Uuid, + platform: &str, + activation_id: Uuid, + enabled_at_sql: &str, +) { + let mut connection = pool.get().expect("connection"); + sql_query(format!( + "INSERT INTO publisher_distribution_platform \ + (publisher_id, platform, enabled, activation_id, enabled_at, disabled_at) \ + VALUES ('{publisher_id}', '{platform}', true, '{activation_id}', {enabled_at_sql}, NULL) \ + ON CONFLICT (publisher_id, platform) DO UPDATE SET \ + enabled = true, activation_id = EXCLUDED.activation_id, \ + enabled_at = EXCLUDED.enabled_at, disabled_at = NULL" + )) + .execute(&mut connection) + .expect("raw assignment write"); +} + +/// Set a publisher's package outside the coordinator, so a package fixture can +/// be established without moving the configuration token. +fn set_package_directly(pool: &PgPool, publisher_id: Uuid, package: ThothPackage) { + let mut connection = pool.get().expect("connection"); + sql_query(format!( + "UPDATE publisher SET subscription_package = '{package}' \ + WHERE publisher_id = '{publisher_id}'" + )) + .execute(&mut connection) + .expect("package fixture"); +} + +fn state_keys(state: &JsonValue) -> Vec { + let mut keys: Vec = state + .as_object() + .expect("audit state must be a JSON object") + .keys() + .cloned() + .collect(); + keys.sort(); + keys +} + +fn platforms_in(state: &JsonValue) -> Vec { + state["enabledDistributionPlatforms"] + .as_array() + .expect("platform array") + .iter() + .map(|value| value.as_str().expect("platform code").to_string()) + .collect() +} + +/// A guard that removes an injected failure trigger even if the test panics. +struct InjectedFailure { + pool: Arc, +} + +impl InjectedFailure { + /// Fail every insert into the configuration audit table, which happens + /// after the package update, after the lifecycle calls and after the token + /// bump, but before the coordinator's transaction commits. + fn on_audit_insert(pool: &Arc) -> Self { + let mut connection = pool.get().expect("connection"); + sql_query( + "CREATE OR REPLACE FUNCTION be03_reject_audit() RETURNS trigger AS $$ \ + BEGIN RAISE EXCEPTION 'injected pre-commit failure'; END; $$ LANGUAGE plpgsql", + ) + .execute(&mut connection) + .expect("create injection function"); + sql_query( + "CREATE TRIGGER be03_reject_audit BEFORE INSERT \ + ON publisher_service_configuration_history \ + FOR EACH ROW EXECUTE FUNCTION be03_reject_audit()", + ) + .execute(&mut connection) + .expect("create injection trigger"); + Self { + pool: Arc::clone(pool), + } + } +} + +impl Drop for InjectedFailure { + fn drop(&mut self) { + let Ok(mut connection) = self.pool.get() else { + return; + }; + let _ = sql_query( + "DROP TRIGGER IF EXISTS be03_reject_audit ON publisher_service_configuration_history", + ) + .execute(&mut connection); + let _ = sql_query("DROP FUNCTION IF EXISTS be03_reject_audit()").execute(&mut connection); + } +} + +// -------------------------------------------------------------------------- +// Normalization vocabulary +// -------------------------------------------------------------------------- + +#[test] +fn requested_platforms_are_deduplicated_and_closed_under_linked_membership() { + use DistributionPlatform::{Doab, Oapen, OclcKb, Zenodo}; + + assert_eq!(normalize_requested_platforms(&[]), Vec::new()); + assert_eq!(normalize_requested_platforms(&[Oapen]), vec![Oapen, Doab]); + assert_eq!(normalize_requested_platforms(&[Doab]), vec![Oapen, Doab]); + assert_eq!( + normalize_requested_platforms(&[Doab, Oapen, Doab]), + vec![Oapen, Doab] + ); + // Canonical `DistributionPlatform::ALL` order, not request order. + assert_eq!( + normalize_requested_platforms(&[Zenodo, OclcKb, Zenodo]), + vec![Zenodo, OclcKb] + ); +} + +// -------------------------------------------------------------------------- +// Committed change, token and audit +// -------------------------------------------------------------------------- + +#[test] +fn a_committed_change_moves_the_token_and_writes_exactly_one_audit_row() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let before = token(&pool, publisher.publisher_id); + + let configuration = replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo, DistributionPlatform::Oapen], + before, + ), + ) + .expect("replace"); + + let after = token(&pool, publisher.publisher_id); + assert!(after > before, "the token must move"); + assert_eq!( + configuration.publisher.service_configuration_updated_at, + after + ); + assert_eq!( + configuration.publisher.subscription_package, + ThothPackage::Sphinx + ); + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![ + DistributionPlatform::Oapen, + DistributionPlatform::Doab, + DistributionPlatform::Zenodo + ] + ); + + let row = only_audit_row(&pool, publisher.publisher_id); + assert_eq!(row.actor, ACTOR); + assert_eq!( + row.source, + PublisherServiceConfigurationSource::SuperuserApi + ); +} + +#[test] +fn the_audit_json_key_set_is_exactly_the_three_canonical_keys() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let before = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Oapen], + before, + ), + ) + .expect("replace"); + + let row = only_audit_row(&pool, publisher.publisher_id); + let expected = vec![ + "configurationVersion".to_string(), + "enabledDistributionPlatforms".to_string(), + "subscriptionPackage".to_string(), + ]; + // This fails if any key is ever added, including an activation identifier, + // a per-row timestamp, a capability list or any publisher metadata. + assert_eq!(state_keys(&row.before_state), expected); + assert_eq!(state_keys(&row.after_state), expected); + + let serialized = format!("{}{}", row.before_state, row.after_state); + for forbidden in [ + "activation", + "enabledAt", + "disabledAt", + "capabilit", + "zitadel", + "publisherName", + "credential", + "token", + "endpoint", + "bucket", + ] { + assert!( + !serialized.contains(forbidden), + "audit JSON must not contain `{forbidden}`: {serialized}" + ); + } +} + +#[test] +fn audit_states_record_the_canonical_before_and_after_configuration() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let first_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Doab], + first_token, + ), + ) + .expect("first replace"); + let second_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Pyramid, + &[ + DistributionPlatform::Doab, + DistributionPlatform::InternetArchive, + ], + second_token, + ), + ) + .expect("second replace"); + + let rows = audit_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 2); + let second = &rows[1]; + + assert_eq!(second.before_state["subscriptionPackage"], "OBELISK"); + assert_eq!(second.after_state["subscriptionPackage"], "PYRAMID"); + // Canonical `DistributionPlatform::ALL` order: INTERNET_ARCHIVE precedes + // OAPEN, which precedes DOAB. + assert_eq!(platforms_in(&second.before_state), vec!["OAPEN", "DOAB"]); + assert_eq!( + platforms_in(&second.after_state), + vec!["INTERNET_ARCHIVE", "OAPEN", "DOAB"] + ); + assert_eq!( + second.before_state["configurationVersion"], + serde_json::to_value(second_token).expect("token json") + ); + assert_eq!( + second.after_state["configurationVersion"], + serde_json::to_value(token(&pool, publisher.publisher_id)).expect("token json") + ); +} + +#[test] +fn one_request_touching_several_groups_writes_one_audit_row_and_one_token_bump() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let before = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[ + DistributionPlatform::Oapen, + DistributionPlatform::OclcKb, + DistributionPlatform::ExLibrisKb, + DistributionPlatform::Crossref, + ], + before, + ), + ) + .expect("replace"); + + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![ + DistributionPlatform::Oapen, + DistributionPlatform::Doab, + DistributionPlatform::Crossref, + DistributionPlatform::OclcKb, + DistributionPlatform::ExLibrisKb, + ] + ); + assert!(token(&pool, publisher.publisher_id) > before); +} + +#[test] +fn the_mutation_writes_no_publisher_history_row() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let before = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + before, + ), + ) + .expect("replace"); + + // The coordinator writes `subscription_package` directly and never through + // the shared `Crud::update` macro, so the generic entity history is not + // touched and the configuration audit is the only history BE-03 writes. + assert_eq!(publisher_history_count(&pool, publisher.publisher_id), 0); +} + +#[test] +fn the_database_rejects_an_actor_with_no_non_whitespace_character() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let mut connection = pool.get().expect("connection"); + + let insert = |connection: &mut diesel::PgConnection, actor: &str| { + diesel::insert_into(publisher_service_configuration_history::table) + .values(&NewPublisherServiceConfigurationHistory { + publisher_id: publisher.publisher_id, + actor: actor.to_string(), + source: PublisherServiceConfigurationSource::SuperuserApi, + before_state: serde_json::json!({}), + after_state: serde_json::json!({}), + }) + .execute(connection) + }; + + // The invariant is that an audit actor must contain at least one + // non-whitespace character, enforced by + // `CHECK (actor ~ '[^[:space:]]')`. The POSIX `[[:space:]]` class covers + // every case below, so each is rejected by the database itself. A narrower + // `btrim(actor) <> ''` predicate would accept everything from the tab case + // downwards, because one-argument `btrim` trims spaces only. + for (name, actor) in [ + ("empty", ""), + ("single space", " "), + ("three spaces", " "), + ("tab", "\t"), + ("newline", "\n"), + ("carriage return", "\r"), + ("vertical tab", "\u{0b}"), + ("form feed", "\u{0c}"), + ("mixed whitespace", " \t\n\r\u{0b}\u{0c} "), + ] { + let outcome = insert(&mut connection, actor); + assert!( + outcome.is_err(), + "the actor check must reject the {name} case {actor:?}" + ); + } + + // An actor carrying a real identifier is accepted even when it is + // surrounded by whitespace: the invariant is presence of a non-whitespace + // character, not absence of whitespace. + for (name, actor) in [ + ("space padded", " real-actor-42 "), + ("tab and newline padded", "\t\nreal-actor-42\r\n"), + ] { + assert!( + insert(&mut connection, actor).is_ok(), + "the actor check must accept the {name} case {actor:?}" + ); + } + + // Only the two accepted rows exist, and no BE-03 write path can produce a + // whitespace-only actor in any case: the only production writer takes it + // from `PolicyContext::user_id()`. + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 2); +} + +#[test] +fn no_be03_path_writes_a_migration_backfill_source() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let mut current = token(&pool, publisher.publisher_id); + + for (package, platforms) in [ + (ThothPackage::Obelisk, vec![DistributionPlatform::Oapen]), + (ThothPackage::Sphinx, vec![DistributionPlatform::Oapen]), + (ThothPackage::Sphinx, vec![]), + ] { + replace( + &pool, + &input(publisher.publisher_id, package, &platforms, current), + ) + .expect("replace"); + current = token(&pool, publisher.publisher_id); + } + + let rows = audit_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 3); + assert!(rows + .iter() + .all(|row| row.source == PublisherServiceConfigurationSource::SuperuserApi)); + assert!(rows + .iter() + .all(|row| row.source != PublisherServiceConfigurationSource::MigrationBackfill)); +} + +#[test] +fn an_unknown_publisher_yields_entity_not_found_and_writes_nothing() { + let (_guard, pool) = test_db::setup_test_db(); + let unknown = Uuid::new_v4(); + + let outcome = replace( + &pool, + &input( + unknown, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + Timestamp::default(), + ), + ); + + assert!(matches!(outcome, Err(ThothError::EntityNotFound))); + assert!(audit_rows(&pool, unknown).is_empty()); +} + +// -------------------------------------------------------------------------- +// True no-op, staleness and rollback +// -------------------------------------------------------------------------- + +#[test] +fn a_true_no_op_moves_no_token_and_writes_no_audit_row() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let first = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Oapen], + first, + ), + ) + .expect("seed replace"); + + let seeded = publisher_row(&pool, publisher.publisher_id); + let rows_before = all_rows(&pool, publisher.publisher_id); + + // Same package, same normalized membership, group already fully normalized. + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Doab], + seeded.service_configuration_updated_at, + ), + ) + .expect("no-op replace"); + + let after = publisher_row(&pool, publisher.publisher_id); + assert_eq!( + after.service_configuration_updated_at, + seeded.service_configuration_updated_at + ); + assert_eq!(after.updated_at, seeded.updated_at); + assert_eq!(all_rows(&pool, publisher.publisher_id), rows_before); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn a_stale_request_fails_and_writes_nothing() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let stale = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + stale, + ), + ) + .expect("first replace"); + let committed = publisher_row(&pool, publisher.publisher_id); + + let outcome = replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Pyramid, + &[DistributionPlatform::Crossref], + stale, + ), + ); + + assert!(matches!( + outcome, + Err(ThothError::StalePublisherServiceConfiguration) + )); + let after = publisher_row(&pool, publisher.publisher_id); + assert_eq!(after, committed); + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![DistributionPlatform::Zenodo] + ); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn a_stale_request_that_would_have_been_a_true_no_op_still_fails() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let stale = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + stale, + ), + ) + .expect("first replace"); + let committed = publisher_row(&pool, publisher.publisher_id); + + // Semantically identical to the committed state, but with the superseded + // token: the version check precedes everything. + let outcome = replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + stale, + ), + ); + + assert!(matches!( + outcome, + Err(ThothError::StalePublisherServiceConfiguration) + )); + assert_eq!(publisher_row(&pool, publisher.publisher_id), committed); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn a_stale_request_that_would_have_repaired_a_split_pair_still_fails() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let stale = token(&pool, publisher.publisher_id); + + // Move the token so the request below is stale, without touching the pair. + replace( + &pool, + &input(publisher.publisher_id, ThothPackage::Obelisk, &[], stale), + ) + .expect("token move"); + + write_raw_assignment( + &pool, + publisher.publisher_id, + "OAPEN", + Uuid::new_v4(), + "now()", + ); + write_raw_assignment( + &pool, + publisher.publisher_id, + "DOAB", + Uuid::new_v4(), + "now() - interval '1 hour'", + ); + let split = all_rows(&pool, publisher.publisher_id); + let committed_token = token(&pool, publisher.publisher_id); + + let outcome = replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Oapen], + stale, + ), + ); + + assert!(matches!( + outcome, + Err(ThothError::StalePublisherServiceConfiguration) + )); + assert_eq!( + all_rows(&pool, publisher.publisher_id), + split, + "the split pair must survive a stale request exactly as it was" + ); + assert_eq!(token(&pool, publisher.publisher_id), committed_token); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn an_injected_pre_commit_failure_rolls_the_whole_change_back() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + start, + ), + ) + .expect("seed replace"); + let committed = publisher_row(&pool, publisher.publisher_id); + let committed_rows = all_rows(&pool, publisher.publisher_id); + + let outcome = { + let _injection = InjectedFailure::on_audit_insert(&pool); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Pyramid, + &[DistributionPlatform::Oapen], + committed.service_configuration_updated_at, + ), + ) + }; + + assert!(outcome.is_err(), "the injected failure must propagate"); + let after = publisher_row(&pool, publisher.publisher_id); + assert_eq!( + after, committed, + "package, token and publisher.updated_at all roll back together" + ); + assert_eq!(all_rows(&pool, publisher.publisher_id), committed_rows); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +// -------------------------------------------------------------------------- +// Independence and linked platforms +// -------------------------------------------------------------------------- + +#[test] +fn a_package_only_change_leaves_every_assignment_row_byte_identical() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen, DistributionPlatform::Zenodo], + start, + ), + ) + .expect("seed replace"); + let rows_before = all_rows(&pool, publisher.publisher_id); + let seeded_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen, DistributionPlatform::Zenodo], + seeded_token, + ), + ) + .expect("package-only replace"); + + assert_eq!(all_rows(&pool, publisher.publisher_id), rows_before); + assert_eq!( + publisher_row(&pool, publisher.publisher_id).subscription_package, + ThothPackage::Sphinx + ); + assert!(token(&pool, publisher.publisher_id) > seeded_token); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 2); +} + +#[test] +fn a_platform_only_change_leaves_the_package_unchanged() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + set_package_directly(&pool, publisher.publisher_id, ThothPackage::Obelisk); + let start = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Jstor], + start, + ), + ) + .expect("platform-only replace"); + + assert_eq!( + publisher_row(&pool, publisher.publisher_id).subscription_package, + ThothPackage::Obelisk + ); + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![DistributionPlatform::Jstor] + ); +} + +#[test] +fn requesting_either_linked_member_enables_both_with_one_shared_activation() { + let (_guard, pool) = test_db::setup_test_db(); + for platform in [DistributionPlatform::Oapen, DistributionPlatform::Doab] { + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[platform], + start, + ), + ) + .expect("linked replace"); + + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 2); + assert!(rows.iter().all(|row| row.enabled)); + assert_eq!(rows[0].activation_id, rows[1].activation_id); + assert_eq!(rows[0].enabled_at, rows[1].enabled_at); + } +} + +#[test] +fn omitting_both_linked_members_disables_both() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + start, + ), + ) + .expect("seed replace"); + let seeded_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[], + seeded_token, + ), + ) + .expect("disable replace"); + + assert!(enabled(&pool, publisher.publisher_id).is_empty()); + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 2, "disabled rows are retained, never deleted"); + assert!(rows + .iter() + .all(|row| !row.enabled && row.disabled_at.is_some())); +} + +#[test] +fn an_empty_request_disables_everything_and_is_never_read_as_all() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[ + DistributionPlatform::Zenodo, + DistributionPlatform::Oapen, + DistributionPlatform::OclcKb, + ], + start, + ), + ) + .expect("seed replace"); + let seeded_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[], + seeded_token, + ), + ) + .expect("empty replace"); + + assert!(enabled(&pool, publisher.publisher_id).is_empty()); +} + +#[test] +fn duplicates_in_the_requested_list_are_deduplicated_with_no_error() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[ + DistributionPlatform::Zenodo, + DistributionPlatform::Zenodo, + DistributionPlatform::Oapen, + DistributionPlatform::Doab, + ], + start, + ), + ) + .expect("duplicate replace"); + + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![ + DistributionPlatform::Oapen, + DistributionPlatform::Doab, + DistributionPlatform::Zenodo + ] + ); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +/// Every membership-equal split state must still be repaired, must bump the +/// token and must write exactly one audit row whose two states differ only in +/// `configurationVersion`. +fn assert_membership_equal_repair(seed: impl Fn(&PgPool, Uuid), requested: DistributionPlatform) { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + seed(&pool, publisher.publisher_id); + let before_token = token(&pool, publisher.publisher_id); + let membership_before = enabled(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[requested], + before_token, + ), + ) + .expect("repair replace"); + + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 2); + assert!(rows + .iter() + .all(|row| row.enabled && row.disabled_at.is_none())); + assert_eq!(rows[0].activation_id, rows[1].activation_id); + assert_eq!(rows[0].enabled_at, rows[1].enabled_at); + + let after_token = token(&pool, publisher.publisher_id); + assert!(after_token > before_token, "a repair is a committed change"); + + let row = only_audit_row(&pool, publisher.publisher_id); + assert_eq!( + row.before_state["subscriptionPackage"], + row.after_state["subscriptionPackage"] + ); + if membership_before.len() == 2 { + assert_eq!( + platforms_in(&row.before_state), + platforms_in(&row.after_state), + "a membership-equal repair differs only in configurationVersion" + ); + } + assert_ne!( + row.before_state["configurationVersion"], + row.after_state["configurationVersion"] + ); + assert_eq!( + row.before_state["configurationVersion"], + serde_json::to_value(before_token).expect("token json") + ); + assert_eq!( + row.after_state["configurationVersion"], + serde_json::to_value(after_token).expect("token json") + ); +} + +#[test] +fn a_split_activation_pair_is_repaired_although_membership_is_unchanged() { + assert_membership_equal_repair( + |pool, publisher_id| { + let enabled_at = "now()"; + write_raw_assignment(pool, publisher_id, "OAPEN", Uuid::new_v4(), enabled_at); + write_raw_assignment(pool, publisher_id, "DOAB", Uuid::new_v4(), enabled_at); + }, + DistributionPlatform::Oapen, + ); +} + +#[test] +fn a_split_enabled_at_pair_is_repaired_although_membership_is_unchanged() { + assert_membership_equal_repair( + |pool, publisher_id| { + let activation = Uuid::new_v4(); + write_raw_assignment(pool, publisher_id, "OAPEN", activation, "now()"); + write_raw_assignment( + pool, + publisher_id, + "DOAB", + activation, + "now() - interval '2 hours'", + ); + }, + DistributionPlatform::Doab, + ); +} + +#[test] +fn a_one_sided_pair_is_repaired_whichever_member_the_request_names() { + for requested in [DistributionPlatform::Oapen, DistributionPlatform::Doab] { + assert_membership_equal_repair( + |pool, publisher_id| { + write_raw_assignment(pool, publisher_id, "OAPEN", Uuid::new_v4(), "now()"); + }, + requested, + ); + } +} + +#[test] +fn a_fully_normalized_pair_with_the_same_request_is_a_true_no_op() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + start, + ), + ) + .expect("seed replace"); + let rows_before = all_rows(&pool, publisher.publisher_id); + let seeded = publisher_row(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + seeded.service_configuration_updated_at, + ), + ) + .expect("no-op replace"); + + assert_eq!(all_rows(&pool, publisher.publisher_id), rows_before); + assert_eq!(publisher_row(&pool, publisher.publisher_id), seeded); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn a_package_change_over_a_split_group_writes_the_package_and_repairs_the_group() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + write_raw_assignment( + &pool, + publisher.publisher_id, + "OAPEN", + Uuid::new_v4(), + "now()", + ); + write_raw_assignment( + &pool, + publisher.publisher_id, + "DOAB", + Uuid::new_v4(), + "now()", + ); + let before = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen], + before, + ), + ) + .expect("replace"); + + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows[0].activation_id, rows[1].activation_id); + assert_eq!( + publisher_row(&pool, publisher.publisher_id).subscription_package, + ThothPackage::Sphinx + ); + assert!(token(&pool, publisher.publisher_id) > before); + assert_eq!( + audit_rows(&pool, publisher.publisher_id).len(), + 1, + "one audit row for the whole change" + ); +} + +#[test] +fn a_no_op_group_with_a_package_change_leaves_assignment_rows_byte_identical() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + start, + ), + ) + .expect("seed replace"); + let rows_before = all_rows(&pool, publisher.publisher_id); + let seeded_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Pyramid, + &[DistributionPlatform::Doab], + seeded_token, + ), + ) + .expect("package replace"); + + assert_eq!(all_rows(&pool, publisher.publisher_id), rows_before); + assert!(token(&pool, publisher.publisher_id) > seeded_token); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 2); +} + +#[test] +fn oclc_and_ex_libris_remain_independently_configurable() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[ + DistributionPlatform::OclcKb, + DistributionPlatform::ExLibrisKb, + ], + start, + ), + ) + .expect("seed replace"); + let rows = all_rows(&pool, publisher.publisher_id); + assert_ne!( + rows[0].activation_id, rows[1].activation_id, + "unlinked platforms receive independent activations" + ); + let seeded_token = token(&pool, publisher.publisher_id); + + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::OclcKb], + seeded_token, + ), + ) + .expect("partial disable"); + + assert_eq!( + enabled(&pool, publisher.publisher_id), + vec![DistributionPlatform::OclcKb] + ); +} + +#[test] +fn requesting_jisc_nbk_fails_before_any_write() { + let (_guard, pool) = test_db::setup_test_db(); + + // (a) A request that would otherwise have changed nothing. + let quiet = test_db::create_publisher(&pool); + let quiet_before = publisher_row(&pool, quiet.publisher_id); + let outcome = replace( + &pool, + &input( + quiet.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::JiscNbk], + quiet_before.service_configuration_updated_at, + ), + ); + assert!(matches!( + outcome, + Err(ThothError::DistributionPlatformNotAssignable(_)) + )); + assert_eq!(publisher_row(&pool, quiet.publisher_id), quiet_before); + assert!(all_rows(&pool, quiet.publisher_id).is_empty()); + assert!(audit_rows(&pool, quiet.publisher_id).is_empty()); + + // (b) A request that would otherwise have changed the package and enabled a + // valid platform: pre-validation of the whole set precedes the first + // lifecycle call, so nothing is written and nothing relies on rollback. + let busy = test_db::create_publisher(&pool); + let busy_before = publisher_row(&pool, busy.publisher_id); + let outcome = replace( + &pool, + &input( + busy.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo, DistributionPlatform::JiscNbk], + busy_before.service_configuration_updated_at, + ), + ); + assert!(matches!( + outcome, + Err(ThothError::DistributionPlatformNotAssignable(_)) + )); + assert_eq!(publisher_row(&pool, busy.publisher_id), busy_before); + assert!(all_rows(&pool, busy.publisher_id).is_empty()); + assert!(audit_rows(&pool, busy.publisher_id).is_empty()); +} + +// -------------------------------------------------------------------------- +// Concurrency +// -------------------------------------------------------------------------- + +/// Two clients holding the same token both submit; exactly one commits. +fn concurrent_replacements( + pool: &Arc, + publisher_id: Uuid, + first: ReplacePublisherServiceConfigurationInput, + second: ReplacePublisherServiceConfigurationInput, +) -> ( + ThothResult, + ThothResult, +) { + let one = { + let pool = Arc::clone(pool); + std::thread::spawn(move || replace(&pool, &first)) + }; + let two = { + let pool = Arc::clone(pool); + std::thread::spawn(move || replace(&pool, &second)) + }; + let _ = publisher_id; + ( + one.join().expect("thread one"), + two.join().expect("thread two"), + ) +} + +fn assert_one_winner_one_stale( + outcomes: ( + ThothResult, + ThothResult, + ), +) { + let (first, second) = outcomes; + let winners = usize::from(first.is_ok()) + usize::from(second.is_ok()); + assert_eq!(winners, 1, "exactly one client may commit"); + for outcome in [first, second] { + if let Err(error) = outcome { + assert!( + matches!(error, ThothError::StalePublisherServiceConfiguration), + "the loser must fail as stale, got {error:?}" + ); + } + } +} + +#[test] +fn two_clients_holding_one_token_produce_one_winner_and_one_stale_loser() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let shared = token(&pool, publisher.publisher_id); + + let outcomes = concurrent_replacements( + &pool, + publisher.publisher_id, + input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + shared, + ), + input( + publisher.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Crossref], + shared, + ), + ); + assert_one_winner_one_stale(outcomes); + + // Exactly one committed change: the loser wrote nothing. + let rows = audit_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 1); + let committed = publisher_row(&pool, publisher.publisher_id); + let enabled_now = enabled(&pool, publisher.publisher_id); + assert_eq!( + platforms_in(&rows[0].after_state), + enabled_now + .iter() + .map(|platform| platform.to_string()) + .collect::>(), + "the final state is exactly what the winner committed" + ); + assert_eq!( + rows[0].after_state["subscriptionPackage"], + JsonValue::from(committed.subscription_package.to_string()) + ); +} + +#[test] +fn concurrent_linked_replacements_leave_no_one_sided_pair() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let shared = token(&pool, publisher.publisher_id); + + let outcomes = concurrent_replacements( + &pool, + publisher.publisher_id, + input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + shared, + ), + input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Doab], + shared, + ), + ); + assert_one_winner_one_stale(outcomes); + + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows.len(), 2); + assert!(rows.iter().all(|row| row.enabled)); + assert_eq!(rows[0].activation_id, rows[1].activation_id); + assert_eq!(rows[0].enabled_at, rows[1].enabled_at); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn two_concurrent_membership_equal_repairs_produce_one_repair_and_one_stale() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + write_raw_assignment( + &pool, + publisher.publisher_id, + "OAPEN", + Uuid::new_v4(), + "now()", + ); + write_raw_assignment( + &pool, + publisher.publisher_id, + "DOAB", + Uuid::new_v4(), + "now()", + ); + let shared = token(&pool, publisher.publisher_id); + + let outcomes = concurrent_replacements( + &pool, + publisher.publisher_id, + input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + shared, + ), + input( + publisher.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Doab], + shared, + ), + ); + assert_one_winner_one_stale(outcomes); + + let rows = all_rows(&pool, publisher.publisher_id); + assert_eq!(rows[0].activation_id, rows[1].activation_id); + assert_eq!(rows[0].enabled_at, rows[1].enabled_at); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); +} + +#[test] +fn concurrent_replacements_for_different_publishers_do_not_contend() { + let (_guard, pool) = test_db::setup_test_db(); + let first = test_db::create_publisher(&pool); + let second = test_db::create_publisher(&pool); + let first_token = token(&pool, first.publisher_id); + let second_token = token(&pool, second.publisher_id); + + let outcomes = concurrent_replacements( + &pool, + first.publisher_id, + input( + first.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + first_token, + ), + input( + second.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Crossref], + second_token, + ), + ); + + assert!(outcomes.0.is_ok() && outcomes.1.is_ok(), "both must commit"); + assert_eq!(audit_rows(&pool, first.publisher_id).len(), 1); + assert_eq!(audit_rows(&pool, second.publisher_id).len(), 1); +} + +#[test] +fn a_replacement_concurrent_with_a_direct_be02_transition_serializes_without_deadlock() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + PublisherDistributionPlatform::enable( + &pool, + publisher.publisher_id, + DistributionPlatform::ProjectMuse, + ) + .expect("seed enable"); + let current = token(&pool, publisher.publisher_id); + + let coordinator = { + let pool = Arc::clone(&pool); + let publisher_id = publisher.publisher_id; + std::thread::spawn(move || { + replace( + &pool, + &input( + publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + current, + ), + ) + }) + }; + let direct = { + let pool = Arc::clone(&pool); + let publisher_id = publisher.publisher_id; + std::thread::spawn(move || { + PublisherDistributionPlatform::disable( + &pool, + publisher_id, + DistributionPlatform::ProjectMuse, + ) + }) + }; + + coordinator + .join() + .expect("coordinator thread") + .expect("coordinator replace"); + direct + .join() + .expect("direct thread") + .expect("direct disable"); + + // Both took the same publisher row lock, so they serialized. Every row is + // internally consistent whichever order they ran in. + for row in all_rows(&pool, publisher.publisher_id) { + assert_eq!(row.enabled, row.disabled_at.is_none()); + } +} + +#[test] +fn the_token_is_strictly_monotonic_per_publisher_across_a_sequence_with_a_repair() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let mut observed = vec![token(&pool, publisher.publisher_id)]; + + // Ordinary committed changes. + for (package, platforms) in [ + (ThothPackage::Obelisk, vec![DistributionPlatform::Oapen]), + (ThothPackage::Sphinx, vec![DistributionPlatform::Oapen]), + ] { + let current = *observed.last().expect("token"); + replace( + &pool, + &input(publisher.publisher_id, package, &platforms, current), + ) + .expect("replace"); + observed.push(token(&pool, publisher.publisher_id)); + } + + // A membership-equal repair is also a committed change. + write_raw_assignment( + &pool, + publisher.publisher_id, + "DOAB", + Uuid::new_v4(), + "now() - interval '1 hour'", + ); + let current = *observed.last().expect("token"); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen], + current, + ), + ) + .expect("repair replace"); + observed.push(token(&pool, publisher.publisher_id)); + + for pair in observed.windows(2) { + assert!( + pair[1] > pair[0], + "the token must be strictly increasing: {pair:?}" + ); + } + let unique: HashSet = observed.iter().map(|value| value.to_rfc3339()).collect(); + assert_eq!(unique.len(), observed.len()); +} + +// -------------------------------------------------------------------------- +// Publisher and work trigger cascade (specification sections 6.4 and 18.4) +// -------------------------------------------------------------------------- + +struct CascadeFixture { + publisher_id: Uuid, + target_work_ids: Vec, + control_work_id: Uuid, +} + +/// One target publisher with two imprints and two works across them, plus a +/// control work belonging to a different publisher, so the trigger's +/// `work -> imprint -> publisher` join is genuinely traversed and its bound is +/// proven rather than assumed. +fn cascade_fixture(pool: &PgPool) -> CascadeFixture { + let publisher = test_db::create_publisher(pool); + let first_imprint = test_db::create_imprint(pool, &publisher); + let second_imprint = test_db::create_imprint(pool, &publisher); + let first_work = test_db::create_work(pool, &first_imprint); + let second_work = test_db::create_work(pool, &second_imprint); + + let other_publisher = test_db::create_publisher(pool); + let other_imprint = test_db::create_imprint(pool, &other_publisher); + let control_work = test_db::create_work(pool, &other_imprint); + + CascadeFixture { + publisher_id: publisher.publisher_id, + target_work_ids: vec![first_work.work_id, second_work.work_id], + control_work_id: control_work.work_id, + } +} + +fn work_freshness(pool: &PgPool, work_id: Uuid) -> Timestamp { + let mut connection = pool.get().expect("connection"); + work::table + .filter(work::work_id.eq(work_id)) + .select(work::updated_at_with_relations) + .first::(&mut connection) + .expect("work freshness") +} + +struct CascadeSnapshot { + configuration_token: Timestamp, + publisher_updated_at: Timestamp, + target_freshness: Vec, + control_freshness: Timestamp, +} + +fn cascade_snapshot(pool: &PgPool, fixture: &CascadeFixture) -> CascadeSnapshot { + let publisher = publisher_row(pool, fixture.publisher_id); + CascadeSnapshot { + configuration_token: publisher.service_configuration_updated_at, + publisher_updated_at: publisher.updated_at, + target_freshness: fixture + .target_work_ids + .iter() + .map(|work_id| work_freshness(pool, *work_id)) + .collect(), + control_freshness: work_freshness(pool, fixture.control_work_id), + } +} + +fn assert_cascade(before: &CascadeSnapshot, after: &CascadeSnapshot, moved: bool) { + if moved { + assert!( + after.configuration_token > before.configuration_token, + "the configuration token must move" + ); + assert!( + after.publisher_updated_at > before.publisher_updated_at, + "publisher.updated_at must move" + ); + for (index, (before_value, after_value)) in before + .target_freshness + .iter() + .zip(after.target_freshness.iter()) + .enumerate() + { + assert!( + after_value > before_value, + "target work {index} freshness must move" + ); + } + } else { + assert_eq!(after.configuration_token, before.configuration_token); + assert_eq!(after.publisher_updated_at, before.publisher_updated_at); + assert_eq!(after.target_freshness, before.target_freshness); + } + assert_eq!( + after.control_freshness, before.control_freshness, + "a work of another publisher must never move" + ); +} + +#[test] +fn a_committed_package_only_change_moves_all_three_timestamps() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + let before = cascade_snapshot(&pool, &fixture); + + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Sphinx, + &[], + before.configuration_token, + ), + ) + .expect("package-only replace"); + + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), true); +} + +#[test] +fn a_committed_platform_only_change_moves_all_three_timestamps() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + let before = cascade_snapshot(&pool, &fixture); + + // Merged BE-02 alone would have moved neither publisher.updated_at nor any + // work freshness value for this change. BE-03 does, because the same + // transaction writes the configuration token to the publisher row. This is + // asserted deliberately so a later reader does not mistake it for a defect. + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Zenodo], + before.configuration_token, + ), + ) + .expect("platform-only replace"); + + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), true); +} + +#[test] +fn a_committed_linked_repair_moves_all_three_timestamps() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + write_raw_assignment( + &pool, + fixture.publisher_id, + "OAPEN", + Uuid::new_v4(), + "now()", + ); + write_raw_assignment(&pool, fixture.publisher_id, "DOAB", Uuid::new_v4(), "now()"); + let before = cascade_snapshot(&pool, &fixture); + + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Oasis, + &[DistributionPlatform::Oapen], + before.configuration_token, + ), + ) + .expect("repair replace"); + + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), true); +} + +#[test] +fn a_true_no_op_moves_no_timestamp_anywhere() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + let seed_token = token(&pool, fixture.publisher_id); + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + seed_token, + ), + ) + .expect("seed replace"); + let before = cascade_snapshot(&pool, &fixture); + + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + before.configuration_token, + ), + ) + .expect("no-op replace"); + + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), false); +} + +#[test] +fn a_stale_request_moves_no_timestamp_anywhere() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + let stale = token(&pool, fixture.publisher_id); + replace( + &pool, + &input(fixture.publisher_id, ThothPackage::Obelisk, &[], stale), + ) + .expect("seed replace"); + let before = cascade_snapshot(&pool, &fixture); + + let outcome = replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + stale, + ), + ); + + assert!(matches!( + outcome, + Err(ThothError::StalePublisherServiceConfiguration) + )); + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), false); +} + +#[test] +fn a_rolled_back_transaction_moves_no_timestamp_anywhere() { + let (_guard, pool) = test_db::setup_test_db(); + let fixture = cascade_fixture(&pool); + let before = cascade_snapshot(&pool, &fixture); + + let outcome = { + let _injection = InjectedFailure::on_audit_insert(&pool); + replace( + &pool, + &input( + fixture.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + before.configuration_token, + ), + ) + }; + + assert!(outcome.is_err()); + assert_cascade(&before, &cascade_snapshot(&pool, &fixture), false); +} + +// -------------------------------------------------------------------------- +// Staff report +// -------------------------------------------------------------------------- + +fn summaries( + pool: &PgPool, + publishers: Vec, + packages: Vec, + enabled_platforms: Vec, +) -> Vec { + PublisherServiceConfiguration::all_summaries( + pool, + 100, + 0, + PublisherOrderBy::default(), + publishers, + packages, + enabled_platforms, + ) + .expect("report") +} + +#[test] +fn the_report_filters_by_publisher_package_and_enabled_platforms() { + let (_guard, pool) = test_db::setup_test_db(); + let first = test_db::create_publisher(&pool); + let second = test_db::create_publisher(&pool); + let third = test_db::create_publisher(&pool); + + let first_token = token(&pool, first.publisher_id); + replace( + &pool, + &input( + first.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Oapen, DistributionPlatform::Zenodo], + first_token, + ), + ) + .expect("first replace"); + let second_token = token(&pool, second.publisher_id); + replace( + &pool, + &input( + second.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + second_token, + ), + ) + .expect("second replace"); + + let all = summaries(&pool, vec![], vec![], vec![]); + assert_eq!(all.len(), 3, "three publishers exist"); + + let by_publisher = summaries(&pool, vec![third.publisher_id], vec![], vec![]); + assert_eq!(by_publisher.len(), 1); + assert_eq!( + by_publisher[0].configuration.publisher_id(), + third.publisher_id + ); + + let by_package = summaries(&pool, vec![], vec![ThothPackage::Sphinx], vec![]); + assert_eq!(by_package.len(), 2); + + // AND semantics: both platforms must be enabled. + let both = summaries( + &pool, + vec![], + vec![], + vec![DistributionPlatform::Zenodo, DistributionPlatform::Oapen], + ); + assert_eq!(both.len(), 1); + assert_eq!(both[0].configuration.publisher_id(), first.publisher_id); + + let single = summaries(&pool, vec![], vec![], vec![DistributionPlatform::Zenodo]); + assert_eq!(single.len(), 2); + + assert_eq!( + PublisherServiceConfiguration::count( + &pool, + vec![], + vec![], + vec![DistributionPlatform::Zenodo, DistributionPlatform::Oapen] + ) + .expect("count"), + 1, + "the count query applies the same predicates as the list query" + ); + assert_eq!( + PublisherServiceConfiguration::count(&pool, vec![], vec![], vec![]).expect("count"), + 3 + ); +} + +#[test] +fn the_report_reports_the_latest_change_or_null() { + let (_guard, pool) = test_db::setup_test_db(); + let changed = test_db::create_publisher(&pool); + let untouched = test_db::create_publisher(&pool); + + let start = token(&pool, changed.publisher_id); + replace( + &pool, + &input( + changed.publisher_id, + ThothPackage::Obelisk, + &[DistributionPlatform::Zenodo], + start, + ), + ) + .expect("first replace"); + let second = token(&pool, changed.publisher_id); + replace( + &pool, + &input( + changed.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + second, + ), + ) + .expect("second replace"); + + let report = summaries(&pool, vec![], vec![], vec![]); + let for_changed = report + .iter() + .find(|summary| summary.configuration.publisher_id() == changed.publisher_id) + .expect("changed publisher"); + let last = for_changed.last_change.as_ref().expect("last change"); + assert_eq!(last.actor, ACTOR); + assert_eq!( + last.source, + PublisherServiceConfigurationSource::SuperuserApi + ); + let rows = audit_rows(&pool, changed.publisher_id); + assert_eq!(rows.len(), 2); + assert_eq!(last.changed_at, rows[1].created_at); + + let for_untouched = report + .iter() + .find(|summary| summary.configuration.publisher_id() == untouched.publisher_id) + .expect("untouched publisher"); + assert!( + for_untouched.last_change.is_none(), + "a publisher with no recorded change reports null, never a placeholder" + ); +} + +#[test] +fn the_report_orders_deterministically_with_a_publisher_id_tie_breaker() { + let (_guard, pool) = test_db::setup_test_db(); + // `publisher_uniq_idx` is a unique index on `lower(publisher_name)`, so + // literally duplicate names cannot exist. The equivalent ordering tie is + // produced on a nullable sort field: every fixture publisher has a NULL + // shortname, so only the mandatory `publisher_id ASC` tie-breaker can make + // offset pagination deterministic. + for _ in 0..12 { + let publisher = test_db::create_publisher(&pool); + assert!(publisher.publisher_shortname.is_none()); + } + + let mut paged: Vec = Vec::new(); + for offset in [0, 5, 10] { + let page = PublisherServiceConfiguration::all_summaries( + &pool, + 5, + offset, + PublisherOrderBy { + field: PublisherField::PublisherShortname, + direction: crate::graphql::types::inputs::Direction::Asc, + }, + vec![], + vec![], + vec![], + ) + .expect("page"); + paged.extend( + page.iter() + .map(|summary| summary.configuration.publisher_id()), + ); + } + + assert_eq!(paged.len(), 12, "offset pagination must not skip or repeat"); + let unique: HashSet = paged.iter().copied().collect(); + assert_eq!(unique.len(), 12); + let mut sorted = paged.clone(); + sorted.sort(); + assert_eq!(paged, sorted, "the publisher_id tie-breaker is ascending"); +} + +// -------------------------------------------------------------------------- +// Migrated database contract +// -------------------------------------------------------------------------- + +#[derive(diesel::QueryableByName)] +struct CatalogText { + #[diesel(sql_type = diesel::sql_types::Text)] + value: String, +} + +fn catalog_values(pool: &PgPool, query: &str) -> Vec { + let mut connection = pool.get().expect("connection"); + sql_query(query) + .load::(&mut connection) + .expect("catalog query") + .into_iter() + .map(|row| row.value) + .collect() +} + +#[test] +fn the_migrated_database_matches_the_schema_contract() { + let (_guard, pool) = test_db::setup_test_db(); + + assert_eq!( + catalog_values( + &pool, + "SELECT e.enumlabel AS value FROM pg_type t JOIN pg_enum e ON e.enumtypid = t.oid \ + WHERE t.typname = 'publisher_service_configuration_source' ORDER BY e.enumsortorder" + ), + vec!["SUPERUSER_API", "MIGRATION_BACKFILL"] + ); + + assert_eq!( + catalog_values( + &pool, + "SELECT conname AS value FROM pg_constraint \ + WHERE conrelid = 'public.publisher_service_configuration_history'::regclass \ + ORDER BY conname" + ), + vec![ + "publisher_service_configuration_history_actor_check", + "publisher_service_configuration_history_pkey", + "publisher_service_configuration_history_publisher_id_fkey", + ] + ); + + // The actor constraint's catalog *definition*, not merely its name: the + // invariant is that an actor contains at least one non-whitespace + // character, so a narrower `btrim` predicate under the same name must fail + // this assertion. + assert_eq!( + catalog_values( + &pool, + "SELECT pg_get_constraintdef(oid) AS value FROM pg_constraint \ + WHERE conname = 'publisher_service_configuration_history_actor_check'" + ), + vec!["CHECK ((actor ~ '[^[:space:]]'::text))"] + ); + + assert_eq!( + catalog_values( + &pool, + "SELECT indexname AS value FROM pg_indexes \ + WHERE tablename = 'publisher_service_configuration_history' ORDER BY indexname" + ), + vec![ + "publisher_service_configuration_history_pkey", + "publisher_service_configuration_history_publisher_created_idx", + ] + ); + + assert_eq!( + catalog_values( + &pool, + "SELECT column_name AS value FROM information_schema.columns \ + WHERE table_name = 'publisher' AND column_name = 'service_configuration_updated_at'" + ), + vec!["service_configuration_updated_at"] + ); + + // No capability state and no job table is created anywhere by BE-03. + assert!(catalog_values( + &pool, + "SELECT table_name AS value FROM information_schema.tables \ + WHERE table_schema = 'public' AND (table_name ILIKE '%capabilit%' OR table_name LIKE '%job%')" + ) + .is_empty()); + assert!(catalog_values( + &pool, + "SELECT column_name AS value FROM information_schema.columns \ + WHERE table_schema = 'public' AND column_name ILIKE '%capabilit%'" + ) + .is_empty()); + + // The audit table is append-only: no `updated_at` column, so no + // `diesel_manage_updated_at` trigger. + assert!(catalog_values( + &pool, + "SELECT tgname AS value FROM pg_trigger \ + WHERE tgrelid = 'public.publisher_service_configuration_history'::regclass \ + AND NOT tgisinternal" + ) + .is_empty()); +} + +#[test] +fn deleting_a_publisher_cascades_to_its_configuration_audit() { + let (_guard, pool) = test_db::setup_test_db(); + let publisher = test_db::create_publisher(&pool); + let start = token(&pool, publisher.publisher_id); + replace( + &pool, + &input( + publisher.publisher_id, + ThothPackage::Sphinx, + &[DistributionPlatform::Zenodo], + start, + ), + ) + .expect("replace"); + assert_eq!(audit_rows(&pool, publisher.publisher_id).len(), 1); + + let mut connection = pool.get().expect("connection"); + diesel::delete(publisher::table.filter(publisher::publisher_id.eq(publisher.publisher_id))) + .execute(&mut connection) + .expect("delete publisher"); + + assert!(audit_rows(&pool, publisher.publisher_id).is_empty()); +} diff --git a/thoth-api/src/schema.rs b/thoth-api/src/schema.rs index 52a39fb2..78e632be 100644 --- a/thoth-api/src/schema.rs +++ b/thoth-api/src/schema.rs @@ -94,6 +94,10 @@ pub mod sql_types { #[derive(diesel::sql_types::SqlType, diesel::query_builder::QueryId)] #[diesel(postgres_type(name = "distribution_platform"))] pub struct DistributionPlatform; + + #[derive(diesel::sql_types::SqlType, diesel::query_builder::QueryId)] + #[diesel(postgres_type(name = "publisher_service_configuration_source"))] + pub struct PublisherServiceConfigurationSource; } use diesel::{allow_tables_to_appear_in_same_query, joinable, table}; @@ -623,6 +627,7 @@ table! { subscription_package -> ThothPackage, created_at -> Timestamptz, updated_at -> Timestamptz, + service_configuration_updated_at -> Timestamptz, } } @@ -642,6 +647,21 @@ table! { } } +table! { + use diesel::sql_types::*; + use super::sql_types::PublisherServiceConfigurationSource; + + publisher_service_configuration_history (publisher_service_configuration_history_id) { + publisher_service_configuration_history_id -> Uuid, + publisher_id -> Uuid, + actor -> Text, + source -> PublisherServiceConfigurationSource, + before_state -> Jsonb, + after_state -> Jsonb, + created_at -> Timestamptz, + } +} + table! { use diesel::sql_types::*; @@ -1005,6 +1025,7 @@ joinable!(publication -> work (work_id)); joinable!(publication_history -> publication (publication_id)); joinable!(publisher_distribution_platform -> publisher (publisher_id)); joinable!(publisher_history -> publisher (publisher_id)); +joinable!(publisher_service_configuration_history -> publisher (publisher_id)); joinable!(reference -> work (work_id)); joinable!(reference_history -> reference (reference_id)); joinable!(series -> imprint (imprint_id)); @@ -1062,6 +1083,7 @@ allow_tables_to_appear_in_same_query!( publisher, publisher_distribution_platform, publisher_history, + publisher_service_configuration_history, reference, reference_history, series, diff --git a/thoth-errors/src/lib.rs b/thoth-errors/src/lib.rs index 2c18d505..8f3ad4e6 100644 --- a/thoth-errors/src/lib.rs +++ b/thoth-errors/src/lib.rs @@ -168,6 +168,17 @@ pub enum ThothError { UpdateLocationChecksumError, #[error("{0} is not currently available for publisher distribution assignment.")] DistributionPlatformNotAssignable(String), + /// The caller's `expectedUpdatedAt` did not match the stored publisher + /// service-configuration version. + /// + /// The message deliberately carries no SQL, table name, column name, driver + /// text or the current stored token: disclosing the current version to a + /// caller that has just failed a version check would let it blind-write over + /// a change it never read. The caller re-reads the configuration instead. + #[error( + "The publisher service configuration changed since it was read. Reload it and try again." + )] + StalePublisherServiceConfiguration, } impl ThothError { @@ -198,6 +209,12 @@ impl juniper::IntoFieldError for ThothError { "type": "NO_ACCESS" }), ), + ThothError::StalePublisherServiceConfiguration => juniper::FieldError::new( + self.to_string(), + graphql_value!({ + "type": "STALE_SERVICE_CONFIGURATION" + }), + ), _ => juniper::FieldError::new( self.to_string(), graphql_value!({