spec(idempotency): discriminated oneOf refactor (closes #2436)#2447
Merged
spec(idempotency): discriminated oneOf refactor (closes #2436)#2447
Conversation
…loses #2436) Swap the draft-07 if/then conditional for a two-branch discriminated union on `supported`: `IdempotencySupported` carries `replay_ttl_seconds` (required); `IdempotencyUnsupported` forbids it via `not: {required: [replay_ttl_seconds]}`. Wire format unchanged. Code generators that silently drop `if/then` (openapi-typescript, zod-to-json-schema, pre-0.25 datamodel-code-generator, quicktype) now emit two named types with the invariant at the type level. Safe to fold into #2434 because `supported` is brand-new in that PR — no SDKs have generated against the interim `if/then` shape yet. Deferring to 4.0 per the original #2436 proposal would have meant shipping a known-bad shape for one whole release cycle. Compliance storyboard narrative and validations updated to reference the new shape; `supported: false` sellers skip the replay storyboard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…scription Address review feedback on PR #2447: - Replace meta-phrasing on IdempotencySupported.supported with wire-behavior description, matching the tone of IdempotencyUnsupported.supported. - Add five schema fixtures exercising the oneOf discriminator: two positive (supported:true with TTL, supported:false alone) and three negative paths (TTL on unsupported, missing TTL on supported, empty block). The negative cases lock the `not: {required}` invariant that Ajv's default error text obscures. Codegen spot-check verified externally: openapi-typescript 7.13 emits `{supported: true; replay_ttl_seconds: number} | {supported: false}` and datamodel-code-generator 0.54 emits two Pydantic classes with `Literal[True]/Literal[False]` and required `replay_ttl_seconds` on the supported branch — exactly the type-level invariant the refactor promises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
71747db to
202fc63
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
adcp.idempotencyfrom draft-07if/thento a two-branch discriminatedoneOfonsupported:IdempotencySupported(requiresreplay_ttl_seconds) andIdempotencyUnsupported(forbidsreplay_ttl_secondsvianot).if/then(openapi-typescript, zod-to-json-schema, pre-0.25 datamodel-code-generator, quicktype) now emit two named types with the invariant at the type level.supportedand clarify thatsupported: falsesellers skip the replay storyboard.tests/composed-schema-validation.test.cjs) — two positive, three negative — locking the discriminator invariant.Rebased onto main after #2434 and #2444 merged.
Why fold into the 3.0 RC instead of deferring to 4.0 (as the original #2436 proposed)
The deferral rationale in #2436 was "breaking change for SDKs generating against
if/then." Butsupportedwas brand-new in #2434 when #2436 was filed — no SDKs had generated against the interimif/thenshape yet. Shippingif/thenat 3.0 GA and fixing at 4.0 would ship a known-bad shape for a whole release cycle.Codegen spot-check (verified externally, per review feedback)
Ran both codegens against the bundled schema:
Pydantic v2 via
datamodel-code-generator0.54:TypeScript via
openapi-typescript7.13:Both produce proper discriminated unions with the
replay_ttl_seconds-when-supported invariant at the type level — exactly what the refactor promises. Minor nit on datamodel-code-generator: it names the unsupported classIdempotency1instead of using thetitle(a known quirk for non-root oneOf subschemas); consumers can rename via codegen config.Item-level
if/thenasymmetry — owning the callsync-creatives-response.jsonstill has an item-levelif/thenforbiddingstatuswhenaction ∈ {failed, deleted}. Same ergonomics trap, not fixed here. The reason isn't schema-refactor cost ($defsfactoring is already used in 7 AdCP schemas including one response schema) — it's thatstatussits inside a family of action-gated optional fields:changes(only whenaction="updated"),errors(only whenaction="failed"),preview_url,expires_at,assigned_to,assignment_errors. None of those siblings have schema-enforced invariants. Splittingstatusalone into aoneOfwould make it anomalously stricter than its siblings — internal-consistency cost without matching benefit. The right question is "should ALL per-item action-gated invariants convert tooneOf?" which is a 4.0-scope protocol-shape discussion, not a one-field refactor.Blast-radius framing also differs: a dropped capability-level
if/thenproduces silent retry-storm misconfiguration at scale (type system lets a buyer ship without TTL awareness). A dropped item-levelif/thenproduces defensive null-checks on failed/deleted items that strict-typed buyers already write — unnecessary overhead, not corruption.Tracked as #2450 for 4.0 review alongside the rest of the action-gated family.
Expert review + feedback addressed
Ran through ad-tech-protocol-expert, dx-expert, adtech-product-expert, and code-reviewer across two rounds. All four validated the capability-level fold-in; 3 of 4 validated the item-level deferral framing after the asymmetry reasoning was sharpened (code-reviewer's pushback corrected the original claim that
$defswould be a novel pattern — it isn't, which reshaped this PR body and #2450).Second-pass feedback addressed:
IdempotencySupported.supported.descriptionrewritten from meta-phrasing ("authoritative") to wire-behavior matching the unsupported branch.{supported: false, replay_ttl_seconds: 3600},{supported: true}(missing TTL),{}(missing discriminator) — all now exercised.not: {required: [replay_ttl_seconds]}kept overadditionalProperties: falsefor forward-compat.Test plan
npm run test:schemas— 7/7 passingnpm run test:examples— 31/31 passingnpm run test:json-schema— 249 schema-annotated JSON blocks passing (includes hand-written idempotency examples)npm run test:composed— 17/17 passing (was 12, +5 new oneOf fixtures: 2 positive, 3 negative)npm run test:extensions— 20/20 passingopenapi-typescript7.13 +datamodel-code-generator0.54 both produce clean discriminated unionstest:unit587/587 +typecheck) — passing🤖 Generated with Claude Code