From 988d53d3ab0173a0b7bbc24e0cafed428920c7ff Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Fri, 28 Aug 2026 09:26:19 -0400 Subject: [PATCH 1/4] test: fix stale shopping.types import paths in the codegen pipeline suite 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 #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. --- tests/test_codegen_pipeline.py | 90 ++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 16 deletions(-) diff --git a/tests/test_codegen_pipeline.py b/tests/test_codegen_pipeline.py index eba1f0c..665e7ac 100644 --- a/tests/test_codegen_pipeline.py +++ b/tests/test_codegen_pipeline.py @@ -31,12 +31,19 @@ try: from pydantic import TypeAdapter, ValidationError - from ucp_sdk.models.schemas.shopping.types.description import Description - from ucp_sdk.models.schemas.shopping.types.totals import Totals - from ucp_sdk.models.schemas.shopping.types.totals_create_request import ( + # NOTE(root-cause-0): these paths moved from shopping.types to + # common.types when #87 (2026-08-25 UCP release regen) restructured the + # schema tree. The old paths silently raise ModuleNotFoundError here, + # which the except clause below swallows as HAVE_SDK = False -- so every + # semantic test gated on HAVE_SDK skips instead of running, and CI is + # green on a suite that mostly never executed. See the sibling fixes to + # the other stale shopping.types.* imports later in this file. + from ucp_sdk.models.schemas.common.types.description import Description + from ucp_sdk.models.schemas.common.types.totals import Totals + from ucp_sdk.models.schemas.common.types.totals_create_request import ( TotalsCreateRequest, ) - from ucp_sdk.models.schemas.shopping.types.totals_update_request import ( + from ucp_sdk.models.schemas.common.types.totals_update_request import ( TotalsUpdateRequest, ) @@ -1014,7 +1021,7 @@ class SignalsPropertyNamesTest(unittest.TestCase): """ def _signals(self): - from ucp_sdk.models.schemas.shopping.types.signals import Signals + from ucp_sdk.models.schemas.common.types.signals import Signals return Signals @@ -1053,13 +1060,13 @@ def test_known_named_fields_still_work(self): def test_request_variants_enforce_property_names(self): # The gap and its fix travel to the generated request variants too. - from ucp_sdk.models.schemas.shopping.types.signals_complete_request import ( + from ucp_sdk.models.schemas.common.types.signals_complete_request import ( SignalsCompleteRequest, ) - from ucp_sdk.models.schemas.shopping.types.signals_create_request import ( + from ucp_sdk.models.schemas.common.types.signals_create_request import ( SignalsCreateRequest, ) - from ucp_sdk.models.schemas.shopping.types.signals_update_request import ( + from ucp_sdk.models.schemas.common.types.signals_update_request import ( SignalsUpdateRequest, ) @@ -1870,18 +1877,41 @@ def test_injected_validator_rejects_duplicates(self) -> None: class UniqueItemsSemanticTest(unittest.TestCase): """Committed models enforce uniqueItems on declared array fields.""" + # NOTE(root-cause-0): card_payment_instrument.json no longer declares a + # Constraints.brands field (uniqueItems) as of the pinned 2026-08-25 UCP + # schema -- the module now generates only Display, ConstraintTarget and + # CardPaymentInstrument (verified against + # src/ucp_sdk/models/schemas/common/types/card_payment_instrument.py). + # These two tests exercised a schema shape that no longer exists; the + # HAVE_SDK import gate bug (see the top of this file) had been hiding + # that they could not pass, not just that they were unrelated to SDK + # availability. Documented skip rather than silent deletion: the + # uniqueItems mechanism itself stays covered by UniqueItemsInjectorTest + # (injector unit tests) and by other committed models with uniqueItems + # fields (e.g. common.types.constraint_expression, context, + # location_filter, request_constraints). + @unittest.skip( + "card_payment_instrument.Constraints.brands (uniqueItems) was " + "removed from the schema before the pinned 2026-08-25 UCP release; " + "no current committed model at this path carries a brands field" + ) def test_brands_rejects_duplicates(self) -> None: """card_payment_instrument brands rejects duplicate entries.""" - from ucp_sdk.models.schemas.shopping.types.card_payment_instrument import ( + from ucp_sdk.models.schemas.common.types.card_payment_instrument import ( Constraints, ) with self.assertRaisesRegex(ValidationError, "[Uu]nique"): Constraints(brands=["visa", "visa"]) + @unittest.skip( + "card_payment_instrument.Constraints.brands (uniqueItems) was " + "removed from the schema before the pinned 2026-08-25 UCP release; " + "no current committed model at this path carries a brands field" + ) def test_brands_accepts_unique_and_none(self) -> None: """Unique lists and missing values are accepted.""" - from ucp_sdk.models.schemas.shopping.types.card_payment_instrument import ( + from ucp_sdk.models.schemas.common.types.card_payment_instrument import ( Constraints, ) @@ -2024,7 +2054,7 @@ class AdditionalPropertiesForbidSemanticTest(unittest.TestCase): """Committed models reject unknown keys on additionalProperties:false.""" def test_error_response_rejects_unknown_keys(self) -> None: - from ucp_sdk.models.schemas.shopping.types.error_response import ( + from ucp_sdk.models.schemas.common.types.error_response import ( ErrorResponse, ) @@ -2045,7 +2075,7 @@ def test_error_response_rejects_unknown_keys(self) -> None: ) def test_error_response_accepts_declared_fields(self) -> None: - from ucp_sdk.models.schemas.shopping.types.error_response import ( + from ucp_sdk.models.schemas.common.types.error_response import ( ErrorResponse, ) @@ -2064,8 +2094,29 @@ def test_error_response_accepts_declared_fields(self) -> None: ) self.assertEqual(obj.messages[0].content, "boom") + # NOTE(root-cause-0): merchant_fulfillment_config.json was renamed and + # restructured to business_fulfillment_config.json before the pinned + # 2026-08-25 UCP release. The nested additionalProperties:false object + # these tests targeted (allows_multi_destination -> AllowsMultiDestination) + # is gone; the current schema's multi_destination field is a list of + # MultiDestinationItem (extra="allow", no nested forbid object) -- + # verified against + # src/ucp_sdk/models/schemas/shopping/types/business_fulfillment_config.py. + # The HAVE_SDK import gate bug (see the top of this file) had been + # hiding that these two tests could not pass at all, not just that they + # were unrelated to SDK availability. Documented skip rather than silent + # deletion: the additionalProperties:false -> extra="forbid" mechanism + # itself stays covered by test_error_response_rejects_unknown_keys above + # and by AdditionalPropertiesForbidInjectorTest/FinderTest. + @unittest.skip( + "merchant_fulfillment_config.AllowsMultiDestination was removed " + "when the schema was restructured to " + "business_fulfillment_config.MultiDestinationItem before the " + "pinned 2026-08-25 UCP release; no current committed model at " + "this path carries a nested additionalProperties:false object" + ) def test_allows_multi_destination_rejects_unknown_keys(self) -> None: - from ucp_sdk.models.schemas.shopping.types.merchant_fulfillment_config import ( + from ucp_sdk.models.schemas.shopping.types.business_fulfillment_config import ( AllowsMultiDestination, ) @@ -2074,12 +2125,19 @@ def test_allows_multi_destination_rejects_unknown_keys(self) -> None: {"shipping": True, "bogus": "x"} ) + @unittest.skip( + "merchant_fulfillment_config.MerchantFulfillmentConfig was renamed " + "and restructured to business_fulfillment_config." + "BusinessFulfillmentConfig before the pinned 2026-08-25 UCP " + "release; see test_allows_multi_destination_rejects_unknown_keys " + "above" + ) def test_sibling_config_keeps_extra_allow(self) -> None: - from ucp_sdk.models.schemas.shopping.types.merchant_fulfillment_config import ( - MerchantFulfillmentConfig, + from ucp_sdk.models.schemas.shopping.types.business_fulfillment_config import ( + BusinessFulfillmentConfig, ) - config = MerchantFulfillmentConfig.model_validate({"bogus": "x"}) + config = BusinessFulfillmentConfig.model_validate({"bogus": "x"}) self.assertEqual(config.model_extra, {"bogus": "x"}) From 21ab4714bf30caf6358e464d76b405890c353dc6 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Fri, 28 Aug 2026 09:46:49 -0400 Subject: [PATCH 2/4] test: add failing coverage for fulfillment_method destination retyping fulfillment_method.json's destinations property is typed via items.$ref to fulfillment_destination.json (bare type: str, id: str), but two if/then allOf branches retype it per the method's own type: when type is shipping, destinations should really be shipping_destination.json items (postal address fields, type const shipping_address); when type is pickup, destinations should really be location_destination.json items (type const business_location). The committed FulfillmentMethod model ignores both branches entirely, so a shipping method can list a destination typed business_location (or vice versa) and it validates in violation of the schema. This is a third if/then shape neither find_conditional_required (adds required fields) nor find_conditional_bounds (narrows numeric ranges) handles: a discriminator retyping an ARRAY PROPERTY's items to a different referenced schema file entirely. No scanner in postprocess_models.py ever looked for it. Adds, mirroring the existing conditional-rule injector tests: - ConditionalArrayRetypingInjectorTest: injector-level unit tests against synthetic fixtures mirroring fulfillment_method.json's exact shape (method/destination/shipping_destination/ location_destination), covering the schema scan (both branches read, a branch whose ref matches the base is not a retype, a stripped request-variant field is inapplicable not malformed, an unresolvable $ref warns), injection idempotency, and the injected validator's runtime behavior against synthetic Method/Destination classes. - FulfillmentMethodDestinationRetypingSemanticTest: exercises the real committed FulfillmentMethod and FulfillmentDestination models. Includes negative controls for an open-vocabulary type (no rule applies) and no destinations at all (unconstrained). RED (test-only; find_conditional_array_retyping and inject_conditional_array_retyping do not exist on this commit -- the generator fix that adds them, developed alongside these tests per the exact schema shape confirmed against the pinned 2026-08-25 UCP spec, follows in the next commit): 101 tests, 2 failures + 6 errors, 4 documented skips (unchanged from the root-cause-0 commit). --- tests/test_codegen_pipeline.py | 354 +++++++++++++++++++++++++++++++++ 1 file changed, 354 insertions(+) diff --git a/tests/test_codegen_pipeline.py b/tests/test_codegen_pipeline.py index 665e7ac..eb3b7e6 100644 --- a/tests/test_codegen_pipeline.py +++ b/tests/test_codegen_pipeline.py @@ -1383,6 +1383,268 @@ def test_injected_validator_enforces_conditional_bounds(self): total(type="total", amount=-5) +class ConditionalArrayRetypingInjectorTest(unittest.TestCase): + """A discriminator retyping an array property's items to a different + referenced schema file is a third if/then shape, distinct from + conditional-required and conditional-bounds above. This mirrors + fulfillment_method.json: the base `destinations` property is typed via + `items.$ref` to fulfillment_destination.json, but a `shipping` method's + destinations should really be shipping_destination.json items (postal + address fields, `type` const `shipping_address`) and a `pickup` + method's should really be location_destination.json items (`type` + const `business_location`). The generator drops both retyping branches + entirely -- no scanner in this module ever looked for this shape. + """ + + MODULE = ( + "from __future__ import annotations\n" + "\n" + "from pydantic import BaseModel, ConfigDict\n" + "\n" + "from . import destination\n" + "\n" + "\n" + "class Method(BaseModel):\n" + ' model_config = ConfigDict(extra="allow")\n' + " type: str\n" + " destinations: list[destination.Destination] | None = None\n" + ) + RULES = [ + { + "discriminator": "type", + "values": ["shipping"], + "field": "destinations", + "required": ["id", "type"], + "consts": {"type": "shipping_address"}, + }, + { + "discriminator": "type", + "values": ["pickup"], + "field": "destinations", + "required": ["type"], + "consts": {"type": "business_location"}, + }, + ] + + def _schema(self): + return { + "title": "Method", + "type": "object", + "properties": { + "type": {"type": "string"}, + "destinations": { + "type": "array", + "items": {"$ref": "destination.json"}, + }, + }, + "allOf": [ + { + "if": { + "properties": {"type": {"const": "shipping"}}, + "required": ["type"], + }, + "then": { + "properties": { + "destinations": { + "type": "array", + "items": {"$ref": "shipping_destination.json"}, + } + } + }, + }, + { + "if": { + "properties": {"type": {"const": "pickup"}}, + "required": ["type"], + }, + "then": { + "properties": { + "destinations": { + "type": "array", + "items": {"$ref": "location_destination.json"}, + } + } + }, + }, + ], + } + + def _write_schema_tree(self, tmp): + Path(tmp, "method.json").write_text(json.dumps(self._schema())) + Path(tmp, "destination.json").write_text( + json.dumps( + { + "title": "Destination", + "type": "object", + "required": ["id", "type"], + "properties": { + "id": {"type": "string"}, + "type": {"type": "string"}, + }, + } + ) + ) + Path(tmp, "shipping_destination.json").write_text( + json.dumps( + { + "title": "Shipping Destination", + "type": "object", + "required": ["id", "type"], + "properties": { + "id": {"type": "string"}, + "type": {"type": "string", "const": "shipping_address"}, + }, + "allOf": [{"$ref": "postal_address.json"}], + } + ) + ) + Path(tmp, "location_destination.json").write_text( + json.dumps( + { + "title": "Business Location Destination", + "type": "object", + "required": ["type"], + "properties": { + "type": {"type": "string", "const": "business_location"} + }, + "allOf": [{"$ref": "location_summary.json"}], + } + ) + ) + + def test_schema_scan_reads_both_retyping_branches(self): + with tempfile.TemporaryDirectory() as tmp: + self._write_schema_tree(tmp) + found = postprocess_models.find_conditional_array_retyping( + Path(tmp) + ) + self.assertEqual(found, {"Method": self.RULES}) + + def test_schema_scan_ignores_branch_matching_the_base_ref(self): + # A then.properties..items.$ref identical to the base ref is + # not a retype -- nothing to approximate. The sibling pickup branch + # (still a genuine retype) is unaffected. + schema = self._schema() + schema["allOf"][0]["then"]["properties"]["destinations"]["items"][ + "$ref" + ] = "destination.json" + with tempfile.TemporaryDirectory() as tmp: + self._write_schema_tree(tmp) + Path(tmp, "method.json").write_text(json.dumps(schema)) + found = postprocess_models.find_conditional_array_retyping( + Path(tmp) + ) + self.assertEqual(found, {"Method": [self.RULES[1]]}) + + def test_schema_scan_skips_rule_whose_field_was_stripped(self): + # A request variant that omits `destinations` entirely (as + # fulfillment_method_create_request.json does) makes the rule + # inapplicable, not malformed -- no warning, and no rule recorded + # for the variant's own class. + schema = self._schema() + schema["title"] = "Method Create Request" + del schema["properties"]["destinations"] + with tempfile.TemporaryDirectory() as tmp: + self._write_schema_tree(tmp) + Path(tmp, "method_create_request.json").write_text( + json.dumps(schema) + ) + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr): + found = postprocess_models.find_conditional_array_retyping( + Path(tmp) + ) + self.assertNotIn("MethodCreateRequest", found) + self.assertEqual(found, {"Method": self.RULES}) + self.assertNotIn("unsupported", stderr.getvalue()) + + def test_schema_scan_warns_when_retyped_ref_cannot_be_loaded(self): + schema = self._schema() + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, "method.json").write_text(json.dumps(schema)) + Path(tmp, "destination.json").write_text( + json.dumps({"title": "Destination", "type": "object"}) + ) + # shipping_destination.json / location_destination.json are + # deliberately absent. + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr): + found = postprocess_models.find_conditional_array_retyping( + Path(tmp) + ) + self.assertEqual(found, {}) + self.assertIn("could not be loaded", stderr.getvalue()) + + def test_injection_is_idempotent(self): + once = postprocess_models.inject_conditional_array_retyping( + self.MODULE, "Method", self.RULES + ) + twice = postprocess_models.inject_conditional_array_retyping( + once, "Method", self.RULES + ) + self.assertEqual(once, twice) + + @unittest.skipUnless(HAVE_SDK, "executing the module needs pydantic") + def test_injected_validator_enforces_retyping(self): + module = ( + "from __future__ import annotations\n" + "\n" + "from pydantic import BaseModel, ConfigDict\n" + "\n" + "\n" + "class Destination(BaseModel):\n" + ' model_config = ConfigDict(extra="allow")\n' + " type: str\n" + " id: str\n" + "\n" + "\n" + "class Method(BaseModel):\n" + ' model_config = ConfigDict(extra="allow")\n' + " type: str\n" + " destinations: list[Destination] | None = None\n" + ) + out = postprocess_models.inject_conditional_array_retyping( + module, "Method", self.RULES + ) + namespace: dict = {} + exec(compile(out, "", "exec"), namespace) # noqa: S102 + # Forward reference (from __future__ import annotations): Method's + # "destinations: list[Destination]" annotation resolves once both + # classes exist in the exec'd namespace. + namespace["Method"].model_rebuild(_types_namespace=namespace) + method_cls = namespace["Method"] + destination_cls = namespace["Destination"] + with self.assertRaises(ValidationError): + method_cls( + type="shipping", + destinations=[ + destination_cls(type="business_location", id="d1") + ], + ) + with self.assertRaises(ValidationError): + method_cls( + type="pickup", + destinations=[ + destination_cls(type="shipping_address", id="d1") + ], + ) + method_cls( + type="shipping", + destinations=[destination_cls(type="shipping_address", id="d1")], + ) + method_cls( + type="pickup", + destinations=[destination_cls(type="business_location", id="d1")], + ) + # A type carrying no rule is unconstrained (open vocabulary). + method_cls( + type="courier", + destinations=[destination_cls(type="anything", id="d1")], + ) + # No destinations at all is unconstrained regardless of type. + method_cls(type="shipping") + + class InjectorTest(unittest.TestCase): """The post-generation injector's own behavior.""" @@ -2172,5 +2434,97 @@ def test_payment_handler_base_rejects_invalid_version(self) -> None: Base.model_validate({"version": {"not": "a version"}}) +@unittest.skipUnless( + HAVE_SDK, "requires the installed package (pip install -e .)" +) +class FulfillmentMethodDestinationRetypingSemanticTest(unittest.TestCase): + """fulfillment_method.json retypes `destinations` per `type`: a + `shipping` method's destinations are shipping_destination.json items + (`type` const `shipping_address`), a `pickup` method's are + location_destination.json items (`type` const `business_location`). + The committed FulfillmentMethod model, before this fix, accepted any + FulfillmentDestination (bare `type: str`, `id: str`) regardless of the + method's own type, so a `shipping` method could list a + `business_location` destination and it would validate. + """ + + def _method(self): + from ucp_sdk.models.schemas.shopping.types.fulfillment_method import ( + FulfillmentMethod, + ) + + return FulfillmentMethod + + def _destination(self): + from ucp_sdk.models.schemas.shopping.types.fulfillment_destination import ( + FulfillmentDestination, + ) + + return FulfillmentDestination + + def test_shipping_method_with_business_location_destination_rejected( + self, + ): + with self.assertRaises(ValidationError): + self._method()( + id="m1", + type="shipping", + line_item_ids=["li1"], + destinations=[ + self._destination()(type="business_location", id="d1") + ], + ) + + def test_pickup_method_with_shipping_address_destination_rejected(self): + with self.assertRaises(ValidationError): + self._method()( + id="m2", + type="pickup", + line_item_ids=["li1"], + destinations=[ + self._destination()(type="shipping_address", id="d1") + ], + ) + + def test_shipping_method_with_shipping_address_destination_accepted( + self, + ): + method = self._method()( + id="m1", + type="shipping", + line_item_ids=["li1"], + destinations=[ + self._destination()(type="shipping_address", id="d1") + ], + ) + self.assertEqual(method.destinations[0].type, "shipping_address") + + def test_pickup_method_with_business_location_destination_accepted(self): + method = self._method()( + id="m2", + type="pickup", + line_item_ids=["li1"], + destinations=[ + self._destination()(type="business_location", id="d1") + ], + ) + self.assertEqual(method.destinations[0].type, "business_location") + + def test_method_type_outside_the_pinned_vocabulary_is_unconstrained( + self, + ): + # type is an open vocabulary ("Businesses MAY use additional + # values"); only shipping/pickup carry a retyping rule. + self._method()( + id="m3", + type="curbside", + line_item_ids=["li1"], + destinations=[self._destination()(type="anything", id="d1")], + ) + + def test_method_without_destinations_is_unconstrained(self): + self._method()(id="m4", type="shipping", line_item_ids=["li1"]) + + if __name__ == "__main__": unittest.main() From a94e5e7b9b8ffeae2f13d95c7323cc1f9769f887 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Fri, 28 Aug 2026 09:47:23 -0400 Subject: [PATCH 3/4] fix(codegen): approximate discriminator array-item retyping Adds an eighth constraint family: a discriminator retyping an array PROPERTY's items to a schema file different from the property's own base $ref, a shape neither find_conditional_required (adds required fields) nor find_conditional_bounds (narrows numeric ranges) handles. fulfillment_method.json's destinations stays typed to the base FulfillmentDestination (bare type: str, id: str) regardless of type, even though a shipping method's destinations are really ShippingDestination (postal address fields, type const shipping_address) and a pickup method's are really LocationDestination (type const business_location). Pydantic has no clean way to retype a field's item type from a source-text splice the way the other families rewrite an annotation or add a validator against the field's own declared type, so this is enforced with a runtime check instead of a static type change: find_conditional_array_retyping scans for the shape (single-key const/enum discriminator naming a property on the enclosing object, then narrowing exactly one array property, also on the enclosing object, to a different items.$ref) and resolves the retyped file's own (root-level, post-merge) required keys and const-pinned properties via _resolve_referenced_shape. inject_conditional_array_retyping then injects a model_validator that, for each item in the array field when the discriminator matches, checks the item carries every required key (via model_fields_set | model_extra, same key-counting idiom used throughout this module) and that every const-pinned field matches its expected value. This is deliberately an approximation, not a full re-derivation of the retyped type: only the referenced schema's own required/const fields are checked, not fields it in turn allOf-references (shipping_ destination.json's own allOf-ref to postal_address.json is not inspected). For fulfillment_method.json the meaningful check is primarily the type const pin -- id/type were already required by the base FulfillmentDestination, so checking them again is redundant here, but the mechanism is general and both required-keys and const-pins are checked mechanically from whatever the referenced schema declares, not hardcoded to this one case. A request variant that omits the retyped field entirely (fulfillment_ method_create_request.json never carries destinations at all -- ucp_request: "omit") makes the rule inapplicable rather than malformed, silently, mirroring find_conditional_bounds's existing "stripped field" handling. 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 (all injector-level unit tests from the RED commit -- which exercise find_conditional_array_retyping/inject_conditional_array_ retyping directly against synthetic fixtures mirroring the real schema shape, not the committed models -- already pass). 101 tests, 2 failures (FulfillmentMethodDestinationRetypingSemanticTest), 4 documented skips. --- postprocess_models.py | 301 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 299 insertions(+), 2 deletions(-) diff --git a/postprocess_models.py b/postprocess_models.py index 84d35e7..4e96632 100644 --- a/postprocess_models.py +++ b/postprocess_models.py @@ -14,7 +14,7 @@ """Post-generation fixes for constraints datamodel-code-generator ignores. -Seven constraint families are handled: +Eight constraint families are handled: * ``minProperties`` on an object schema WITH declared properties is dropped by the generator (issue #49): every field is optional, so an empty instance @@ -83,6 +83,24 @@ fields were stripped by request-variant projection is inapplicable rather than malformed and is skipped silently. +* A discriminator retyping an array PROPERTY's items to a schema file + different from the property's own base ``$ref`` is dropped entirely, a + third if/then shape distinct from the required-fields and numeric-bounds + families above. ``fulfillment_method.json``'s ``destinations`` stays typed + to the base ``FulfillmentDestination`` regardless of ``type``, even though + a `shipping` method's destinations are really ``ShippingDestination`` + (postal address fields, `type` const `shipping_address`) and a `pickup` + method's are really ``LocationDestination`` (`type` const + `business_location`) — so a `shipping` method can currently list a + destination typed `business_location` and it validates. Pydantic has no + clean way to retype a field's item type from a source-text splice, so + this is enforced with a runtime check instead of a static type change: + each item is checked against the referenced schema's own (root-level, + post-merge) required keys and const-pinned properties — an approximation, + not a full re-derivation of the retyped type (a schema the retyped file + itself ``allOf``-references, e.g. ``postal_address.json``, is not + inspected). + * ``additionalProperties: false`` on an object schema with named properties is normally overridden by the generator's ``--extra-fields=allow`` flag. The script detects schemas with ``additionalProperties: false`` and flips their @@ -197,6 +215,47 @@ def {marker}(self): return self ''' +_RETYPE_MARKER = "_enforce_conditional_item_retyping" + +_RETYPE_TEMPLATE = ''' + @model_validator(mode="after") + def {marker}(self): + """JSON Schema if/then: approximate a discriminator's array-item + retyping to a different referenced schema, via that schema's own + required keys and const-pinned fields.""" + rules = {rules!r} + for rule in rules: + actual = getattr(self, rule["discriminator"], None) + if actual not in rule["values"]: + continue + for _item in getattr(self, rule["field"], None) or []: + _provided = ( + set(_item.keys()) + if isinstance(_item, dict) + else _item.model_fields_set | set(_item.model_extra or {{}}) + ) + for _required in rule["required"]: + if _required not in _provided: + raise ValueError( + f"Field {{_required!r}} is required for " + f"{{rule['field']}} items when " + f"{{rule['discriminator']}} is {{actual!r}}" + ) + for _const_field, _const_value in rule["consts"].items(): + _actual_value = ( + _item.get(_const_field) + if isinstance(_item, dict) + else getattr(_item, _const_field, None) + ) + if _actual_value != _const_value: + raise ValueError( + f"Field {{_const_field!r}} must equal " + f"{{_const_value!r}} for {{rule['field']}} items " + f"when {{rule['discriminator']}} is {{actual!r}}" + ) + return self +''' + _UNIQUE_VALIDATOR_TEMPLATE = ''' @field_validator("{field}", mode="after") def {marker}_{field}(cls, value): # noqa: N805 @@ -987,6 +1046,207 @@ def inject_conditional_bounds(source, class_name, rules): return _ensure_pydantic_import(out, "model_validator") +def _resolve_referenced_shape(ref, schema_path): + """Load ``ref`` (relative to ``schema_path``) and return its own + (root-level, post-merge) required keys and const-pinned properties. + + Returns ``None`` if the file cannot be loaded. Deliberately shallow: it + reads only the referenced schema's own ``required``/``properties``, not + anything it in turn ``allOf``-references (e.g. shipping_destination.json + ``allOf``-refs postal_address.json, whose fields are not inspected) -- + an approximation, not a full re-derivation of the retyped shape. + """ + file_part = ref.split("#", 1)[0] + if not file_part: + return None + target_path = (Path(schema_path).parent / file_part).resolve() + try: + referenced = json.loads(target_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + sys.stderr.write( + f" ! {schema_path}: retyped $ref {ref!r} could not be loaded; " + "rule skipped\n" + ) + return None + if not isinstance(referenced, dict): + return None + required = sorted( + name for name in referenced.get("required", []) if isinstance(name, str) + ) + consts = { + name: prop["const"] + for name, prop in (referenced.get("properties") or {}).items() + if isinstance(prop, dict) and "const" in prop + } + return {"required": required, "consts": consts} + + +def _is_ref_array(node): + """True when ``node`` is an array property typed via ``items.$ref``.""" + return ( + isinstance(node, dict) + and node.get("type") == "array" + and isinstance(node.get("items"), dict) + and isinstance(node["items"].get("$ref"), str) + ) + + +def _describe_retyping_branch(branch, properties, schema_path): + """Describe one allOf if/then branch that retypes an array property's + items to a schema file different from the property's own base ``$ref``. + + Mechanical and narrow by design, mirroring the other conditional + scanners in this module: a single-key ``const``/``enum`` discriminator + naming a property present on the enclosing object, and a ``then`` that + narrows exactly one array property (also present on the enclosing + object) to a different ``items.$ref``. Anything else -- multiple + discriminators, a non-array or non-$ref field, a `then` naming a field + absent from the enclosing object (a request variant that omits it, as + fulfillment_method_create_request.json does for ``destinations``) -- + returns ``None`` silently: those are either a different rule shape + (left to find_conditional_required/find_conditional_bounds, which scan + the same branches) or legitimately inapplicable, not malformed. + """ + if not isinstance(branch, dict) or set(branch) != {"if", "then"}: + return None + condition = branch["if"] + consequence = branch["then"] + if ( + not isinstance(condition, dict) + or set(condition) != {"properties", "required"} + or not isinstance(consequence, dict) + or set(consequence) != {"properties"} + ): + return None + condition_props = condition["properties"] + condition_required = condition["required"] + if ( + not isinstance(condition_props, dict) + or len(condition_props) != 1 + or not isinstance(condition_required, list) + or len(condition_required) != 1 + ): + return None + discriminator, predicate = next(iter(condition_props.items())) + if condition_required != [discriminator] or not isinstance(predicate, dict): + return None + if set(predicate) == {"const"}: + values = [predicate["const"]] + elif ( + set(predicate) == {"enum"} + and isinstance(predicate["enum"], list) + and predicate["enum"] + ): + values = predicate["enum"] + else: + return None + if discriminator not in properties or any( + not isinstance(value, (str, int, float, bool)) for value in values + ): + return None + consequence_props = consequence["properties"] + if not isinstance(consequence_props, dict) or len(consequence_props) != 1: + return None + field, field_schema = next(iter(consequence_props.items())) + if field not in properties or not _is_ref_array(field_schema): + return None + base_field_schema = properties[field] + if not _is_ref_array(base_field_schema): + return None + base_ref = base_field_schema["items"]["$ref"] + new_ref = field_schema["items"]["$ref"] + if new_ref == base_ref: + return None + target = _resolve_referenced_shape(new_ref, schema_path) + if target is None: + return None + return { + "discriminator": discriminator, + "values": values, + "field": field, + "required": target["required"], + "consts": target["consts"], + } + + +def find_conditional_array_retyping(schema_dir): + """Map generated class names to array-item retyping rules. + + Complements find_conditional_required/find_conditional_bounds, which + only handle a ``then`` that adds required fields or narrows a numeric + range. A ``then`` that instead retypes an array PROPERTY's items to a + schema file different from the property's own base ``$ref`` is a third + shape the generator drops entirely: fulfillment_method.json's + ``destinations`` stays typed to the base FulfillmentDestination + regardless of ``type``, even though a `shipping` method's destinations + are really ShippingDestination (postal address fields, `type` const + `shipping_address`) and a `pickup` method's are really + LocationDestination (`type` const `business_location`). Pydantic has no + clean way to retype a field's item type from a source-text splice, so + this is enforced with a runtime check instead of a static type change: + each item is checked against the referenced schema's own required keys + and const-pinned fields (see _resolve_referenced_shape), an + approximation rather than a full re-derivation of the retyped type. + """ + rules_by_class = {} + + def walk(node, current_class_name, schema_path): + if not isinstance(node, dict): + return + if isinstance(node.get("title"), str): + current_class_name = _alias_name(node["title"]) + properties = node.get("properties") + allof = node.get("allOf") + if isinstance(properties, dict) and isinstance(allof, list): + for branch in allof: + rule = _describe_retyping_branch( + branch, properties, schema_path + ) + if rule is not None and current_class_name is not None: + rules_by_class.setdefault(current_class_name, []).append( + rule + ) + if isinstance(properties, dict): + for name, prop in properties.items(): + walk(prop, _to_camel_case(name), schema_path) + defs = node.get("$defs") + if isinstance(defs, dict): + for def_name, def_node in defs.items(): + walk(def_node, _to_camel_case(def_name), schema_path) + + for path in sorted(Path(schema_dir).rglob("*.json")): + try: + schema = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + continue + if not isinstance(schema, dict): + continue + root_title = schema.get("title") + initial_class = ( + _alias_name(root_title) if root_title else _to_camel_case(path.stem) + ) + walk(schema, initial_class, path) + return rules_by_class + + +def inject_conditional_array_retyping(source, class_name, rules): + """Inject array-item retyping checks into one generated class.""" + class_re = re.compile(rf"^class {re.escape(class_name)}\(", re.M) + match = class_re.search(source) + if not match: + return source + tail = re.compile(r"^\S", re.M) + end_match = tail.search(source, match.end()) + end = end_match.start() if end_match else len(source) + if f"def {_RETYPE_MARKER}(" in source[match.start() : end]: + return source + method = _RETYPE_TEMPLATE.format(marker=_RETYPE_MARKER, rules=rules) + body = source[:end].rstrip("\n") + rest = source[end:] + out = body + "\n" + method + ("\n" + rest if rest else "") + return _ensure_pydantic_import(out, "model_validator") + + def find_unique_items_fields(schema_dir): """Map generated class names to fields carrying ``uniqueItems``. @@ -1320,6 +1580,41 @@ def _patch_conditional_bounds(): return patched, 0 +def _patch_conditional_array_retyping(): + """Inject array-item retyping checks; return counts and status.""" + rules_by_class = find_conditional_array_retyping(SCHEMA_DIR) + if not rules_by_class: + sys.stdout.write( + "postprocess: no conditional array-item retyping rules found\n" + ) + return 0, 0 + patched = 0 + for class_name, rules in sorted(rules_by_class.items()): + hits = [] + for path in sorted(OUTPUT_DIR.rglob("*.py")): + source = path.read_text(encoding="utf-8") + if not re.search( + rf"^class {re.escape(class_name)}\(", source, re.M + ): + continue + updated = inject_conditional_array_retyping( + source, class_name, rules + ) + if updated != source: + path.write_text(updated, encoding="utf-8") + patched += 1 + hits.append(path) + label = ( + ", ".join(str(path) for path in hits) or "NO GENERATED CLASS FOUND" + ) + sys.stdout.write( + f" conditional array-item retyping on '{class_name}' -> {label}\n" + ) + if not hits: + return patched, 1 + return patched, 0 + + def _patch_unique_items(): """Inject uniqueItems validators; return (patched_count, exit_code).""" unique_fields_by_class = find_unique_items_fields(SCHEMA_DIR) @@ -1460,6 +1755,7 @@ def main(): patched_ac, rc_ac = _patch_array_contains() patched_cr, rc_cr = _patch_conditional_required() patched_cb, rc_cb = _patch_conditional_bounds() + patched_rt, rc_rt = _patch_conditional_array_retyping() patched_ui, rc_ui = _patch_unique_items() patched_ef, rc_ef = _patch_extra_forbid() total = ( @@ -1468,11 +1764,12 @@ def main(): + patched_ac + patched_cr + patched_cb + + patched_rt + patched_ui + patched_ef ) sys.stdout.write(f"postprocess: {total} module(s) patched\n") - return rc_mp or rc_pn or rc_ac or rc_cr or rc_cb or rc_ui or rc_ef + return rc_mp or rc_pn or rc_ac or rc_cr or rc_cb or rc_rt or rc_ui or rc_ef if __name__ == "__main__": From eb5624c9428dbea16028ded199bb8769c16fcab6 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Fri, 28 Aug 2026 09:48:28 -0400 Subject: [PATCH 4/4] 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. One file changes: FulfillmentMethod gains an _enforce_conditional_item_retyping validator covering both the shipping and pickup destination retyping rules. Verified: - Full suite: 101 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 file: 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). --- .../shopping/types/fulfillment_method.py | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/src/ucp_sdk/models/schemas/shopping/types/fulfillment_method.py b/src/ucp_sdk/models/schemas/shopping/types/fulfillment_method.py index b9f7c06..820f5c5 100644 --- a/src/ucp_sdk/models/schemas/shopping/types/fulfillment_method.py +++ b/src/ucp_sdk/models/schemas/shopping/types/fulfillment_method.py @@ -18,7 +18,7 @@ from __future__ import annotations -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, model_validator from . import fulfillment_destination, fulfillment_group @@ -57,3 +57,55 @@ class FulfillmentMethod(BaseModel): """ Fulfillment groups for selecting options. Agent sets selected_option_id on groups to choose shipping method. """ + + @model_validator(mode="after") + def _enforce_conditional_item_retyping(self): + """JSON Schema if/then: approximate a discriminator's array-item + retyping to a different referenced schema, via that schema's own + required keys and const-pinned fields.""" + rules = [ + { + "discriminator": "type", + "values": ["shipping"], + "field": "destinations", + "required": ["id", "type"], + "consts": {"type": "shipping_address"}, + }, + { + "discriminator": "type", + "values": ["pickup"], + "field": "destinations", + "required": ["type"], + "consts": {"type": "business_location"}, + }, + ] + for rule in rules: + actual = getattr(self, rule["discriminator"], None) + if actual not in rule["values"]: + continue + for _item in getattr(self, rule["field"], None) or []: + _provided = ( + set(_item.keys()) + if isinstance(_item, dict) + else _item.model_fields_set | set(_item.model_extra or {}) + ) + for _required in rule["required"]: + if _required not in _provided: + raise ValueError( + f"Field {_required!r} is required for " + f"{rule['field']} items when " + f"{rule['discriminator']} is {actual!r}" + ) + for _const_field, _const_value in rule["consts"].items(): + _actual_value = ( + _item.get(_const_field) + if isinstance(_item, dict) + else getattr(_item, _const_field, None) + ) + if _actual_value != _const_value: + raise ValueError( + f"Field {_const_field!r} must equal " + f"{_const_value!r} for {rule['field']} items " + f"when {rule['discriminator']} is {actual!r}" + ) + return self