From 590855c79d81a24b4d41d28653b70271ebd277af Mon Sep 17 00:00:00 2001 From: FanouZeng-TT <18280587072@163.com> Date: Wed, 12 Aug 2026 16:39:50 +0800 Subject: [PATCH] fix: enforce additionalProperties:false in generated models --- postprocess_models.py | 125 +++++++++++- .../types/business_fulfillment_config.py | 2 +- .../schemas/shopping/types/error_response.py | 2 +- .../types/merchant_fulfillment_config.py | 2 +- tests/test_codegen_pipeline.py | 192 ++++++++++++++++++ 5 files changed, 317 insertions(+), 6 deletions(-) diff --git a/postprocess_models.py b/postprocess_models.py index 2262586..6b08219 100644 --- a/postprocess_models.py +++ b/postprocess_models.py @@ -14,7 +14,7 @@ """Post-generation fixes for constraints datamodel-code-generator ignores. -Four constraint families are handled: +Six 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 @@ -74,6 +74,12 @@ then injects a ``model_validator(mode="after")``. More complex conditions are skipped rather than approximated. +* ``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 + generated ``model_config`` to ``extra="forbid"`` while preserving + ``extra="allow"`` on sibling models in the same module. + Runs from generate_models.sh between generation and formatting; idempotent. """ @@ -1004,6 +1010,111 @@ def _patch_unique_items(): return unique_patched, 0 +def find_extra_forbid_class_names(schema_dir): + """Map generated class names for objects that forbid unknown keys. + + The gap this targets: an object schema that declares + ``additionalProperties: false`` AND carries named ``properties`` is still + emitted by the generator as ``BaseModel(extra="allow")`` (generation runs + with ``--extra-fields=allow``), so unknown keys are silently retained in + ``model_extra`` instead of being rejected. The rule is mechanical: an + object node with ``additionalProperties is False`` and non-empty named + ``properties`` maps to its generated class name via its ``title`` (root + objects) or its property path (untitled nested objects, e.g. + ``allows_multi_destination`` -> ``AllowsMultiDestination``). + """ + found = set() + + def visit(node, class_name): + if not isinstance(node, dict): + if isinstance(node, list): + for item in node: + visit(item, class_name) + return + effective = ( + _alias_name(node["title"]) if node.get("title") else class_name + ) + if ( + node.get("additionalProperties") is False + and isinstance(node.get("properties"), dict) + and node["properties"] + ): + found.add(effective) + for name, child in (node.get("properties") or {}).items(): + visit(child, _to_camel_case(name)) + + 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_name = ( + _alias_name(schema["title"]) + if schema.get("title") + else _to_camel_case(path.stem) + ) + visit(schema, root_name) + return found + + +def inject_extra_forbid(source, class_name): + """Flip the target class's ``extra="allow"`` config to ``extra="forbid"``. + + Only the named class's own ``model_config`` is changed (its body, from the + ``class`` statement to the next top-level ``class``/``def``), so sibling + classes in the same module keep ``extra="allow"``. The source is returned + unchanged when the class is absent or already ``extra="forbid"``. + """ + head = re.search( + rf"^class {re.escape(class_name)}\(BaseModel\):", source, re.M + ) + if not head: + return source + rest = source[head.end() :] + next_top = re.search(r"^(?=class |def )", rest, re.M) + body_end = len(rest) if next_top is None else next_top.start() + body = rest[:body_end] + if 'extra="allow"' not in body: + return source + new_body = body.replace('extra="allow"', 'extra="forbid"', 1) + return source[: head.end()] + new_body + rest[body_end:] + + +def _patch_extra_forbid(): + """Inject extra="forbid" on models whose schema forbids unknown keys.""" + class_names = find_extra_forbid_class_names(SCHEMA_DIR) + if not class_names: + sys.stdout.write( + "postprocess: no additionalProperties:false models found\n" + ) + return 0, 0 + patched = 0 + for class_name in sorted(class_names): + 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_extra_forbid(source, class_name) + if updated != source: + path.write_text(updated, encoding="utf-8") + patched += 1 + hits.append(path) + label = ", ".join(str(h) for h in hits) or "NO GENERATED CLASS FOUND" + sys.stdout.write(f" extra=forbid on '{class_name}' -> {label}\n") + if not hits: + sys.stderr.write( + f" ! '{class_name}' has no generated class; " + "constraint not enforced\n" + ) + return patched, 1 + return patched, 0 + + def main(): """Main entry point to scan schemas and patch generated models.""" patched_mp, rc_mp = _patch_min_properties() @@ -1011,9 +1122,17 @@ def main(): patched_ac, rc_ac = _patch_array_contains() patched_cr, rc_cr = _patch_conditional_required() patched_ui, rc_ui = _patch_unique_items() - total = patched_mp + patched_pn + patched_ac + patched_cr + patched_ui + patched_ef, rc_ef = _patch_extra_forbid() + total = ( + patched_mp + + patched_pn + + patched_ac + + patched_cr + + 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_ui + return rc_mp or rc_pn or rc_ac or rc_cr or rc_ui or rc_ef if __name__ == "__main__": diff --git a/src/ucp_sdk/models/schemas/shopping/types/business_fulfillment_config.py b/src/ucp_sdk/models/schemas/shopping/types/business_fulfillment_config.py index 53e3bd5..55a9007 100644 --- a/src/ucp_sdk/models/schemas/shopping/types/business_fulfillment_config.py +++ b/src/ucp_sdk/models/schemas/shopping/types/business_fulfillment_config.py @@ -29,7 +29,7 @@ class AllowsMultiDestination(BaseModel): """ model_config = ConfigDict( - extra="allow", + extra="forbid", ) shipping: bool | None = None """ diff --git a/src/ucp_sdk/models/schemas/shopping/types/error_response.py b/src/ucp_sdk/models/schemas/shopping/types/error_response.py index 3f02f0e..d4628a0 100644 --- a/src/ucp_sdk/models/schemas/shopping/types/error_response.py +++ b/src/ucp_sdk/models/schemas/shopping/types/error_response.py @@ -30,7 +30,7 @@ class ErrorResponse(BaseModel): """ model_config = ConfigDict( - extra="allow", + extra="forbid", ) ucp: ucp_1.UcpMetadata """ diff --git a/src/ucp_sdk/models/schemas/shopping/types/merchant_fulfillment_config.py b/src/ucp_sdk/models/schemas/shopping/types/merchant_fulfillment_config.py index b908e47..f4b1afd 100644 --- a/src/ucp_sdk/models/schemas/shopping/types/merchant_fulfillment_config.py +++ b/src/ucp_sdk/models/schemas/shopping/types/merchant_fulfillment_config.py @@ -29,7 +29,7 @@ class AllowsMultiDestination(BaseModel): """ model_config = ConfigDict( - extra="allow", + extra="forbid", ) shipping: bool | None = None """ diff --git a/tests/test_codegen_pipeline.py b/tests/test_codegen_pipeline.py index 2d516b8..ccdef8e 100644 --- a/tests/test_codegen_pipeline.py +++ b/tests/test_codegen_pipeline.py @@ -1472,5 +1472,197 @@ def test_brands_accepts_unique_and_none(self) -> None: self.assertIsNone(Constraints().brands) +class AdditionalPropertiesForbidFinderTest(unittest.TestCase): + """additionalProperties:false objects map to generated class names.""" + + def test_root_titled_object(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, "error_response.json").write_text( + json.dumps( + { + "title": "Error Response", + "type": "object", + "additionalProperties": False, + "properties": {"messages": {"type": "array"}}, + } + ), + encoding="utf-8", + ) + names = postprocess_models.find_extra_forbid_class_names(Path(tmp)) + self.assertEqual(names, {"ErrorResponse"}) + + def test_nested_untitled_object_uses_property_path(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, "merchant_fulfillment_config.json").write_text( + json.dumps( + { + "title": "Merchant Fulfillment Config", + "type": "object", + "properties": { + "allows_multi_destination": { + "type": "object", + "additionalProperties": False, + "properties": {"shipping": {"type": "boolean"}}, + } + }, + } + ), + encoding="utf-8", + ) + names = postprocess_models.find_extra_forbid_class_names(Path(tmp)) + self.assertEqual(names, {"AllowsMultiDestination"}) + + def test_loose_and_map_objects_are_excluded(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, "open.json").write_text( + json.dumps( + { + "title": "Open Object", + "type": "object", + "properties": {"a": {"type": "string"}}, + } + ), + encoding="utf-8", + ) + Path(tmp, "map.json").write_text( + json.dumps( + { + "title": "Map Object", + "type": "object", + "additionalProperties": {"type": "string"}, + "properties": {"a": {"type": "string"}}, + } + ), + encoding="utf-8", + ) + names = postprocess_models.find_extra_forbid_class_names(Path(tmp)) + self.assertEqual(names, set()) + + +class AdditionalPropertiesForbidInjectorTest(unittest.TestCase): + """The injector flips only the target class's model_config to forbid.""" + + SOURCE = '''\ +class AllowsMultiDestination(BaseModel): + """ + Permits multiple destinations per method type. + """ + + model_config = ConfigDict( + extra="allow", + ) + shipping: bool | None = None + + +class MerchantFulfillmentConfig(BaseModel): + """ + Merchant's fulfillment configuration. + """ + + model_config = ConfigDict( + extra="allow", + ) + allows_multi_destination: AllowsMultiDestination | None = None +''' + + def test_flips_only_target_class(self) -> None: + updated = postprocess_models.inject_extra_forbid( + self.SOURCE, "AllowsMultiDestination" + ) + # Target class body now forbids extra keys. + self.assertIn('extra="forbid"', updated) + # The sibling class in the same module keeps extra="allow". + sibling = """class MerchantFulfillmentConfig(BaseModel): + \"\"\" + Merchant's fulfillment configuration. + \"\"\" + + model_config = ConfigDict( + extra="allow", + )""" + self.assertIn(sibling, updated) + + def test_idempotent_after_flip(self) -> None: + once = postprocess_models.inject_extra_forbid( + self.SOURCE, "AllowsMultiDestination" + ) + twice = postprocess_models.inject_extra_forbid( + once, "AllowsMultiDestination" + ) + self.assertEqual(once, twice) + + def test_unknown_class_untouched(self) -> None: + self.assertEqual( + postprocess_models.inject_extra_forbid(self.SOURCE, "Nope"), + self.SOURCE, + ) + + +@unittest.skipUnless( + HAVE_SDK, "requires the installed package (pip install -e .)" +) +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 ( + ErrorResponse, + ) + + with self.assertRaises(ValidationError): + ErrorResponse.model_validate( + { + "ucp": {"version": "2026-04-08", "status": "error"}, + "messages": [ + { + "type": "error", + "code": "not_found", + "severity": "unrecoverable", + "content": "boom", + } + ], + "bogus": "x", + } + ) + + def test_error_response_accepts_declared_fields(self) -> None: + from ucp_sdk.models.schemas.shopping.types.error_response import ( + ErrorResponse, + ) + + obj = ErrorResponse.model_validate( + { + "ucp": {"version": "2026-04-08", "status": "error"}, + "messages": [ + { + "type": "error", + "code": "not_found", + "severity": "unrecoverable", + "content": "boom", + } + ], + } + ) + self.assertEqual(obj.messages[0].content, "boom") + + def test_allows_multi_destination_rejects_unknown_keys(self) -> None: + from ucp_sdk.models.schemas.shopping.types.merchant_fulfillment_config import ( + AllowsMultiDestination, + ) + + with self.assertRaises(ValidationError): + AllowsMultiDestination.model_validate( + {"shipping": True, "bogus": "x"} + ) + + def test_sibling_config_keeps_extra_allow(self) -> None: + from ucp_sdk.models.schemas.shopping.types.merchant_fulfillment_config import ( + MerchantFulfillmentConfig, + ) + + config = MerchantFulfillmentConfig.model_validate({"bogus": "x"}) + self.assertEqual(config.model_extra, {"bogus": "x"}) + + if __name__ == "__main__": unittest.main()