Skip to content

fix: enforce array contains/minContains/maxContains on generated models - #57

Draft
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/enforce-array-contains
Draft

fix: enforce array contains/minContains/maxContains on generated models#57
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/enforce-array-contains

Conversation

@vishkaty

Copy link
Copy Markdown
Contributor

What

totals.json requires an array that contains exactly one subtotal and one
total
entry (allOf of two contains groups, each minContains:1/maxContains:1).
datamodel-code-generator drops contains/minContains/maxContains, so the
generated Totals (a bare list[Total] alias) accepts arrays that violate this —
empty, missing a total, or duplicate subtotals all pass validation.

This extends the existing postprocess_models.py mechanism (added in #55 for
minProperties) to also enforce array containment. Because preprocess_schemas.py
merges allOf and a JSON node can hold only one contains (silently dropping the
second group), the injector now snapshots the pristine schemas before
preprocessing and derives every predicate from contains.properties.*.const. It
threads a pydantic.AfterValidator enforcing all bounds into the alias metadata —
Totals and its create/update request variants alike. Data-driven (nothing
hardcodes subtotal/total), idempotent, dependency-free.

Also fixed: SDK-guarded tests were silently skipping

description.json moved from shopping/types to common/types, so the shared
test import raised and set HAVE_SDK = False — every SDK-guarded test (including
#55's minProperties tests) was skipping in CI. This corrects the import, so those
tests run again.

Tests

tests/test_codegen_pipeline.py:

  • TotalsContainsTest — across Totals, TotalsCreateRequest, TotalsUpdateRequest:
    reject [], [total, total], [subtotal, subtotal], [subtotal], [total];
    accept [subtotal, total]; plus a test asserting a subtotal-only array fails on
    the total rule specifically.
  • ArrayContainsInjectorTest — scanner reads both allOf branches and root-level
    single contains; ignores non-array / predicate-less; injection + idempotency.

Verification

  • Full suite green as CI runs it (python -m unittest discover -s tests -p "test_*.py"):
    33 passed, 0 skipped (with the SDK-guarded tests now actually running; fix: enforce minProperties on generated models #55's
    tests pass).
  • bash generate_models.sh regenerates the validator on all three models;
    idempotent (byte-identical re-run); ruff + ruff-format clean.

Generated models are intentionally not committed (the tracked generated tree is
stale vs main); the injector emits the validator whenever generate_models.sh runs.

@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Jul 28, 2026
datamodel-code-generator drops contains/minContains/maxContains, so the
generated Totals (a bare list[Total] alias) accepts arrays that violate
totals.json's "exactly one subtotal and one total" rule. Snapshot the pristine
schemas before preprocessing (which merges allOf and would drop the second
contains), derive every predicate from contains.properties.*.const, and thread
a pydantic AfterValidator enforcing all bounds into the alias metadata — base
model and request variants alike. Also fix the moved description import so the
SDK-guarded codegen tests run. Data-driven, idempotent, dependency-free.
@vishkaty
vishkaty force-pushed the fix/enforce-array-contains branch from 1bd8d19 to f1c4c44 Compare July 28, 2026 21:17
@vishkaty
vishkaty marked this pull request as draft July 28, 2026 21:19
@vishkaty

Copy link
Copy Markdown
Contributor Author

Following up with the results of a full end to end verification.

I regenerated the models against the pinned schema and ran the complete conformance suite against the reference flower shop server, comparing a build with these validators to a baseline without them. The baseline passes cleanly (69 tests, 0 failures). With the contains validators active, every checkout operation fails (67 failures across all 15 files).

The cause is a construction order in the reference server, not a spec violation. The server creates the checkout with an empty totals list and then fills it in during recalculation:

checkout = Checkout(..., totals=[], ...)   # services/checkout_service.py
... later ...
_recalculate_totals(checkout)              # appends subtotal + total

Because the constraint is an AfterValidator, it runs at construction time on the transient empty list and raises before recalculation runs. The final wire document the server emits is fully valid (exactly one subtotal and one total), so this is only about when the check fires.

The wider point is one of altitude. The generated models are used both to parse incoming documents and to build outgoing ones incrementally. A wire level contains cardinality rule enforced on every construction rejects a legitimate mid build state, which makes it a breaking change for any builder style consumer, the reference server being the clearest example. This differs from a leaf constraint like the minProperties work, where the object is never built empty and then populated.

Since you maintain both the SDK and the reference server, I did not want to pick a direction unilaterally. Two options that both preserve strict enforcement:

  1. Keep the validators and adjust the reference server to avoid constructing an invalid intermediate, for example computing totals before construction or using model_construct for the transient. Small server change, keeps enforcement strict.
  2. Move enforcement to the parse and serialize boundary so it validates complete documents rather than every in memory construction.

I am happy to implement either, or a variation you prefer. Marking this as a draft until we align on the approach.

@damaz91

damaz91 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Thanks for drafting the change and outlining the options @vishkaty ! I'm good with option 1

@damaz91 damaz91 added WIP and removed status:needs-triage Signal that the PR is ready for human triage labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants