feat(publisher-services): implement BE-02 distribution platforms - #805
Conversation
… model Add the inactive additive persistence foundation for publisher distribution-platform configuration, per the approved BE-02 specification and ADR-0004's repository-authoritative inventory. - PostgreSQL enum `public.distribution_platform` with the 17 approved labels in canonical declaration order, and the additive `publisher_distribution_platform` relation with composite primary key `(publisher_id, platform)`, `ON DELETE CASCADE` foreign key to `publisher`, the named `enabled == (disabled_at IS NULL)` check constraint, the partial enabled index and the standard `set_updated_at` trigger. The migration inserts no data. - `thoth-api/src/schema.rs` updated manually and atomically in the same bounded change, per ADR-0003 Architecture A. No Diesel CLI, `diesel.toml` or schema-synchronisation subsystem is introduced. - The closed `DistributionPlatform` enum with no `OTHER`, `UNKNOWN`, `PROVISIONAL`, fallback or `Default` variant, and no shared enum or conversion with Thoth Metrics, plus code-owned compile-time-exhaustive descriptors. `AssignmentAvailability`, `MechanismReadiness` and `DistributionAdapterProfile` stay internal Rust vocabulary. - The six-transition assignment lifecycle: disabled rows are retained, a genuine re-enable generates a new application-side activation UUID, a same-state operation writes nothing and moves no timestamp, and `ABSENT -> DISABLED` creates no never-activated row. Every transition runs in one transaction that first locks the publisher row. - Atomic OAPEN/DOAB linked normalization: enabling is a complete no-op only when both rows exist, are enabled and share one `activation_id` and one `enabled_at`; a one-sided, split-activation or split-timestamp pair is repaired to one new shared activation instead. - `OCLC_KB` and `EX_LIBRIS_KB` keep independent assignments and activations while sharing an internal feed profile. `JISC_NBK` is included but inactive and non-assignable, failing closed before any write through the new stable `ThothError::DistributionPlatformNotAssignable`. `Crud` is deliberately not implemented for the assignment entity: BE-02 adds no generic CRUD mutation surface, creates no distribution job, performs no dissemination and activates no destination.
…oader Add the four approved additive public read surfaces and adopt the merged ADR-0007 request-local non-cached DataLoader foundation for the first time in a production field. Public contract (exactly the approved additive inventory): - `distributionPlatformOptions` returns the 17 code-owned descriptors in canonical order with no arguments and no database access; - `publishersByDistributionPlatform` joins enabled assignment rows only, in one set-based query, with the requested publisher order plus a mandatory `publisher_id ASC` tie-breaker so pagination is deterministic; - `publisherCountByDistributionPlatform` counts exactly that population; - `Publisher.distributionPlatforms` returns enabled assignments only, in canonical platform order, and an empty list when there are none. Two new object types and three new enums are added; no mutation, input, scalar or interface is. Activation IDs, disabled history, adapter and feed identity, package or capability state and protected BE-03 configuration are not exposed. All four surfaces are intentionally public, matching the current read architecture, and add no new authorization branch. DataLoader adoption: - one typed loader added to the existing request-local `RequestLoaders` bundle rather than a second batching subsystem; - key is exactly `publisher_id`; the field takes no result-changing argument, so no second key dimension exists; - built through `configured_loader`, so the explicit max batch size 200 and yield count 10 cannot fall back to crate defaults; - loader-first: the resolver registers its key at entry, with no unrelated awaited work before `try_load`; - `try_load` only, and batches are total over their keys — zero enabled assignments load a successful empty vector, and a backend failure produces an error for every key in the failed chunk with no retry and no per-parent fallback; - one set-based `eq_any` statement per dispatch chunk, executed entirely inside `tokio::task::spawn_blocking`, with the Diesel connection acquired, used and dropped inside the closure and never held across an `.await`; - the field's conventional message-only Juniper error shape is preserved through the existing non-serde `SharedBatchError` projection. No global or static loader, no cross-request cache, no ADR-0006 A2 machinery and no mutation-guard coupling is introduced.
Add the CHANGELOG entry for implementation PR #805, reconcile the Publisher Services tracker to the actual live state, and add the BE-02 implementation report. The tracker's previous "BLOCKED - IMPLEMENTATION NOT AUTHORIZED" wording is now stale: the reconciled specification was independently reviewed, CTO-approved and merged through PR #788, and implementation was then separately authorized against that exact `develop` SHA. BE-02 moves to "IMPLEMENTED - AWAITING INDEPENDENT REVIEW / MERGE AUTHORIZATION" — not complete, because fresh independent exact-head review and separate CTO merge authorization are both still outstanding. The report records the preflight, the exact commands and their concise results, empty and representative-populated migration evidence, empirical `pg_locks` observation on a disposable PostgreSQL 17.10 database, the base-versus-head SDL diff, the DataLoader contract, the real-SQL query-count evidence for both parent shapes and every required boundary, and the known limitations — including the two places where the repository's own facts shaped how a specification requirement could be evidenced. Implementation remains an inactive foundation. No deployment, production migration, backfill, distribution activation or guard-mode change is performed or authorized, and PR #799 is untouched.
Exact-head CI evidenceHead: Every workflow and every job is recorded individually. Workflow success is not taken as
CI is not approvalThis PR still requires fresh independent exact-head review by an agent/model that did not |
Independent review provenance + CTO merge authorizationIndependent exact-head implementation reviewA fresh independent Claude review session that did not implement, author, remediate, or materially assist with BE-02 reviewed PR #805 at exact head The independent decision was: The reviewer independently re-verified repository authority, the complete diff, migration/catalog shape, PostgreSQL 17.10 lock behaviour on a disposable database, populated-data preservation, all six lifecycle transitions, OAPEN/DOAB normalization and concurrency, GraphQL SDL/client compatibility, ADR-0007 DataLoader architecture and loader-first behaviour, real SQL query-count evidence, failure semantics, workspace tests, and exact-head CI. Final reviewer statement: the implementation at this exact head is suitable for CTO merge authorization; any subsequent commit invalidates the review. Provenance note: this comment records the decision produced by the separate independent review session. It is not a GitHub CTO merge authorizationThe CTO has explicitly authorized:
Binding boundaryThe merge authorization is valid only for exact head The merge does not authorize deployment, environment or production migration execution, assignment creation/backfill, distribution activation, |
|
Pre-merge control checkpoint: after recording independent-review provenance and CTO merge authorization, PR #805 remains OPEN / DRAFT / UNMERGED / mergeable at exact head |
|
State transition note: marking PR #805 ready for review is an administrative prerequisite for the already-authorized guarded merge. It does not expand authorization beyond merge of exact head |
|
Administrative checkpoint only; no scope expansion. |
|
Ready-state transition is pending; this comment does not itself change PR state. |
|
No-op control note. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039ca979b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let enabled_members: Vec<DistributionPlatform> = existing | ||
| .iter() | ||
| .filter(|row| row.enabled) | ||
| .map(|row| row.platform) | ||
| .collect(); |
There was a problem hiding this comment.
Update every existing row during a linked disable
When a linked pair is already inconsistent—one OAPEN/DOAB row enabled and the other disabled—this filters the update target down to only the enabled row. The supported disable therefore leaves the other existing member with a stale disabled_at/updated_at, contrary to the linked-disable contract that, once any member is enabled, every existing member participates in the same logical transition. Build the target list from all existing member rows after the enabled-state check.
Useful? React with 👍 / 👎.
| Programme integration branch: None | ||
| Task branch: `feature/publisher-services/be-02` (created from exactly the authorized base) | ||
| Head commit: recorded on the pull request; see section 3 for the commit series | ||
| Pull request: [#805](https://github.com/thoth-pub/thoth/pull/805) - OPEN, DRAFT, UNMERGED |
There was a problem hiding this comment.
Remove transient PR status from the committed report
Once this commit is reviewed or merged, OPEN, DRAFT, UNMERGED becomes false while remaining in the durable implementation report, forcing a follow-up documentation correction. Record the durable decision/authority condition here and leave live review and merge status to the GitHub PR history.
AGENTS.md reference: docs/engineering/AGENTS.md:L32-L59
Useful? React with 👍 / 👎.
BE-02-CLOSEOUT-01 authorization — transcription of a CTO instructionThis comment is a transcription, by the authoring agent, of an explicit CTO Recorded instruction: Why a bounded post-merge task exists at all: The closeout deliberately does not copy review identifiers, approval Nothing in this comment authorizes deployment, environment or production The closeout is delivered as a separate bounded draft pull request against |
Task identity
origin/developwas verified at1c752a52before any edit, with zero commits added sinceauthorization, and the branch was created from exactly that SHA.
Scope
The complete approved BE-02 specification, as an inactive additive foundation:
DistributionPlatformdomain enum;distribution_platformenum;publisher_distribution_platformpersistence relation;Publisher.distributionPlatforms;thoth-api/src/schema.rsupdate;Non-goals
Nothing in this PR implements or alters BE-03, BE-04, MIG-01, Publisher Services
application/UI work, distribution workers, distribution jobs, dissemination activation,
OAI, Thoth Metrics, mutation-guard architecture,
OBSERVE,ENFORCE, PR #799, a Juniperupgrade, an
async-graphqlmigration, ADR-0006 A2 batching machinery, or any new generalbatching architecture. No
thoth-appchange is made.Migration summary
thoth-api/migrations/20260812_v1.7.0/{up,down}.sql, plus the manual atomicthoth-api/src/schema.rsupdate required by ADR-0003 Architecture A.public.distribution_platformwith the 17 approved labels incanonical declaration order;
publisher_distribution_platformwith composite primary key(publisher_id, platform),ON DELETE CASCADEforeign key topublisher(publisher_id), the namedpublisher_distribution_platform_enabled_state_checkconstraint enforcingenabled == (disabled_at IS NULL),enabled/activation_id/enabled_atNOT NULLwith no
enableddefault, anddisabled_atas the only nullable lifecycle field;publisher_distribution_platform_enabled_idxon(platform, publisher_id) WHERE enabled;set_updated_attrigger viadiesel_manage_updated_at.The migration is additive and inserts no data.
down.sqldrops the table before the enumtype. Migration files are append-only; no historical migration was edited.
GraphQL summary
The generated SDL diff against the authorized base is purely additive: 0 removed lines,
63 added lines, matching specification section 12.1 exactly.
distributionPlatformOptions,publishersByDistributionPlatform,publisherCountByDistributionPlatform;Publisher.distributionPlatforms;DistributionPlatformOption,PublisherDistributionPlatformAssignment;DistributionPlatform(17 values),DistributionPlatformGroup,BackCatalogueBehaviour;AssignmentAvailability,MechanismReadinessandDistributionAdapterProfileremaininternal Rust vocabulary and are absent from SDL, as are
activationId,disabledAt,retained history, package/capability state, protected BE-03 configuration and any
adapter, feed, host, endpoint, bucket, account or credential identity. All four surfaces
are intentionally public and add no new authorization branch.
DataLoader / N+1 summary
Publisher.distributionPlatformsis the first production consumer of the merged ADR-0007foundation. One typed loader was added to the existing request-local
RequestLoadersbundle — not a second loader subsystem.
dataloader::non_cached::Loader;publisher_id(Uuid);configured_loader, somax_batch_size = 200andyield_count = 10are explicit;try_loadonly;DataLoader::loadis not used;before it;
Ok([]), no requestedkey is omitted, and a backend failure produces an error for every key in the failed
chunk with no retry and no per-parent fallback;
eq_anystatement per dispatch chunk, executed entirely insidetokio::task::spawn_blockingwith the connection acquired, used and dropped inside theclosure and never held across an
.await;existing non-serde
SharedBatchErrorprojection.Query-count evidence
Measured with real Diesel connection instrumentation (
SqlProbe), not an internalcounter. The loader's statement selects
FROM "publisher_distribution_platform", whilethe reverse root query reaches the table through
FROM ("publisher" INNER JOIN ...), sochild and root SQL are classified apart and reported separately.
Reference case, both parent shapes:
Boundaries, using the production constructor and the production batcher:
These are the shapes the loader-first resolver actually creates; no universal
arbitrary-scheduling guarantee is claimed.
Test evidence
67 new tests (40 model/lifecycle, 27 GraphQL/DataLoader). Full workspace suite: 1173
passed, 0 failed, of which
thoth-apicontributes 976 (909 at base).Covered: 17-value inventory and order, no fallback/default, exhaustive descriptors,
linked-group metadata, OCLC/Ex Libris independence, JISC non-assignability and
fail-before-write; all six singleton transitions, retained disabled rows, re-enable
activation replacement, same-state no-op timestamps,
ABSENT -> DISABLED, the databaseCHECK, cascade, concurrency; linked enable/disable from either member, normalized no-op,
one-sided repair, split-activation repair, split-timestamp repair, injected second-row
rollback, concurrent linked enables; options metadata, enabled-only child field, reverse
lookup, count agreement, empty results, deterministic pagination through ordering ties,
anonymous access, negative exposure assertions, SDL inventory; loader-first, 200/10
config,
try_loadonly, total batches, real SQL counts, request isolation, non-caching,zero-relation totality, backend failure, no retry, no fallback, and direct-vs-loader
failure equivalence (identical message, path and extensions convention).
Migration evidence
Disposable PostgreSQL 17.10 databases only. No production database was touched.
Empty database —
cargo run migratesucceeded; catalog queries confirmed 17 enumlabels in exact order, 8 columns with the specified types/nullability/defaults, the
composite PK, the
ON DELETE CASCADEFK, the named CHECK, the partial index, theset_updated_attrigger and zero assignment rows;cargo run migrate --revertremoved the table and enum type; a fresh
cargo run migratereproduced the same schema.Representative populated database — seeded with 4 publishers across all four BE-01
packages plus imprints, a work, a title, publisher history and a contact. After the
forward migration the publisher fingerprint and the related-record fingerprint were
byte-identical to the pre-migration baseline, the full row census was unchanged, and
publisher_distribution_platformcontained zero rows. FK rejection of an unknownpublisher,
ON DELETE CASCADE, both CHECK violation directions, composite-PK duplicaterejection and partial-index usage were each observed directly.
Lock evidence — observed from a second session with the migration transaction open on
a populated disposable database, PostgreSQL 17.10. The migration session held
ShareRowExclusiveLockandAccessShareLockonpublisher, andAccessExclusiveLock/ShareLock/ShareRowExclusiveLock/AccessShareLockon the newchild table. A concurrent
SELECTonpublishersucceeded; a concurrentUPDATEblockedand failed with
canceling statement due to lock timeoutatlock_timeout = '3s', withthe row unchanged. The child table is created empty, so there is no child-row FK
validation scan. No production duration is claimed.
Client / schema evidence
The authoritative SDL was regenerated by building both the authorized base (in a
throwaway
git worktree) and this head, then diffing the two generatedthoth-client/assets/schema.graphqlfiles:thoth-client/queries.graphqlis unchanged, andthoth-clientcompiles and tests againstthe new schema, so no generated client type changes.
thoth-export-servercompiles andits 144 tests pass unchanged. This is a verified result, not an assumption.
Rollout
None authorizes the next. After merge alone, only Git history changes. After a separately
authorized deployment and migration in an environment with zero assignment rows, options
returns 17 descriptors, publisher assignments return empty lists, reverse lookups return
empty pages, counts return 0, no distribution job exists and dissemination is unchanged.
Rollback
Before any environment adoption, a bounded repository revert is possible under normal
review. After a separately authorized environment migration, the specified response to a
defect is retained foundation plus forward repair: keep the enum type, table,
constraints, index, trigger and any stored rows, keep downstream consumers inactive, and
forward-fix under review. The committed down migration is reversibility evidence, not the
normal production rollback. Dropping populated assignment state would require a separate
CTO-approved task.
Authorization boundary
Known gaps
publisher_uniq_idxis a unique index onlower(publisher_name), so literally duplicatepublisher names cannot exist. The ordering-tie test therefore creates the equivalent tie
on a nullable sort field (
publisher_shortname, NULL for every fixture publisher), whichexercises the mandatory
publisher_id ASCtie-breaker in both sort directions.pathfor a list element does not carry the element index, sothe direct-vs-loader equivalence test asserts the stable
["publishers", "distributionPlatforms"]path rather than an indexed one. Message, pathand extensions convention are asserted equal between the two paths.
MetricPlatformdoes not exist in the repository at this base, so "no shared enum and noconversion with Metrics" is evidenced by construction and by a source assertion rather
than by a cross-type test.
behaviour under production concurrency is a release-time concern and is deliberately not
asserted here.
IMPLEMENTATION AGENT HAS NOT APPROVED THIS PR
This PR requires fresh independent exact-head review by an agent/model that did not
implement the task, followed by separate explicit CTO merge authorization bound to that
exact head. A green CI result is not approval.