Repository navigation
feat(reporting): add the buyer write facade and guard raw sync_principal - #1737
Conversation
sync_principal treats reporting_subscriptions and reporting_destinations
as complete desired state, so a raw write that adds one feed deactivates
every other subscription, and omitting a destination revokes it for every
account. Python had no guard (DX plan section 4, item 1).
client.reporting (ReportingClient) is patch-shaped:
- subscribe() replaces one subscription and keeps every other entry as
the seller stores it. It auto-versions: the version is kept when only
active or replace_account_configs change and bumped when the selector,
scope, destination, reconciliation_mode, exclude_account_ids or
operator_domains change; a supplied version must exceed the current one
when the content changed. register_destination upserts a destination in
the same write. An unchanged subscription writes nothing.
- unsubscribe() is the only deactivation path; resubscribing reactivates
at the same version.
- replace_subscriptions() is the expert complete-set write: omitting an
active subscription needs deactivate_omitted=True, and [] needs
deactivate_all=True.
- list_subscriptions() and preview() (a seller dry run) round it out.
Pre-dispatch checks raise ReportingSubscriptionError with dispatched=False:
an explicit scope, a registered active destination, {account_id} in the
path template of a multi-account scope, a subscription_id that clashes
with a delivery_config_id on list_accounts (SUBSCRIPTION_ID_CLASH), an
all-accounts scope across operators without operator_domains
(SUBSCRIPTION_OPERATOR_SCOPE_REQUIRED), and a selector no advertised
offering matches (SUBSCRIPTION_SELECTOR_UNMATCHED, naming the nearest
offerings). The roster and selector checks run when the client can read
list_accounts and capabilities; a partial roster is never used.
Each write carries expected_configuration_version and a fresh idempotency
key per attempt. A transport failure resends the same key and body once;
CONFLICT re-reads, re-merges and retries, up to 3 attempts, then raises
SUBSCRIPTION_CONFLICT_EXHAUSTED. The readback must carry every submitted
subscription at its version, state and content, or the write raises
SUBSCRIPTION_READBACK_MISMATCH with the readback attached.
ADCPClient.sync_principal now reads the principal first when a write
carries either section, and refuses before dispatch a write that would
deactivate an active subscription (SUBSCRIPTION_WOULD_DEACTIVATE) or
revoke a destination (DESTINATION_WOULD_REVOKE) by omission, unless
allow_deactivation / allow_revocation is passed. An explicit active: false
is always allowed.
Refs #1695
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| element for element. | ||
| """ | ||
| if isinstance(sent, list): | ||
| if not isinstance(stored, list) or len(stored) != len(sent): |
There was a problem hiding this comment.
Medium: differing_path compares arrays order-sensitively (len + element-for-element at L226-L231), but the subscription's account_ids, dimensions, exclude_account_ids and operator_domains are uniqueItems sets. _verify feeds the whole submitted config through here, so a conformant seller that returns any of those arrays in a canonical (e.g. sorted) order different from the buyer's input trips a terminal SUBSCRIPTION_READBACK_MISMATCH ("The seller altered a submitted field; contact the seller") on an otherwise successful write. The SDK's own registry._semantic sorts exactly these four arrays before comparing versions — because sellers reorder them — so the verifier is inconsistent with the version path. Normalize set-valued arrays before the element compare.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Checked: new 818-line patch-shaped write facade (facade.py), a client.py guard, and a 346-line test. Conflict/idempotency/version logic checks out; import layering respected; no public-surface removals/renames flagged, no credential-in-ctx_metadata issue, CI gates untouched.
Medium findings
- src/adcp/reporting/subscriptions/facade.py:226 — Readback verifier is order-sensitive on uniqueItems set arrays (account_ids/dimensions/exclude_account_ids/operator_domains are set-semantic per SDK
_semantic, so a conformant seller that reorders them could trip a terminal SUBSCRIPTION_READBACK_MISMATCH). Worth a follow-up but non-blocking.
Note for humans: sync_principal now raises pre-dispatch by default on by-omission deactivation/revocation — a documented behavior change to an existing public method, opt-out via allow_deactivation/allow_revocation, shipped under feat(reporting). Reviewer judged this non-blocking.
Decision path: no critical/high findings; gated_paths false; high_risk false; no author team gate; only 1 medium finding (fewer than 3). Falls through to row 9 — approve.
Medium findings
- src/adcp/reporting/subscriptions/facade.py:226 — Readback verifier is order-sensitive on uniqueItems set arrays
Summary
Gate slice P4 of the reporting-subscriptions DX plan (§2.7, §4 item 1, §6.2), stacked on #1736. It adds the buyer write facade,
client.reporting, and the rawsync_principalguard that Python lacked.client.reporting(ReportingClient)subscribe(sub, register_destination=, acknowledge_cross_operator=)unsubscribe(id)replace_subscriptions(list, deactivate_omitted=, deactivate_all=, reporting_destinations=, allow_revocation=)deactivate_omitted=True, and[]needsdeactivate_all=True.list_subscriptions(),preview()would_be, the version and the resolved offering ids.Auto-versioning:
activeorreplace_account_configschange.reconciliation_mode,exclude_account_idsoroperator_domainschange.Pre-dispatch checks, all
dispatched=False:{account_id}when the scope can cover several accounts;SUBSCRIPTION_ID_CLASH(fromlist_accounts);SUBSCRIPTION_OPERATOR_SCOPE_REQUIRED;SUBSCRIPTION_SELECTOR_UNMATCHED, resolved with the seller-side matching rules over the advertised offerings and naming the nearest ones.The roster and selector checks run when the client can read
list_accountsand capabilities. A partial roster is never used.Write:
expected_configuration_version, with a fresh key per attempt.CONFLICTre-reads, re-merges and retries, up to 3 attempts, then raisesSUBSCRIPTION_CONFLICT_EXHAUSTED(transient).syncReportingSubscriptionsV1; a mismatch raisesSUBSCRIPTION_READBACK_MISMATCH(terminal, withreadback).Raw
sync_principalguardADCPClient.sync_principalreads the principal only when the write carriesreporting_subscriptionsorreporting_destinations. Before dispatch, it refuses:SUBSCRIPTION_WOULD_DEACTIVATE, carryingwould_deactivate;DESTINATION_WOULD_REVOKE.Pass
allow_deactivation=Trueorallow_revocation=Trueto proceed. An explicitactive: falseis always allowed. The docstring points toclient.reporting.subscribe().Not in this slice
preview()'s per-account resolution (perAccount) needs the derived roster and coverage engine; it follows with P10. Error codes will be re-aligned to the spec-repoerror-codes.json(adcp#8616) in a follow-up commit on #1735 and #1736.Tests
tests/test_reporting_subscription_facade.pyruns against a real in-process seller and covers:allow_deactivation, explicitactive: false, and destination revocation;ADCPClient.sync_principalrunning the guard;replace_subscriptionsflags;Refs #1695
🤖 Generated with Claude Code