fix: add the missing maxProperties constraint family - #91
Open
vishkaty wants to merge 4 commits into
Open
Conversation
…uite The HAVE_SDK import gate at the top of test_codegen_pipeline.py still imported Description/Totals from ucp_sdk.models.schemas.shopping.types, paths that moved to ucp_sdk.models.schemas.common.types when Universal-Commerce-Protocol#87 (the 2026-08-25 UCP release regen) restructured the schema tree. The stale paths raise ModuleNotFoundError, which the surrounding try/except catches and sets HAVE_SDK = False, so every test gated on HAVE_SDK skips instead of running (unittest reports a skip as OK, so the suite reads green while ~37% of it never executes). Fix every stale shopping.types.* reference in the file: description, totals (+ its request variants), signals (+ its request variants), and error_response moved to common.types under the same class names, so those tests now run and pass unmodified. Two targets did not survive the schema restructuring at all -- card_payment_instrument.Constraints (a uniqueItems brands field) and merchant_fulfillment_config's nested additionalProperties:false object (now business_fulfillment_config, reshaped) -- so their four tests become documented unittest.skip with the reason recorded in the schema, not silently deleted. Before: 89 tests, 33 skipped (26 on the HAVE_SDK gate, 7 on 'executing the module needs pydantic', which also reads HAVE_SDK). After: 89 tests, 4 skipped, all four with a stated schema-shape reason. No production code changes; test-only.
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.
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).
damaz91
added
the
status:needs-triage
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
3 participants
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.
Fixes #90
What this changes
Adds find_root_max_properties, inject_max_properties, and
_patch_max_properties to postprocess_models.py, mirroring their
minProperties counterparts one for one (same marker guarded
idempotency, same model_fields_set union model_extra key counting
semantics, same free form object exclusion for a bound without declared
properties, already handled natively by the generator through
Field(max_length=...)). Wired into the script main() as an independent
patch pass so
both bounds can be injected into the same class without either
clobbering the other, which location_serves.json needs since it
declares minProperties 1 and maxProperties 1 together.
One deliberate difference from the function it mirrors:
find_root_min_properties treats a falsy minProperties (0) as absent,
using not minimum, which is harmless since minProperties 0 permits
everything minProperties absent already does. maxProperties 0 is a real
and different constraint, no properties allowed at all, so
find_root_max_properties checks isinstance(maximum, int) instead of
truthiness. This is new code, not a change to the existing minProperties
function, which is out of scope here.
Test plan
for minProperties, plus LocationServesMaxPropertiesSemanticTest
against the real committed model, including a negative control
proving the existing minProperties check is untouched by this change.
generator fix. Three files change: LocationServes,
LocationServesCreateRequest, LocationServesUpdateRequest.
pycache. No difference.
regenerated, reinstalled. The same failures reappeared exactly.
Restored the fix and regenerated again to confirm green.
Not included
README.md, which ruff format also reformats when generate_models.sh
runs, unrelated to this fix and present before this PR. Left untouched
to keep the diff scoped.