fix: enforce minProperties on generated models by vishkaty · Pull Request #55 · Universal-Commerce-Protocol/python-sdk

vishkaty · 2026-07-16T03:14:12Z

Fixes #49: datamodel-code-generator drops minProperties on object schemas with declared properties, so Description() validates with zero fields in violation of description.json's minProperties: 1. (The free-form-object case is unaffected — the generator already maps that to Field(min_length=...) on the dict field, e.g. AvailablePaymentInstrument.constraints.)

Implements the post-processing option proposed in the issue: a new postprocess_models.py step (wired into generate_models.sh between generation and formatting) scans the preprocessed schemas for root-level minProperties constraints and injects a model_validator(mode="after") into each matching generated class.

Counting semantics follow JSON Schema (keys present on the object), verified empirically against pydantic: provided fields (model_fields_set) unioned with extra keys (model_extra) — an explicit null is a present key, and unknown keys on extra="allow" models count too; the union avoids double-counting since pydantic includes extras in model_fields_set.

Properties of the step:

Data-driven: future minProperties additions to any schema are picked up on regeneration, no code change needed (currently description.json is the only root-level case on ucp main; there are no nested property-carrying cases).
Idempotent, and fails generation loudly if a constraint can't be mapped to a generated class — a constraint can never be silently dropped again.
The committed Description model is regenerated with the validator.

Validation

tests/test_min_properties.py: dependency-free injector tests (insertion, idempotency, schema scan, multi-class modules) + semantic tests on the real Description model (empty rejected; single field, explicit-null key, and extra-only key accepted). Full unittest discover: 23/23 green.
The workflow now runs pip install -e . so the semantic tests execute in CI rather than skipping.
Downstream check: the conformance suite (editable SDK) run against the samples Flower Shop stays green with the patched model.
ruff format + ruff check clean.

…otocol#49) datamodel-code-generator drops minProperties on object schemas with declared properties, so Description() validated with zero fields in violation of description.json's minProperties: 1. (minProperties on free-form object properties is unaffected — the generator already maps those to Field(min_length=...) on the dict field.) A new post-generation step (postprocess_models.py, wired into generate_models.sh before formatting) scans the preprocessed schemas for root-level minProperties constraints and injects a model_validator(mode="after") into each matching generated class. JSON Schema counts the keys present on the object, so the validator counts provided fields (model_fields_set) unioned with extra keys (model_extra): an explicit null is a present key, and unknown keys on extra="allow" models count too. The step is idempotent, data-driven from the schemas (future minProperties additions are picked up on regeneration), and fails generation loudly if a constraint can't be mapped to a generated class. The committed Description model is regenerated with the validator. Tests cover the injector (dependency-free) and the enforced semantics on Description; the workflow now installs the package so those semantic tests run in CI.

Deletes tests/test_min_properties.py and merges its test cases into tests/test_codegen_pipeline.py (renamed from test_preprocess_schemas.py to better reflect its expanded scope of testing both pre- and post-processing).

Replaces print statements with sys.stdout.write/sys.stderr.write (T201), splits long lines (E501), and adds docstring to main (D103) to comply with project lint configuration.

Replaces propert{y_ies} template variable with properties_noun to avoid codespell false positive on propert. Regenerated models to apply template change.

