Skip to content

fix: rewrite $ref in top level oneOf/anyOf/allOf branches during variant generation - #83

Open
vishkaty wants to merge 5 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/variant-oneof-refs
Open

fix: rewrite $ref in top level oneOf/anyOf/allOf branches during variant generation#83
vishkaty wants to merge 5 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/variant-oneof-refs

Conversation

@vishkaty

Copy link
Copy Markdown
Contributor

Description

What

_create_single_variant in preprocess_schemas.py builds a create/update/complete variant of a
schema by filtering properties and rewriting external $ref links so they point at sibling
request variants instead of the base response schemas. That rewrite only ran on refs found inside
properties. Schemas whose polymorphism lives at the top level, such as a oneOf union with no
own properties, or a single allOf inheritance ref, kept pointing at the base response schema
files. This change makes the rewrite run over the whole variant tree, so oneOf, anyOf, allOf,
and items are all covered, and it makes the dependency propagation step aware of refs found
outside properties so the target variant files actually get generated.

Observed before this change, FulfillmentDestinationCreateRequest wrapped
ShippingDestination | RetailLocation, the response models, where id is required. A caller
submitting a brand new shipping destination during create, with no id yet, failed validation
that should have passed.

Expected and now observed, FulfillmentDestinationCreateRequest wraps
ShippingDestinationCreateRequest | RetailLocationCreateRequest, where id is optional, and the
same payload validates cleanly.

Root cause

preprocess_schemas.py:537 to 561, _create_single_variant. The function only ever calls
_apply_request_rules_to_object (preprocess_schemas.py:508), which reads
object_schema.get("properties", {}) (preprocess_schemas.py:512), empty for a schema whose only
content is oneOf, anyOf, or a single allOf ref. No code path walked those branches, so
rewrite_refs_to_variants (preprocess_schemas.py:483) never saw the refs inside them.

The fix adds one call at the end of _create_single_variant that runs
rewrite_refs_to_variants over the entire variant, not only properties, and generalizes
extract_external_refs and propagate_needs_transitive so a ref found through oneOf, anyOf,
allOf, or items is tracked and propagated the same way a property ref already was.

The additional postprocess defect

Rewriting these refs lengthens some array root item type references. For totals.json, the item
type reference changes from total.Total to total_create_request.TotalCreateRequest. That extra
length is enough to push ruff formatting to wrap the surrounding Annotated[...] onto multiple
lines with a trailing comma before the closing bracket.

inject_array_contains in postprocess_models.py (around line 638 to 674, the function that
threads an AfterValidator into an array root alias for contains/minContains/maxContains
enforcement) spliced its new element in right before that closing bracket without checking for an
existing trailing comma. Once the comma was there, the splice landed after it, producing
Field(...), followed by a new line and , AfterValidator(...)], two commas with nothing between
them. That is invalid Python, not a formatting complaint, and ast.parse confirms it.

Because tests/test_codegen_pipeline.py imported the generated Totals*Request classes inside a
bare except ImportError, and SyntaxError is not caught by that clause, the broken file took the
whole test module down at collection time. Every test in the file, not only the ones touching
totals, failed to run.

This change fixes the splice in inject_array_contains to insert after the last real token before
the closing bracket, reusing an existing trailing comma when present, and widens the import guard
to except (ImportError, SyntaxError) so one broken generated file degrades to skipping the
SDK dependent tests instead of failing collection for the whole module.

Testing

Full suite, python -m unittest discover -s tests -p "test_*.py", the exact command the
schema-preprocessing CI job runs: 88 of 88 tests pass, 0 failures, 0 errors. Main before this
change runs 85 of 85. The 3 added tests are the 2 already present on this branch from the earlier
attempt at this fix, covering _create_single_variant and propagate_needs_transitive directly
against synthetic oneOf/anyOf/allOf schemas, plus one new test asserting that a line wrapped
Annotated[...] with a trailing comma still parses after inject_array_contains runs.

The original repro now validates cleanly: a create payload for FulfillmentDestinationCreateRequest
with no id returns a populated model instead of raising 3 field required errors.

generate_models.sh 2026-04-08, the pinned release per the README compatibility table, was run
from a clean checkout twice, with the same end of file normalization the model-drift CI job
applies. Both runs produced output byte identical to what is committed here, so model-drift will
stay green.

Kill tests, to confirm each fix is load bearing rather than coincidentally passing: reverting
preprocess_schemas.py alone, with the tests kept, turns 1 of 88 tests red
(test_composition_variant_rewrites_refs). Reverting postprocess_models.py alone, with the tests
kept, turns a different 1 of 88 red (the new line wrap test), with the same SyntaxError shape
that motivated the fix. Restoring both returns the suite to 88 of 88.

pre-commit run against the repo pinned .pre-commit-config.yaml on every hand written file this
change touches (preprocess_schemas.py, postprocess_models.py,
tests/test_codegen_pipeline.py) passes every hook, ruff lint and ruff format run and checked
separately, with zero files needing modification.

Sweep of the defect class

Every schema in the pinned release with a top level oneOf, anyOf, or allOf branch pointing at
an external file was checked against its generated output, before and after this change.

