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.