refactor(models)!: relax the schema-v2 mirrors from extra="forbid" to extra="ignore" - #387
Open
rahlk wants to merge 1 commit into
Open
refactor(models)!: relax the schema-v2 mirrors from extra="forbid" to extra="ignore"#387rahlk wants to merge 1 commit into
rahlk wants to merge 1 commit into
Conversation
… extra="ignore"
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #385 — target it, not
release/2.0, so the diff shows only this change. GitHub retargetsautomatically when #385 merges.
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
varonparam_in/param_out— backward compatible byconstruction — and
JAnalysisproduced 2515 validation errors on daytrader8 untilJParamEdgedeclared it.
What this gives up
forbidwas the drift detector, and it worked: it surfaced thatvarwithin seconds of the pin bump.Under
ignorethe same addition is absorbed silently, and the first symptom is a wrong answer fromsomething reading a field the SDK never learned about — slow and quiet in place of loud and immediate.
The replacement worth building is a comparison against each analyzer's published schema in
codeanalyzer-schema(which codeanalyzer-python#198 / java#252 / typescript#199 now enable): reportwhat is unmodelled instead of refusing to parse. That decouples parseability from completeness, which
is the actual problem with
forbid. Not in this PR.Scoped narrower than the whole codebase
_Baseinmodels/javaandmodels/typescript— the analyzer mirrors, validated fromanalysis.json— plusJCompilationUnit, which overridesmodel_configwholesale for its alias settings, so_Base's change never reached it (a subclass config replaces rather than merges)forbidJCallableOverview/JClassOverviewinprojections.pyNothing calls
model_validateon the projections — they are constructed by this SDK from graph rowsand never parsed from the wire. Strictness there catches our typo'd keyword argument, not the
analyzer's additions, so relaxing it would surrender a real guard for zero uptake benefit. Say so if
you want it widened; there is a test pinning the distinction either way.
ignore, notallowallowretains unknown fields inmodel_extraand so widensmodel_dump_json()with whatever theanalyzer emitted — and several tests assert properties of dumps (E6's
"can://" not in overview.model_dump_json()). A field the SDK does not model must not be able tochange what a dump contains. Every new test asserts
model_extrais empty for that reason.What this does not cost — now asserted, not assumed
extradoes not govern required fields, so "a 1.xanalysis.jsonis refused, not parsed" survivesintact. A v1 payload still fails, because it lacks what v2 requires rather than carrying extras.
test_v1_shaped_payload_is_rejectedpasses unchanged in both languages, and the new tests say sobeside it, so the two mechanisms are not confused for each other next time this policy is weighed.
The blast radius was found empirically, not by grep
Four tests asserted the old policy and not one contained the word "forbid" — they were named for
the behaviour (
test_unknown_field_is_rejected,test_unknown_top_level_key_is_rejected, …), so onlyrunning the full suite located them. Replaced rather than deleted, because the surviving half is worth
writing down: each now asserts an undeclared field is accepted and dropped, so a caller reaching
for unmodelled analyzer data learns it finds nothing rather than an error.
One replacement corrected me rather than the code. I first asserted
not hasattr(unit, "file_path")and got
assert not True:file_pathis a property over aPrivateAttrthatJApplicationstamps from the symbol-table key (
models.py:691,714), not a wire field. The test now asserts whatmatters — the key is not a model field, the property still resolves from the symbol-table key, and the
injected wire value never took effect. Stronger than the original, which only proved something
raised.
Two comments were actively misleading and are fixed.
# extra="forbid": this line is the assertionannotated a bare
_load(level)that now asserts nothing, and the test it sat in claimed that parsingproved no undeclared field had appeared. That test is renamed and its docstring now states that the
positive assertions carry it, and that a silent pass is the accepted cost of this change rather than
an oversight in the test.
Gate
1599 passed, 370 skipped, no failures — 1597 on #385's branch, +2 net.Closes #386