feat: report the schema fields the write contract does not apply - #1223
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
c257f7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e36ea6a2.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://dg-schema-load-extra-field-f.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #1223 +/- ##
====================================================
- Coverage 85.21% 84.00% -1.21%
====================================================
Files 146 147 +1
Lines 15605 13063 -2542
Branches 2631 1932 -699
====================================================
- Hits 13298 10974 -2324
+ Misses 1642 1522 -120
+ Partials 665 567 -98
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ajtmccarty
left a comment
There was a problem hiding this comment.
one idea for a different approach while I review the tests and the other PR. there's also a cubic comment that I'm not really sure about
| console.print(f"[red]Unable to find {schema}") | ||
| raise typer.Exit(1) | ||
|
|
||
| client = initialize_client() |
There was a problem hiding this comment.
nice to see the client removed here
| """ | ||
| model = type(instance) | ||
| fields = model.model_fields | ||
| read_only = _read_only_fields(model) |
There was a problem hiding this comment.
I would think that model.model_fields would include all fields defined on the model including those inherited, in which case passing fields into _read_only_fields would let you skip the __mro__ loop. but I could definitely be missing something
| except PydanticValidationError as exc: | ||
| _collect_validation_errors(exc=exc, errors=errors) | ||
| else: | ||
| _collect_extra_fields(payload=schema, instance=validated, errors=errors, warnings=warnings) |
There was a problem hiding this comment.
this looks like it works, but it is hard to follow and uses some patterns that aren't quite code smells, but are a little suspicious
__mro__access- a lot of
isinstance,getattr, and.get()that are brittle
I have an idea for another approach that might be worth. I haven't tested it and could definitely be wrong about it.
- first deserialize the request into the
Readmodel, capturing any errors. I think this would cover the removed field and typo cases - transfer the
Readinstance to the correspondingWritemodel, capturing anything dropped/rejected as warnings
again, not sure if this would work, but it would be nice to let Pydantic do this work for us
There was a problem hiding this comment.
Fair on all three, and two of them are pushed in c257f7c.
__mro__ is gone — the generator now resolves the parent chain when it emits the table, so each entry is complete per class and the lookup is a plain dict access. The defensive isinstance checks on the raw payload have collapsed into a single contract guard at the top of the walk; the ones that remain dispatch on the validated value's shape rather than second-guessing the input. Doing that surfaced a latent bug I'd otherwise have shipped: _descend_context reads keys off the raw value, so it had to move inside the guard instead of running before it.
I did try the read→write idea. It has a hole worth knowing about: the read root only declares nodes and generics, so with extra="ignore" step 1 silently eats version and the entire extensions block, and step 2 then fails version: Field required on every payload — with extension attributes never validated or loaded. Step 1 also only catches typos if the read models are extra="forbid", and that breaks the read path, since kind is a computed field on read and a forbid-read model rejects the exact body GET /api/schema emits ([('kind',)] ['extra_forbidden']).
read root fields : ['generics', 'nodes']
write root fields: ['extensions', 'generics', 'nodes', 'version']
The write models set extra="ignore", so a field the user may not set never reaches the server. That decided the field has no effect but left the author with no feedback, so a misspelled key produced a schema quietly different from the one they wrote. Classify every extra key instead. A name the contract knows at that location but the user may not set is reported as a warning and still dropped, so a schema read back from Infrahub keeps loading; any other name is an error. The split is driven by a new generated artifact, schema/generated/contract.py, holding the non-settable field names of each write class. Applying it needs to know which model governs each place in the payload, so _collect_extra_fields walks the raw payload alongside the validated write document: the document resolves the model at every location, including which member of a discriminated union an attribute matched. One consequence is that extra fields surface only once the payload is otherwise valid. validate_schema() now returns warnings alongside errors, and client.schema.validate() reaches the same verdict -- raising ValueError rather than a pydantic ValidationError, and returning the verdict when the payload is accepted. infrahubctl validate schema reports both offline; schema load/check report errors locally and leave the warnings to the server response, which already carries them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A finding carried the bare key, so `parameters.id` was reported as `id` against the owning attribute -- claiming a field is read-only that is in fact settable there -- and collided with an `id` reported from another block when consumers group findings by name. Qualify the name with the fields walked since the last kind or element, which re-anchor the identity a finding is reported against. `inherited` on an attribute is unchanged; `parameters.id` and `extensions.id` now say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
758065b to
83b52d2
Compare
The table held each class's own fields and the lookup unioned them across the model's MRO, reaching into the generated hierarchy from the consumer side. Resolve the inheritance in the generator instead, so the emitted table is already complete per class and the lookup is a plain dict access. Fold the paired defensive isinstance checks on the raw payload into one contract guard at the top of the walk, which also covers the context resolution now that it happens there. Every remaining isinstance dispatches on the validated value's shape rather than second-guessing the input. Record on the walk why it pairs the payload with the validated model: neither side alone carries both the dropped keys and the model governing each location. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrahub_sdk/schema/validate.py">
<violation number="1" location="infrahub_sdk/schema/validate.py:168">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The new exact class-name lookup for read-only fields (`READ_ONLY_FIELDS.get(type(instance).__name__, frozenset())`) removes the previous MRO-based safety net, but the generated `contract.py` map is incomplete for concrete parameter subclasses. `AttributeParametersWrite` is listed with `{"id", "state"}`, yet `TextAttributeParametersWrite`, `ListAttributeParametersWrite`, and other subclasses that inherit from it are absent from `READ_ONLY_FIELDS`. Because those subclasses do not declare `id` or `state` themselves, a round-trip payload containing those bookkeeping keys under `parameters` will be classified as hard errors instead of warnings, breaking the documented round-trip guarantee that read-only fields are warned and dropped. Consider restoring the MRO walk or ensuring the generator emits entries for every concrete subclass that can appear in a validated instance.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ) | ||
|
|
||
| fields = type(instance).model_fields | ||
| read_only = READ_ONLY_FIELDS.get(type(instance).__name__, frozenset()) |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
The new exact class-name lookup for read-only fields (READ_ONLY_FIELDS.get(type(instance).__name__, frozenset())) removes the previous MRO-based safety net, but the generated contract.py map is incomplete for concrete parameter subclasses. AttributeParametersWrite is listed with {"id", "state"}, yet TextAttributeParametersWrite, ListAttributeParametersWrite, and other subclasses that inherit from it are absent from READ_ONLY_FIELDS. Because those subclasses do not declare id or state themselves, a round-trip payload containing those bookkeeping keys under parameters will be classified as hard errors instead of warnings, breaking the documented round-trip guarantee that read-only fields are warned and dropped. Consider restoring the MRO walk or ensuring the generator emits entries for every concrete subclass that can appear in a validated instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/schema/validate.py, line 168:
<comment>The new exact class-name lookup for read-only fields (`READ_ONLY_FIELDS.get(type(instance).__name__, frozenset())`) removes the previous MRO-based safety net, but the generated `contract.py` map is incomplete for concrete parameter subclasses. `AttributeParametersWrite` is listed with `{"id", "state"}`, yet `TextAttributeParametersWrite`, `ListAttributeParametersWrite`, and other subclasses that inherit from it are absent from `READ_ONLY_FIELDS`. Because those subclasses do not declare `id` or `state` themselves, a round-trip payload containing those bookkeeping keys under `parameters` will be classified as hard errors instead of warnings, breaking the documented round-trip guarantee that read-only fields are warned and dropped. Consider restoring the MRO walk or ensuring the generator emits entries for every concrete subclass that can appear in a validated instance.</comment>
<file context>
@@ -140,24 +132,40 @@ def _descend_context(
+ )
+
+ fields = type(instance).model_fields
+ read_only = READ_ONLY_FIELDS.get(type(instance).__name__, frozenset())
for key in sorted(set(payload) - set(fields)):
</file context>
| read_only = READ_ONLY_FIELDS.get(type(instance).__name__, frozenset()) | |
| read_only = frozenset() | |
| for klass in type(instance).__mro__: | |
| read_only |= READ_ONLY_FIELDS.get(klass.__name__, frozenset()) |
There was a problem hiding this comment.
False positive — the premise is wrong. TextAttributeParametersWrite and ListAttributeParametersWrite are both in READ_ONLY_FIELDS, and were before this change too. From contract.py at the reviewed commit:
"AttributeParametersWrite": frozenset({"id", "state"}),
"ListAttributeParametersWrite": frozenset({"id", "state"}),
"NumberAttributeParametersWrite": frozenset({"id", "state"}),
"NumberPoolParametersWrite": frozenset({"id", "state"}),
"TextAttributeParametersWrite": frozenset({"id", "state"}),
Each parameters class is mapped to its own internal counterpart when the table is built, so it gets its own entry rather than relying on the base. The behaviour predicted to break does not:
Text valid=True warnings=['parameters.id', 'parameters.state'] errors=0
Number valid=True warnings=['parameters.id', 'parameters.state'] errors=0
List valid=True warnings=['parameters.id', 'parameters.state'] errors=0
NumberPool valid=True warnings=['parameters.id', 'parameters.state'] errors=0
Dropdown valid=True warnings=['parameters.id', 'parameters.state'] errors=0
The general concern behind it is reasonable, though: with a lookup by exact class name there is nothing to catch a generator that forgets a class. That is now covered by a test asserting no generated write class inherits a read-only field its own entry omits — it passes on the current table, and fails with TextAttributeWrite: ['inherited'], NodeSchemaWrite: ['hash', 'kind'] and five more if the generator regresses to emitting own-fields-only.
Not taking the suggested MRO walk: it was removed deliberately in this commit, and resolving the inheritance once in the generator is what makes the lookup a plain dict access.
What changed
The generated write models set
extra="ignore", so a field the user may not set never reaches the server. That settled whether such a field has an effect, but not whether the author hears about it — a misspelled key produced a schema quietly different from the one they wrote.Every extra key in a submitted payload is now classified:
Implementation notes
A generated table, not a third model family.
infrahub_sdk/schema/generated/contract.pyholds the non-settable field names of each write class (17 entries). It is produced by the Infrahub-side generator from two diffs: the read variant of a class against its write variant, and the internal pydantic counterpart of a value model against its generated write model. The second diff matters — the parameters/choice/computed-attribute/extension models are hand-declared in the generator, so they omit theid/statebookkeeping every internal schema model carries, and each computed-attribute variant omits its siblings' fields. Those names all appear in a schema dumped from Infrahub's own models, so treating them as unknown would have broken the round trip.Applying the table needs to know which model governs each place in the payload.
_collect_extra_fieldswalks the raw payload alongside the validated write document: the document resolves the model at every location, including which member of a discriminated union an attribute matched, so raw keys are compared against the fields that location actually accepts. This is why the walk gives exact dotted paths and the owning kind/element for free, and it avoids both a second generated model family and a hand-rolled discriminator resolver.One consequence, documented in the docstring: extra fields surface only once the payload is otherwise valid, because the validated document is what resolves the contract. A payload rejected for another reason names its extra fields on the next run.
API changes
SchemaValidationResultgainswarnings(list[SchemaValidationWarningDetail]) andwarning_messages. Each warning carries the dotted path, the bare field name, and the schema kind / element that set it.client.schema.validate()returns the verdict instead ofNone, and raisesValueErrorrather than a pydanticValidationError.ValidationErrorsubclassesValueError, soexcept ValueErrorcallers are unaffected; a caller catchingValidationErrorspecifically needs updating.validate_schema_content_and_exit()no longer takesclient— it never needed a server.infrahubctl validate schemano longer builds a client, so it works with no server configured. It reports warnings and errors;infrahubctl schema load/schema checkreport errors locally and leave warnings to the server response, which already carries them, to avoid printing each one twice.Two shapes that previously passed and now fail
Both were silently dropped before, so the loaded schema differed from the authored one:
parametersbelonging to a different attributekind— for examplestart_rangeon aNumberattribute, which configured nothing.Testing
tests/unit/test_schema_offline_validation.pyrestructured: read-only cases assert the exact set of warning paths, unknown-field cases assert the exact set of error paths. Covers every nesting level (root, node, generic, attribute, relationship,parameters,choices,computed_attribute,extensions.nodes[*].attributes), the value-model bookkeeping fields, the sibling-variant field, and the reporting-order consequence above.test_repository_app.py/test_task_app.py(rich table width mismatches) fail identically on a clean tree.invoke formatandinvoke lint-code(ruff, ty, mypy) clean.Companion PR
contract.pyis generated byinvoke backend.generatein the Infrahub repo. The Infrahub side, which generates it and consumes the warnings onPOST /api/schema/load, is opsmill/infrahub#10095.Note this branch is based on the commit the Infrahub
release-1.11submodule currently pins, so it sits behindinfrahub-develop; the changes toctl/schema.pyon develop are additive, in a different part of the file.🤖 Generated with Claude Code