Schema Pattern Result
fulfillment_destination.json root oneOf Fixed
message.json root oneOf Fixed
shipping_destination.json root allOf inheritance Fixed
totals.json items.allOf array root union Fixed
card_credential.json root allOf inheritance Not applicable, no request variant is generated for this schema in this release
card_payment_instrument.json root allOf inheritance Not applicable, same reason
detail_option_value.json root allOf inheritance Not applicable, same reason
token_credential.json root allOf inheritance Not applicable, same reason

No schema in the release has a property level oneOf/anyOf with an external ref left unhandled;
that path already worked before this change.

Credit

The preprocessing approach in this change is the one proposed by xiaoxuanz-hub in #35: walk the
whole variant tree for external refs rather than special casing oneOf/anyOf, and propagate
composition derived refs the same way property derived refs already propagate. Both commits from
#35 are carried over with their original authorship, along with the test coverage damaz91 added on
that branch. This change supersedes #35 with that approach intact, adding the regeneration against
the pinned spec and a fix for a postprocessing defect that surfaced only once the longer variant
type references from this fix hit inject_array_contains, which #35 could not have caught since
the postprocessing constraint enforcement it interacts with landed after #35 was last updated.

Related work

#80, in flight while this change was prepared, adds the same whole tree rewrite_refs_to_variants
call in _create_single_variant as part of extension schema support, and generalizes
extract_external_refs and propagate_needs_transitive toward $defs where this change
generalizes them toward the root composition keywords. The two changes fix different generation
gaps and will collide textually in those three functions; whichever lands second needs a small
mechanical rebase, and if #80 goes first I will rebase this change on top of it.

Category (Required)

Please select one or more categories that apply to this change.

  • Core Protocol: Changes to the base communication layer, global context, or breaking refactors. (Requires Technical Council approval)
  • Governance/Contributing: Updates to GOVERNANCE.md, CONTRIBUTING.md, or CODEOWNERS. (Requires Governance Council approval)
  • Capability: New schemas (Discovery, Cart, etc.) or extensions. (Requires Maintainer approval)
  • Documentation: Updates to README, or documentations regarding schema or capabilities. (Requires Maintainer approval)
  • Infrastructure: CI/CD, Linters, or build scripts. (Requires DevOps Maintainer approval)
  • Maintenance: Version bumps, lockfile updates, or minor bug fixes. (Requires DevOps Maintainer approval)
  • SDK: Language-specific SDK updates and releases. (Requires DevOps Maintainer approval)
  • Samples / Conformance: Maintaining samples and the conformance suite. (Requires Maintainer approval)
  • UCP Schema: Changes to the ucp-schema tool (resolver, linter, validator). (Requires Maintainer approval)
  • Community Health (.github): Updates to templates, workflows, or org-level configs. (Requires DevOps Maintainer approval)

Related Issues

Fixes #34. Supersedes #35.

Checklist

  • I have followed the Contributing Guide (including Conventional Commits title requirements and ! for breaking changes).
  • I have updated the documentation (if applicable).
  • My changes pass all local linting and formatting checks.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • (For Core/Capability) I have included/updated the relevant JSON schemas.
  • I have regenerated Python Pydantic models by running generate_models.sh under python_sdk.

xiaoxuanz-hub and others added 5 commits August 20, 2026 11:39
(cherry picked from commit e652462)
_create_single_variant now rewrites $ref inside top-level oneOf/anyOf/
allOf/items branches (see the preceding two commits, cherry-picked from
Universal-Commerce-Protocol#35), which lengthens some array-root item type references, e.g.
total.Total becomes total_create_request.TotalCreateRequest for
TotalsCreateRequest. That extra length pushes ruff's formatter to wrap
Annotated[...] onto multiple lines with a trailing comma before the
closing bracket.

inject_array_contains spliced AfterValidator(...) in right before that
closing bracket without checking for the trailing comma, landing the
new element after an orphaned comma with nothing between them - a
SyntaxError, not just a formatting nit. Because tests/test_codegen_pipeline.py
imported the generated Totals*Request classes inside a bare
"except ImportError", the SyntaxError went uncaught and took the whole
test module down at collection time, failing every test in the file.

Fix the splice to insert after the last real token before the closing
bracket instead of blindly before it, reusing an existing trailing
comma when present. Also widen the import guard to catch SyntaxError
so one broken generated file degrades to HAVE_SDK = False instead of
failing collection for the whole module.
Runs generate_models.sh 2026-04-08 (README compat table: SDK 0.4.x ->
UCP schema 2026-04-08) against the three preceding commits (the Universal-Commerce-Protocol#35
ref-rewrite cherry-picks plus the inject_array_contains fix). Only
totals_create_request.py/totals_update_request.py change beyond what
the cherry-picks already carried, because those two needed both fixes
together: the ref rewrite (to point at total_create_request.json
instead of total.json) and the trailing-comma-safe splice (to still
parse once that longer reference pushes Annotated[...] onto multiple
lines).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:needs-triage Signal that the PR is ready for human triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Variant generation doesn't rewrite $ref in top-level oneOf/anyOf branches

4 participants