From c5df93ea5190bfde220c00ffe3525a7480501e01 Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Wed, 9 Sep 2026 13:02:07 -0400 Subject: [PATCH] refactor(models)!: relax the schema-v2 mirrors from extra="forbid" to extra="ignore" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking up an additive analyzer release used to fail the whole payload rather than the one field it added. codeanalyzer-java 3.1.2 put var on param_in and param_out — backward compatible by construction — and JAnalysis produced 2515 validation errors on daytrader8 until JParamEdge declared it. Uptake should not require an SDK edit before anything parses at all. What this gives up, stated plainly because it is the whole cost: forbid was the drift detector. It surfaced that var within seconds of the pin bump. Under ignore the same addition is absorbed silently, and the first symptom is a wrong answer from something reading a field the SDK never learned about — slow and quiet in place of loud and immediate. The intended replacement is a comparison against each analyzer's published schema in codeanalyzer-schema, reporting what is unmodelled rather than refusing to parse; that is tracked separately. Scoped narrower than the whole codebase, on a distinction that matters: relaxed _Base in models/java and models/typescript — the analyzer mirrors, validated from analysis.json — plus JCompilationUnit, which overrides model_config wholesale for its alias settings so _Base's change never reached it (a subclass config replaces rather than merges). kept JCallableOverview and JClassOverview in projections.py. Nothing calls model_validate on them; they are built by this SDK from graph rows and never parsed from the wire. Strictness there catches our own typo'd kwarg, not the analyzer's additions, so relaxing it would surrender a real guard for no uptake benefit. ignore rather than allow, deliberately: allow keeps unknown fields in model_extra and so widens model_dump_json() with whatever the analyzer emitted, and several tests assert properties OF dumps (E6's "can://" not in ...model_dump_json()). A field the SDK does not model must not be able to change what a dump contains. Every new test asserts model_extra is empty for that reason. One thing this does NOT cost, now asserted rather than assumed: extra does not govern required fields, so "a 1.x analysis.json is refused, not parsed" survives intact — a v1 payload still fails, because it LACKS what v2 requires rather than carrying extras. test_v1_shaped_payload_is_rejected passes unchanged in both languages, and the new tests say so next to it, so the two mechanisms are not confused for each other the next time this policy is weighed. Four tests asserted the old policy and none of them contained the word "forbid" — they were named for the behaviour, so only running the suite found them. Replaced rather than deleted, because the surviving half is worth writing down: each now asserts that an undeclared field is accepted AND dropped, so a caller reaching for analyzer data the SDK does not model learns it finds nothing rather than an error. Two comments were actively misleading and are fixed: "extra=forbid: this line is the assertion" annotated a bare _load(level) that now asserts nothing, and the test it sat in claimed parsing proved no undeclared field had appeared. That test is renamed, and its docstring now says the positive assertions carry it and that a silent pass is the accepted cost of this change rather than an oversight. Gate: 1599 passed, 370 skipped, no failures (1597 + 2 net). Closes #386 --- cldk/models/java/models.py | 31 +++++- cldk/models/typescript/models.py | 26 ++++- tests/models/java/test_java_models.py | 45 +++++++-- tests/models/typescript/test_ts_v2_models.py | 97 +++++++++++++++++-- .../resources/java/analysis_json/v2/README.md | 8 +- .../typescript/analysis_json/v2/README.md | 3 +- 6 files changed, 182 insertions(+), 28 deletions(-) diff --git a/cldk/models/java/models.py b/cldk/models/java/models.py index 6c64f8e..240fe65 100644 --- a/cldk/models/java/models.py +++ b/cldk/models/java/models.py @@ -32,7 +32,11 @@ ``refs``, the ``is_*`` type predicates over ``kind`` and the owner chain. What the wire does not carry (CRUD) is an empty list, and the facade raises for it (J-4). -``extra="forbid"`` is intentional: drift between the analyzer's JSON and these models fails loudly. +``extra="ignore"`` since #386: an additive analyzer release is consumable without an SDK edit. What +that gives up is the drift detector — an unmodelled field is dropped silently, so a field the SDK does +not declare is unreachable rather than an error. The projections in ``projections.py`` keep +``extra="forbid"``, because they are constructed by this SDK and never validated from the wire, so +there strictness guards our own typos rather than the analyzer's additions. """ from __future__ import annotations @@ -46,7 +50,26 @@ class _Base(BaseModel): - model_config = ConfigDict(extra="forbid") + #: ``ignore``, not ``forbid`` (#386). An additive analyzer release used to fail the whole payload + #: rather than the one field it added: codeanalyzer-java 3.1.2's ``var`` on ``param_in``/ + #: ``param_out`` produced 2515 validation errors on daytrader8 until ``JParamEdge`` declared it, + #: for a change that was backward compatible by construction. Uptake should not require an SDK + #: edit before anything parses. + #: + #: What this gives up is the drift detector. ``forbid`` was what surfaced that ``var`` within + #: seconds of the pin bump; under ``ignore`` the same addition is absorbed silently and the first + #: symptom is a wrong answer from something reading a field the SDK never learned. The intended + #: replacement is a comparison against each analyzer's published schema in ``codeanalyzer-schema`` + #: — reporting what is unmodelled instead of refusing to parse — which is tracked separately. + #: + #: ``ignore`` rather than ``allow`` on purpose: ``allow`` keeps unknown fields in ``model_extra`` + #: and so widens ``model_dump_json()`` with whatever the analyzer emitted, and several tests + #: assert properties *of* dumps (E6's ``"can://" not in ...model_dump_json()``). A field the SDK + #: does not model must not be able to change what a dump contains. + #: + #: A declared field is therefore the only way a value is reachable. Relaxing this does not make + #: the explicit ones redundant — it makes them load-bearing. + model_config = ConfigDict(extra="ignore") # ---------------------------------------------------------------------------------------------- @@ -653,7 +676,9 @@ class JCompilationUnit(_Node): :class:`JImport` records, exposed as :attr:`import_declarations`; the 1.x ``imports`` (a list of paths) is the property of that name.""" - model_config = ConfigDict(extra="forbid", validate_by_name=True, validate_by_alias=True, serialize_by_alias=True) + #: Overrides ``_Base`` entirely, so it needs its own ``ignore`` (#386) — a subclass + #: ``model_config`` replaces rather than merges, and this one is here for the alias settings. + model_config = ConfigDict(extra="ignore", validate_by_name=True, validate_by_alias=True, serialize_by_alias=True) kind: Literal["module"] = "module" span: JSpan diff --git a/cldk/models/typescript/models.py b/cldk/models/typescript/models.py index a078448..4d29064 100644 --- a/cldk/models/typescript/models.py +++ b/cldk/models/typescript/models.py @@ -31,7 +31,8 @@ ``classes``/``interfaces``/… maps over the unified ``types``). What the wire no longer carries (``path``, ``accessed_symbols``, ``local_variables``, ``code_start_line``) is gone, not faked. -``extra="forbid"`` is intentional: drift between the analyzer's JSON and these models fails +``extra="ignore"`` since #386: an additive analyzer release is consumable without an SDK edit, at the +cost of the drift detector — an unmodelled field is dropped silently rather than failing loudly. The fields the next analyzer release (1.3.0) is known to add are already declared ``Optional`` so that pin bump changes no model. """ @@ -45,7 +46,26 @@ class _Base(BaseModel): - model_config = ConfigDict(extra="forbid") + #: ``ignore``, not ``forbid`` (#386). An additive analyzer release used to fail the whole payload + #: rather than the one field it added: codeanalyzer-java 3.1.2's ``var`` on ``param_in``/ + #: ``param_out`` produced 2515 validation errors on daytrader8 until ``JParamEdge`` declared it, + #: for a change that was backward compatible by construction. Uptake should not require an SDK + #: edit before anything parses. + #: + #: What this gives up is the drift detector. ``forbid`` was what surfaced that ``var`` within + #: seconds of the pin bump; under ``ignore`` the same addition is absorbed silently and the first + #: symptom is a wrong answer from something reading a field the SDK never learned. The intended + #: replacement is a comparison against each analyzer's published schema in ``codeanalyzer-schema`` + #: — reporting what is unmodelled instead of refusing to parse — which is tracked separately. + #: + #: ``ignore`` rather than ``allow`` on purpose: ``allow`` keeps unknown fields in ``model_extra`` + #: and so widens ``model_dump_json()`` with whatever the analyzer emitted, and several tests + #: assert properties *of* dumps (E6's ``"can://" not in ...model_dump_json()``). A field the SDK + #: does not model must not be able to change what a dump contains. + #: + #: A declared field is therefore the only way a value is reachable. Relaxing this does not make + #: the explicit ones redundant — it makes them load-bearing. + model_config = ConfigDict(extra="ignore") # ---------------------------------------------------------------------------------------------- @@ -417,7 +437,7 @@ class _Type(_Spanned): is_ambient: bool = False # 1.3.0 additive, on *every* type kind (TS-8): the analyzer stamps the entrypoint tier on # whatever declaration a ruleset matched, so an interface, enum, type alias or namespace can - # carry it too. Declaring it only on TSClass would fail ``extra="forbid"`` on those four the + # carry it too. Declaring it only on TSClass would have failed the old ``extra="forbid"`` on those four the # day the pin moves -- exactly the breakage TS-8 exists to prevent. entrypoints: Optional[List[TSEntrypoint]] = None is_entrypoint: Optional[bool] = None diff --git a/tests/models/java/test_java_models.py b/tests/models/java/test_java_models.py index a116ece..02137a8 100644 --- a/tests/models/java/test_java_models.py +++ b/tests/models/java/test_java_models.py @@ -80,19 +80,48 @@ def test_round_trip_is_byte_equal(fixture_name: str, request): assert _sorted(dumped) == _sorted(json.loads(raw)) -def test_unknown_top_level_key_is_rejected(analysis_json_a4: str): +def test_an_unknown_top_level_key_is_ignored(analysis_json_a4: str): + """#386: the mirrors are ``extra="ignore"``, so an undeclared key is dropped, not rejected. + + ``repository`` is a real field on codeanalyzer-python's application and absent from Java's, so it + stands in for the shape this policy exists to absorb: a sibling analyzer's field arriving in a + later Java release. It used to fail the whole payload; now it parses and the value is gone. + """ raw = json.loads(analysis_json_a4) raw["repository"] = "x" - with pytest.raises(ValidationError): - JAnalysis.model_validate(raw) + + a = JAnalysis.model_validate(raw) + assert not hasattr(a, "repository") + assert "repository" not in a.model_dump() + assert a.model_extra in (None, {}), "extra=ignore must not retain it; extra=allow would" -def test_unknown_nested_key_is_rejected(analysis_json_a4: str): +def test_an_unknown_nested_key_is_ignored(analysis_json_a4: str): + """The same policy one level down, where the old behaviour was most expensive. + + ``file_path`` is a **retired 1.x** key on the compilation unit. Rejecting it meant a stale or + misspelled wire key failed the entire analysis; ignoring it means the key is unreachable and + nothing says so. That is the trade #386 accepted, and the assertion here is what keeps it + explicit rather than folklore. + + ``JCompilationUnit`` is worth naming: it overrides ``model_config`` wholesale for its alias + settings, so ``_Base``'s policy does not reach it and it carries its own ``ignore``. If this test + ever fails while the top-level one passes, that override has drifted back to ``forbid``. + """ raw = json.loads(analysis_json_a4) - unit = next(iter(raw["application"]["symbol_table"].values())) - unit["file_path"] = "x" - with pytest.raises(ValidationError): - JAnalysis.model_validate(raw) + key = next(iter(raw["application"]["symbol_table"])) + raw["application"]["symbol_table"][key]["file_path"] = "x" + + a = JAnalysis.model_validate(raw) + unit = a.application.symbol_table[key] + + # `file_path` is a property over a PrivateAttr that `JApplication` stamps from the symbol-table + # key (models.py:691,714) -- not a wire field -- so the test is not that the attribute vanishes + # but that the injected wire value never reaches it. + assert "file_path" not in type(unit).model_fields, "file_path is not a wire field" + assert unit.file_path == key, "the property still derives from the symbol-table key" + assert unit.file_path != "x", "the ignored wire value must not have taken effect" + assert unit.model_extra in (None, {}), "extra=ignore must not retain it; extra=allow would" def test_v1_shaped_payload_is_rejected(): diff --git a/tests/models/typescript/test_ts_v2_models.py b/tests/models/typescript/test_ts_v2_models.py index 746065d..85d38e7 100644 --- a/tests/models/typescript/test_ts_v2_models.py +++ b/tests/models/typescript/test_ts_v2_models.py @@ -224,11 +224,32 @@ def test_1x_shaped_document_is_rejected(): TSApplication.model_validate(v1) -def test_unknown_field_is_rejected(): +def test_an_unknown_field_is_ignored_and_a_v1_payload_is_still_refused(): + """#386 relaxed the mirrors to ``extra="ignore"``, and this test carries both halves of what that + changed and did not change. + + ``file_path`` is a **retired 1.x** key. It used to make the payload fail; now it is dropped, which + is the cost of the relaxation stated plainly — a stale or misspelled wire key reads as a missing + feature rather than an error. + + What relaxation does **not** touch is required-field validation, and that is the guard actually + doing the version work: the test above feeds a whole v1 payload and it is still refused, because + it *lacks* fields v2 requires rather than carrying extra ones. So "a 1.x analysis.json is refused, + not parsed" survives #386 intact. Asserting that here keeps the two mechanisms from being + confused for each other the next time someone weighs this policy. + """ raw = json.loads((FIXTURES / "a1" / "analysis.json").read_text(encoding="utf-8")) - raw["application"]["symbol_table"][next(iter(raw["application"]["symbol_table"]))]["file_path"] = "x" + key = next(iter(raw["application"]["symbol_table"])) + raw["application"]["symbol_table"][key]["file_path"] = "x" + + a = TSAnalysis.model_validate(raw) + module = a.application.symbol_table[key] + assert not hasattr(module, "file_path") + assert "file_path" not in module.model_dump() + assert module.model_extra in (None, {}) + with pytest.raises(ValidationError): - TSAnalysis.model_validate(raw) + TSAnalysis.model_validate({"schema_version": "2.0.0", "language": "typescript"}) def test_unknown_type_kind_is_rejected(): @@ -240,9 +261,11 @@ def test_unknown_type_kind_is_rejected(): def test_every_type_kind_accepts_the_1_3_0_entrypoint_fields(): - # TS-8: both fields are declared on the shared ``_Type`` base, so a 1.3.0 payload that stamps - # them on any of the five kinds validates under ``extra="forbid"`` before the pin moves -- - # declaring them on ``TSClass`` alone would fail validation on the other four. + # TS-8: both fields are declared on the shared ``_Type`` base, so a payload that stamps them on + # any of the five kinds is read on all five. Under the old ``extra="forbid"`` this was enforced by + # validation failing on the other four; since #386 relaxed the mirrors to ``ignore``, a field + # declared on ``TSClass`` alone would be *dropped* on the rest rather than raising -- so the + # positive assertions below are now what catches it, and they must stay positive for that reason. span = {"start": (1, 1), "end": (2, 1), "bytes": (0, 4)} entrypoint = {"framework": "express", "route": "/x", "http_methods": ["GET"]} for cls, kind in ((TSClass, "class"), (TSInterface, "interface"), (TSEnum, "enum"), (TSTypeAlias, "type_alias"), (TSNamespace, "namespace")): @@ -253,11 +276,16 @@ def test_every_type_kind_accepts_the_1_3_0_entrypoint_fields(): assert cls.model_validate({k: v for k, v in raw.items() if k not in ("is_entrypoint", "entrypoints")}).is_entrypoint is None, kind -def test_the_pinned_generation_parses_with_nothing_widened(): +def test_the_pinned_generation_carries_the_fields_this_leg_declared(): """The 1.3.0 bump moved no model (leg 2.5b, Task 0). - Every fixture is 1.3.0 output and every model is ``extra="forbid"``, so a field 1.3.0 added - that 2.5a had not pre-declared would be a ``ValidationError`` here, not a silent pass. What + **What this test proves changed with #386.** It used to rest on ``extra="forbid"``: every model + rejected extras, so merely parsing a fixture proved no field had appeared that the models had not + pre-declared. Since the mirrors are ``extra="ignore"``, parsing proves nothing of the kind — an + undeclared field is dropped in silence. So the assertions below carry the whole test now, and they + are positive on purpose: each one names a field and demands its value, which is the only remaining + way to notice it went missing. Adding a field to the fixtures without declaring it here will pass + silently, and that is the accepted cost of #386 rather than an oversight in this test. What 1.3.0 added over 1.2.0 in this corpus: ``application.entrypoint_report`` (**required** on the application in 1.3.0's ``schema.ts``, kept optional here because the graph-backed application view carries the report as a JSON string on the anchor instead), ``is_entrypoint`` / @@ -265,7 +293,7 @@ def test_the_pinned_generation_parses_with_nothing_widened(): L4 port lattice wired into the statement DDG. """ for level in (1, 2, 3, 4): - a = _load(level) # extra="forbid": this line is the assertion + a = _load(level) # since #386 this line only parses; the assertions below are the test report = a.application.entrypoint_report assert report is not None, f"a{level} carries no entrypoint_report" assert report.rulesets == ["shipped"] @@ -332,3 +360,52 @@ def test_span_bytes_are_utf8_offsets_and_code_decodes_them(): drifted += module.source[start:end] != expected assert checked, "no non-ASCII module in the fixture; this test proves nothing as written" assert drifted, "every span in the non-ASCII modules starts before the first multi-byte character; the test cannot fail" + + +# ---------------------------------------------------------------------------------------------- +# #386: the mirrors ignore what they do not declare. The projections do not. +# ---------------------------------------------------------------------------------------------- + + +def test_the_schema_mirrors_ignore_unknown_fields_and_drop_them(): + """The models accept a field they do not declare, and — the half worth asserting — **discard** it. + + This is the policy #386 chose over ``extra="forbid"``, and the trade is deliberate: forbidding + made an additive analyzer release fail the whole payload rather than the one field it added + (codeanalyzer-java 3.1.2's ``var`` cost 2515 validation errors on daytrader8 before + ``JParamEdge`` declared it). What it gives up is that a new field no longer announces itself, so + a caller reaching for analyzer data the SDK does not model finds nothing rather than an error. + That is written down here rather than left to be discovered. + + ``ignore`` and not ``allow``: the value must not survive into ``model_extra`` or a dump, because + several tests assert properties *of* dumps (E6's ``"can://" not in ...model_dump_json()``), and a + field the SDK does not model must not be able to change what a dump contains. + """ + from cldk.models.java.models import JParamEdge + from cldk.models.typescript.models import TSParamEdge + + for model in (TSParamEdge, JParamEdge): + assert model.model_config.get("extra") == "ignore", model.__name__ + e = model.model_validate({"src": "a", "dst": "b", "var": "v", "field_from_a_future_release": 1}) + assert e.var == "v", "a declared field is still read" + assert not hasattr(e, "field_from_a_future_release") + assert "field_from_a_future_release" not in e.model_dump() + assert e.model_extra in (None, {}), "extra=ignore must not retain it; extra=allow would" + + +def test_the_sdk_authored_projections_still_forbid_extras(): + """Scope of #386: the mirrors relaxed, the projections did not, and the difference is not arbitrary. + + A projection is constructed by this SDK from graph rows and is never validated from an + ``analysis.json`` — nothing in the codebase calls ``model_validate`` on one. So strictness there + catches *our* typo'd keyword argument, not the analyzer's additions, and relaxing it would give up + a real guard for no uptake benefit. + """ + from pydantic import ValidationError + + from cldk.models.java.projections import JCallableOverview, JClassOverview + + for model in (JCallableOverview, JClassOverview): + assert model.model_config.get("extra") == "forbid", model.__name__ + with pytest.raises(ValidationError): + JClassOverview(qualified_name="a.B", name="B", kind="class", path="a/B.java", start_line=1, end_line=2, nope=1) diff --git a/tests/resources/java/analysis_json/v2/README.md b/tests/resources/java/analysis_json/v2/README.md index 7d3ce4e..235f972 100644 --- a/tests/resources/java/analysis_json/v2/README.md +++ b/tests/resources/java/analysis_json/v2/README.md @@ -71,9 +71,11 @@ dataflow structure (2,358 `ddg`, 2,038 `ssa`, 320 `points-to`, 20 self-loops, 1, `cdg`, 76 `summary`, 247 `call_graph`, 258 `param_in`, 97 `param_out`), as well as the signature table further down (154 / 0 / 45 of 45 for `a1`, 8 / 4 / 4 of 5 for `a4`). -Note that the new fields make a 3.1.0 payload **unparsable by the pre-#369 models**, which are -`extra="forbid"`: the graph contract stays at 2.0.0 and the wire is additive, but the SDK's mirror -had to grow the five fields before it could read one. +Note that the new fields made a 3.1.0 payload **unparsable by the pre-#369 models**, which were +`extra="forbid"` at the time: the graph contract stays at 2.0.0 and the wire is additive, but the +SDK's mirror had to grow the five fields before it could read one. Since #386 the mirrors are +`extra="ignore"`, so an additive release no longer fails to parse -- it is consumable immediately, +and an undeclared field is simply unreachable until someone declares it. ## What the 3.0.3 regeneration moved diff --git a/tests/resources/typescript/analysis_json/v2/README.md b/tests/resources/typescript/analysis_json/v2/README.md index be7492a..f647e79 100644 --- a/tests/resources/typescript/analysis_json/v2/README.md +++ b/tests/resources/typescript/analysis_json/v2/README.md @@ -31,7 +31,8 @@ Measured on `a4`: `param_in` went from 5 of 31 edges carrying `var` to 31 of 31, model needed widening, because `TSParamEdge` had already declared `var` as optional. The sibling analyzers shipped the same fix in lockstep (codeanalyzer-python 1.5.1 / #196, codeanalyzer-java 3.1.2 / #250), and Java's did need a model widening: `JParamEdge` had only `src`/`dst`, so every -param edge failed `extra="forbid"` until the field was added. +param edge failed validation until the field was added — the mirrors were `extra="forbid"` then, and +are `extra="ignore"` since #386, so an addition like this one is now absorbed silently instead. Why it mattered: the schemas had declared the property since the L4 layer landed and the projections wrote nothing, so a consumer predicate on `var` was `null` on every edge crossing a call boundary.