normalize_metadata_schemas hardcoded the UcpMetadata root union as five members (platform, business, response_{checkout,order,cart}) and so silently omitted response_catalog_schema, which is defined in ucp.json at the 2026-04-08 release the SDK targets. The generated UcpMetadata (src/ucp_sdk/models/schemas/ucp.py) therefore lacked ResponseCatalogSchema (the class was generated from the $def, but the union omitted it), so a UCP message carrying catalog-response metadata failed validation against every model's `ucp` field. Derive the union members from ucp.json's $defs instead: the discovery profiles (platform/business) plus every response_*_schema. This fixes the missing catalog entry and keeps the union complete as the protocol adds response types, matching the data-driven approach used for minProperties (Universal-Commerce-Protocol#55). Output order follows $defs insertion order, so existing members are unchanged and catalog is appended. Update the generated UcpMetadata in ucp.py accordingly. A full regeneration is intentionally not bundled here: the tracked generated tree predates the current ruff toolchain and regenerating adds unrelated formatting churn, so only the one union line is edited — exactly what regeneration produces for this fix. Add MetadataUnionTest covering the derivation (profiles + all response schemas, automatic pickup of new response types, exclusion of non-schema defs such as request_schema, empty $defs) and update the existing normalize_metadata_schemas test to assert the six-member union.

…og (#58) * fix(preprocess): derive UcpMetadata union from $defs to include catalog normalize_metadata_schemas hardcoded the UcpMetadata root union as five members (platform, business, response_{checkout,order,cart}) and so silently omitted response_catalog_schema, which is defined in ucp.json at the 2026-04-08 release the SDK targets. The generated UcpMetadata (src/ucp_sdk/models/schemas/ucp.py) therefore lacked ResponseCatalogSchema (the class was generated from the $def, but the union omitted it), so a UCP message carrying catalog-response metadata failed validation against every model's `ucp` field. Derive the union members from ucp.json's $defs instead: the discovery profiles (platform/business) plus every response_*_schema. This fixes the missing catalog entry and keeps the union complete as the protocol adds response types, matching the data-driven approach used for minProperties (#55). Output order follows $defs insertion order, so existing members are unchanged and catalog is appended. Update the generated UcpMetadata in ucp.py accordingly. A full regeneration is intentionally not bundled here: the tracked generated tree predates the current ruff toolchain and regenerating adds unrelated formatting churn, so only the one union line is edited — exactly what regeneration produces for this fix. Add MetadataUnionTest covering the derivation (profiles + all response schemas, automatic pickup of new response types, exclusion of non-schema defs such as request_schema, empty $defs) and update the existing normalize_metadata_schemas test to assert the six-member union. * fix(models): include catalog response in metadata request variants * style: fix end of file newlines in regenerated schema modules --------- Co-authored-by: damaz91 <federico.damato91@gmail.com>

datamodel-code-generator drops `uniqueItems`, so generated list fields accept duplicate entries in violation of the schema. Three UCP array properties declare `uniqueItems: true` at 2026-04-08: context.eligibility, card_payment_instrument.brands, and identity_linking.required_claims. Extend postprocess_models.py to collect array property names declared with `uniqueItems` and inject a `field_validator(mode="after")` into each generated class that declares a matching list field. The check uses equality (`item in seen`) so it holds for both hashable (str) and unhashable (model) items. Mirrors the data-driven, idempotent approach used for minProperties (Universal-Commerce-Protocol#55). Two generated models carry the affected list fields and gain the validator: Context.eligibility and Constraints.brands. required_claims has no generated typed field (ScopePolicy is extra="allow" free-form), so it is not enforceable and is skipped. Add UniqueItemsInjectorTest (scan walks nested properties and ignores non-arrays; injection targets only matching list fields, is idempotent, and enforces uniqueness when exec'd) and UniqueItemsSemanticTest (Constraints rejects duplicate brands, accepts unique/None).

* fix: enforce uniqueItems on generated array fields datamodel-code-generator drops `uniqueItems`, so generated list fields accept duplicate entries in violation of the schema. Three UCP array properties declare `uniqueItems: true` at 2026-04-08: context.eligibility, card_payment_instrument.brands, and identity_linking.required_claims. Extend postprocess_models.py to collect array property names declared with `uniqueItems` and inject a `field_validator(mode="after")` into each generated class that declares a matching list field. The check uses equality (`item in seen`) so it holds for both hashable (str) and unhashable (model) items. Mirrors the data-driven, idempotent approach used for minProperties (#55). Two generated models carry the affected list fields and gain the validator: Context.eligibility and Constraints.brands. required_claims has no generated typed field (ScopePolicy is extra="allow" free-form), so it is not enforceable and is skipped. Add UniqueItemsInjectorTest (scan walks nested properties and ignores non-arrays; injection targets only matching list fields, is idempotent, and enforces uniqueness when exec'd) and UniqueItemsSemanticTest (Constraints rejects duplicate brands, accepts unique/None). * chore: bump version to 0.4.4 and regenerate models --------- Co-authored-by: damaz91 <federico.damato91@gmail.com>

location_serves.json declares both minProperties: 1 AND maxProperties: 1 at the schema root ("The Platform MUST supply exactly one target form"), but only minProperties was ever scanned: find_root_min_properties reads schema.get("minProperties") and there is no symmetric find_root_max_properties at all (maxProperties has been unhandled since PR Universal-Commerce-Protocol#55 added the minProperties family for issue Universal-Commerce-Protocol#49). The committed LocationServes model enforces the minimum but not the maximum, so a map naming both point and address validates in violation of the schema. Adds, mirroring InjectorTest (the existing minProperties injector test) one for one: - MaxPropertiesInjectorTest: injector-level unit tests against synthetic fixtures for find_root_max_properties (schema scan) and inject_max_properties (validator injection), including that both bounds can coexist on the same class without clobbering each other, and that a free-form object (no named properties, already handled natively via Field(max_length=...)) stays out of scope -- mirroring the min side's existing free-form-object exclusion. - LocationServesMaxPropertiesSemanticTest: exercises the real committed LocationServes model. Includes a negative control (test_empty_still_rejected_by_the_existing_minimum) proving the pre-existing minProperties check is untouched by this change, and a case confirming an extension key still counts toward the total under extra="allow" key-counting semantics. RED: 100 tests, 2 failures + 6 errors (find_root_max_properties and inject_max_properties do not exist yet), 4 documented skips (unchanged, from the root-cause-0 commit).

find_root_min_properties (added in Universal-Commerce-Protocol#55 for issue Universal-Commerce-Protocol#49) scans root-level minProperties on object schemas with declared properties, but maxProperties never grew a matching scanner: there is no find_root_max_properties at all. location_serves.json declares both minProperties: 1 and maxProperties: 1 on the same schema ("the Platform MUST supply exactly one target form"), so the committed LocationServes model enforces the minimum but silently accepts an object naming both point and address, which JSON Schema rejects. Adds find_root_max_properties, inject_max_properties, and _patch_max_properties, mirroring their minProperties counterparts one for one (same marker-guarded idempotency, same model_fields_set | model_extra key-counting semantics, same free-form object exclusion for maxProperties without declared properties, already handled natively via Field(max_length=...)). Wired into main() as an independent patch pass so both bounds can be injected into the same class without either clobbering the other. One deliberate deviation from the minProperties scanner it mirrors: find_root_min_properties treats a falsy minProperties (0) as absent via "not minimum", which is harmless since minProperties: 0 permits everything minProperties: absent already does. maxProperties: 0 is a real, different constraint (no properties allowed at all), so find_root_max_properties checks "isinstance(maximum, int)" instead of truthiness -- new code, not a fix to the existing (out of scope) min-side function. Generator-level change only (postprocess_models.py); no generated model files touched in this commit. Regeneration follows in a separate commit, which is what turns the two still-red semantic tests green (the injector-level unit tests added in the prior commit -- which exercise find_root_max_properties/inject_max_properties directly against synthetic fixtures, not the committed models -- already pass). 100 tests, 2 failures (LocationServesMaxPropertiesSemanticTest), 4 documented skips.

location_serves.json declares both minProperties: 1 AND maxProperties: 1 at the schema root ("The Platform MUST supply exactly one target form"), but only minProperties was ever scanned: find_root_min_properties reads schema.get("minProperties") and there is no symmetric find_root_max_properties at all (maxProperties has been unhandled since PR Universal-Commerce-Protocol#55 added the minProperties family for issue Universal-Commerce-Protocol#49). The committed LocationServes model enforces the minimum but not the maximum, so a map naming both point and address validates in violation of the schema. Adds, mirroring InjectorTest (the existing minProperties injector test) one for one: - MaxPropertiesInjectorTest: injector-level unit tests against synthetic fixtures for find_root_max_properties (schema scan) and inject_max_properties (validator injection), including that both bounds can coexist on the same class without clobbering each other, and that a free-form object (no named properties, already handled natively via Field(max_length=...)) stays out of scope -- mirroring the min side's existing free-form-object exclusion. - LocationServesMaxPropertiesSemanticTest: exercises the real committed LocationServes model. Includes a negative control (test_empty_still_rejected_by_the_existing_minimum) proving the pre-existing minProperties check is untouched by this change, and a case confirming an extension key still counts toward the total under extra="allow" key-counting semantics. RED: 100 tests, 2 failures + 6 errors (find_root_max_properties and inject_max_properties do not exist yet), 4 documented skips (unchanged, from the root-cause-0 commit).

find_root_min_properties (added in Universal-Commerce-Protocol#55 for issue Universal-Commerce-Protocol#49) scans root-level minProperties on object schemas with declared properties, but maxProperties never grew a matching scanner: there is no find_root_max_properties at all. location_serves.json declares both minProperties: 1 and maxProperties: 1 on the same schema ("the Platform MUST supply exactly one target form"), so the committed LocationServes model enforces the minimum but silently accepts an object naming both point and address, which JSON Schema rejects. Adds find_root_max_properties, inject_max_properties, and _patch_max_properties, mirroring their minProperties counterparts one for one (same marker-guarded idempotency, same model_fields_set | model_extra key-counting semantics, same free-form object exclusion for maxProperties without declared properties, already handled natively via Field(max_length=...)). Wired into main() as an independent patch pass so both bounds can be injected into the same class without either clobbering the other. One deliberate deviation from the minProperties scanner it mirrors: find_root_min_properties treats a falsy minProperties (0) as absent via "not minimum", which is harmless since minProperties: 0 permits everything minProperties: absent already does. maxProperties: 0 is a real, different constraint (no properties allowed at all), so find_root_max_properties checks "isinstance(maximum, int)" instead of truthiness -- new code, not a fix to the existing (out of scope) min-side function. Generator-level change only (postprocess_models.py); no generated model files touched in this commit. Regeneration follows in a separate commit, which is what turns the two still-red semantic tests green (the injector-level unit tests added in the prior commit -- which exercise find_root_max_properties/inject_max_properties directly against synthetic fixtures, not the committed models -- already pass). 100 tests, 2 failures (LocationServesMaxPropertiesSemanticTest), 4 documented skips.

* test: add failing coverage for location_serves maxProperties location_serves.json declares both minProperties: 1 AND maxProperties: 1 at the schema root ("The Platform MUST supply exactly one target form"), but only minProperties was ever scanned: find_root_min_properties reads schema.get("minProperties") and there is no symmetric find_root_max_properties at all (maxProperties has been unhandled since PR #55 added the minProperties family for issue #49). The committed LocationServes model enforces the minimum but not the maximum, so a map naming both point and address validates in violation of the schema. Adds, mirroring InjectorTest (the existing minProperties injector test) one for one: - MaxPropertiesInjectorTest: injector-level unit tests against synthetic fixtures for find_root_max_properties (schema scan) and inject_max_properties (validator injection), including that both bounds can coexist on the same class without clobbering each other, and that a free-form object (no named properties, already handled natively via Field(max_length=...)) stays out of scope -- mirroring the min side's existing free-form-object exclusion. - LocationServesMaxPropertiesSemanticTest: exercises the real committed LocationServes model. Includes a negative control (test_empty_still_rejected_by_the_existing_minimum) proving the pre-existing minProperties check is untouched by this change, and a case confirming an extension key still counts toward the total under extra="allow" key-counting semantics. RED: 100 tests, 2 failures + 6 errors (find_root_max_properties and inject_max_properties do not exist yet), 4 documented skips (unchanged, from the root-cause-0 commit). * fix(codegen): add the missing maxProperties constraint family find_root_min_properties (added in #55 for issue #49) scans root-level minProperties on object schemas with declared properties, but maxProperties never grew a matching scanner: there is no find_root_max_properties at all. location_serves.json declares both minProperties: 1 and maxProperties: 1 on the same schema ("the Platform MUST supply exactly one target form"), so the committed LocationServes model enforces the minimum but silently accepts an object naming both point and address, which JSON Schema rejects. Adds find_root_max_properties, inject_max_properties, and _patch_max_properties, mirroring their minProperties counterparts one for one (same marker-guarded idempotency, same model_fields_set | model_extra key-counting semantics, same free-form object exclusion for maxProperties without declared properties, already handled natively via Field(max_length=...)). Wired into main() as an independent patch pass so both bounds can be injected into the same class without either clobbering the other. One deliberate deviation from the minProperties scanner it mirrors: find_root_min_properties treats a falsy minProperties (0) as absent via "not minimum", which is harmless since minProperties: 0 permits everything minProperties: absent already does. maxProperties: 0 is a real, different constraint (no properties allowed at all), so find_root_max_properties checks "isinstance(maximum, int)" instead of truthiness -- new code, not a fix to the existing (out of scope) min-side function. Generator-level change only (postprocess_models.py); no generated model files touched in this commit. Regeneration follows in a separate commit, which is what turns the two still-red semantic tests green (the injector-level unit tests added in the prior commit -- which exercise find_root_max_properties/inject_max_properties directly against synthetic fixtures, not the committed models -- already pass). 100 tests, 2 failures (LocationServesMaxPropertiesSemanticTest), 4 documented skips. * chore(models): regenerate against the pinned 2026-08-25 UCP schema Regenerates via ./generate_models.sh 2026-08-25 (the same command the model-drift CI job runs) to pick up the postprocessing fix in the prior commit. Three files change, all in the location_serves family: LocationServes, LocationServesCreateRequest and LocationServesUpdateRequest each gain an _enforce_max_properties validator alongside their existing _enforce_min_properties one. Verified: - Full suite: 100 tests, 0 failures, 4 documented skips (both new semantic tests from the RED commit now pass). - Double-regen: ran generate_models.sh 2026-08-25 twice; diff -rq between both outputs (excluding __pycache__) is empty. - Kill-test: reverted postprocess_models.py to its pre-fix state, regenerated, reinstalled -- the same 2 failures + 6 errors from the RED commit reappeared verbatim. Restored the fix and regenerated again to confirm the suite returns to green. - pre-commit run on the changed files: clean. Not committed: README.md, which ruff format also reformats as a pre-existing docstring-code-block spacing drift in main, unrelated to this fix (see the equivalent note on the jwk-conditional-rules branch). --------- Co-authored-by: Vishal Katyal <vishal@katyal.ai>

damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Jul 16, 2026

ShuoRen-TT mentioned this pull request Jul 16, 2026

fix: reuse duplicate capability extension models #56

Merged

damaz91 added status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Jul 16, 2026

damaz91 approved these changes Jul 17, 2026

View reviewed changes

damaz91 and others added 4 commits July 17, 2026 10:04

Merge branch 'main' into fix/min-properties-49

a8cea66

fix: resolve ruff linter issues in postprocess_models.py

c3a415f

Replaces print statements with sys.stdout.write/sys.stderr.write (T201), splits long lines (E501), and adds docstring to main (D103) to comply with project lint configuration.

fix: resolve codespell error in postprocess_models.py

c5af615

Replaces propert{y_ies} template variable with properties_noun to avoid codespell false positive on propert. Regenerated models to apply template change.

damaz91 merged commit c1ffd1b into Universal-Commerce-Protocol:main Jul 17, 2026
14 checks passed

vishkaty mentioned this pull request Jul 28, 2026

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

Merged

XiaolongZhang-TT mentioned this pull request Jul 30, 2026

fix(preprocess): derive UcpMetadata union from $defs to include catalog #58

Merged

17 tasks

XiaolongZhang-TT mentioned this pull request Aug 3, 2026

fix: enforce uniqueItems on generated array fields #59

Merged

17 tasks

vishkaty mentioned this pull request Aug 28, 2026

bug: location_serves.json maxProperties is never enforced #90

Closed

Navigation Menu

Uh oh!

fix: enforce minProperties on generated models - #55

fix: enforce minProperties on generated models#55
damaz91 merged 5 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/min-properties-49

vishkaty commented Jul 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vishkaty commented Jul 16, 2026

Validation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants