diff --git a/infrahub_sdk/ctl/schema.py b/infrahub_sdk/ctl/schema.py index 9114d251..144a3158 100644 --- a/infrahub_sdk/ctl/schema.py +++ b/infrahub_sdk/ctl/schema.py @@ -6,19 +6,19 @@ from datetime import datetime, timezone from enum import Enum from pathlib import Path -from typing import TYPE_CHECKING, Any, Literal +from typing import Any, Literal import typer import yaml -from pydantic import ValidationError from rich.console import Console +from rich.markup import escape from rich.table import Table from ..async_typer import AsyncTyper from ..ctl.client import initialize_client from ..ctl.utils import catch_exception, init_logging from ..queries import SCHEMA_HASH_SYNC_STATUS -from ..schema import NodeSchemaAPI, SchemaWarning +from ..schema import NodeSchemaAPI, SchemaWarning, validate_schema from ..yaml import SchemaFile from .parameters import CONFIG_PARAM from .schema_format import ( @@ -30,9 +30,6 @@ ) from .utils import load_yamlfile_from_disk_and_exit -if TYPE_CHECKING: - from .. import InfrahubClient - SchemaContainer = Literal["nodes", "generics", "relationships"] app = AsyncTyper() @@ -53,17 +50,21 @@ def callback() -> None: """Manage the schema in a remote Infrahub instance.""" -def validate_schema_content_and_exit(client: InfrahubClient, schemas: list[SchemaFile]) -> None: +def validate_schema_content_and_exit(schemas: list[SchemaFile]) -> None: + """Report every offline contract violation and exit when at least one schema is invalid. + + Read-only fields are reported by the server on the load/check response, so only errors are + rendered here to avoid warning about the same field twice. + """ has_error: bool = False for schema_file in schemas: - try: - client.schema.validate(data=schema_file.payload) - except ValidationError as exc: - console.print(f"[red]Schema not valid, found '{len(exc.errors())}' error(s) in {schema_file.location}") - has_error = True - for error in exc.errors(): - loc_str = [str(item) for item in error["loc"]] - console.print(f" '{'/'.join(loc_str)}' | {error['msg']} ({error['type']})") + result = validate_schema(schema=schema_file.payload) + if result.valid: + continue + has_error = True + console.print(f"[red]Schema not valid, found '{len(result.errors)}' error(s) in {schema_file.location}") + for error in result.errors: + console.print(f" {escape(error.message)}") if has_error: raise typer.Exit(1) @@ -208,7 +209,7 @@ async def load( schemas_data = load_yamlfile_from_disk_and_exit(paths=schemas, file_type=SchemaFile, console=console) schema_definition = "schema" if len(schemas_data) == 1 else "schemas" client = initialize_client() - validate_schema_content_and_exit(client=client, schemas=schemas_data) + validate_schema_content_and_exit(schemas=schemas_data) start_time = time.time() response = await client.schema.load(schemas=[item.payload for item in schemas_data], branch=branch) @@ -258,7 +259,7 @@ async def check( schemas_data = load_yamlfile_from_disk_and_exit(paths=schemas, file_type=SchemaFile, console=console) client = initialize_client() - validate_schema_content_and_exit(client=client, schemas=schemas_data) + validate_schema_content_and_exit(schemas=schemas_data) success, response = await client.schema.check(schemas=[item.payload for item in schemas_data], branch=branch) @@ -280,9 +281,9 @@ async def check( def _display_schema_warnings(console: Console, warnings: list[SchemaWarning]) -> None: for warning in warnings: - console.print( - f"[yellow] {warning.type.value}: {warning.message} [{', '.join([kind.display for kind in warning.kinds])}]" - ) + # A warning about a top-level key has no kind to attribute it to. + kinds = f" [{', '.join(kind.display for kind in warning.kinds)}]" if warning.kinds else "" + console.print(f"[yellow] {warning.type.value}: {escape(warning.message)}{escape(kinds)}") def _default_export_directory() -> Path: diff --git a/infrahub_sdk/ctl/validate.py b/infrahub_sdk/ctl/validate.py index cf69e2fa..8b03b332 100644 --- a/infrahub_sdk/ctl/validate.py +++ b/infrahub_sdk/ctl/validate.py @@ -5,14 +5,15 @@ import typer import ujson -from pydantic import ValidationError from rich.console import Console +from rich.markup import escape from ..async_typer import AsyncTyper -from ..ctl.client import initialize_client, initialize_client_sync +from ..ctl.client import initialize_client_sync from ..ctl.exceptions import QueryNotFoundError from ..ctl.utils import catch_exception, find_graphql_query, parse_cli_vars from ..exceptions import GraphQLError +from ..schema import validate_schema as validate_schema_offline from ..utils import write_to_file from ..yaml import SchemaFile from .parameters import CONFIG_PARAM @@ -36,16 +37,16 @@ async def validate_schema(schema: Path, _: str = CONFIG_PARAM) -> None: console.print(f"[red]Unable to find {schema}") raise typer.Exit(1) - client = initialize_client() + result = validate_schema_offline(schema=schema_data[0].payload) - try: - client.schema.validate(schema_data[0].payload) - except ValidationError as exc: - console.print(f"[red]Schema not valid, found {len(exc.errors())} error(s)") - for error in exc.errors(): - loc_str = [str(item) for item in error["loc"]] - console.print(f" '{'/'.join(loc_str)}' | {error['msg']} ({error['type']})") - raise typer.Exit(1) from None + for warning in result.warnings: + console.print(f"[yellow]{escape(warning.message)}") + + if not result.valid: + console.print(f"[red]Schema not valid, found {len(result.errors)} error(s)") + for error in result.errors: + console.print(f" {escape(error.message)}") + raise typer.Exit(1) console.print("[green]Schema is valid !!") diff --git a/infrahub_sdk/schema/__init__.py b/infrahub_sdk/schema/__init__.py index 300d26ac..37658da3 100644 --- a/infrahub_sdk/schema/__init__.py +++ b/infrahub_sdk/schema/__init__.py @@ -44,7 +44,12 @@ SchemaRootAPI, TemplateSchemaAPI, ) -from .validate import SchemaValidationErrorDetail, SchemaValidationResult, validate_schema +from .validate import ( + SchemaValidationErrorDetail, + SchemaValidationResult, + SchemaValidationWarningDetail, + validate_schema, +) if TYPE_CHECKING: from ..client import InfrahubClient, InfrahubClientSync, SchemaType, SchemaTypeSync @@ -74,6 +79,7 @@ "SchemaRootAPI", "SchemaValidationErrorDetail", "SchemaValidationResult", + "SchemaValidationWarningDetail", "TemplateSchemaAPI", "schema_to_export_dict", "validate_schema", @@ -173,10 +179,19 @@ def _build_export_schemas( ns_map[ns].nodes.append(schema_dict) return SchemaExport(namespaces=ns_map) - def validate(self, data: dict[str, Any]) -> None: - # Validate against the generated write contract so this matches what /api/schema/load - # enforces (unknown keys rejected, attribute kinds discriminated, extensions understood). - InfrahubSchemaWrite.model_validate(data) + def validate(self, data: dict[str, Any]) -> SchemaValidationResult: + """Validate a schema payload against the generated write contract. + + Returns: + The verdict, carrying a warning for every read-only field the payload sets. + + Raises: + ValueError: When the payload is invalid, joining every field-level message. + + """ + # Delegating to the offline validator keeps this verdict identical to the one + # /api/schema/load reaches, since the server runs the same models. + return validate_schema(schema=data, raise_on_error=True) def validate_data_against_schema(self, schema: MainSchemaTypesAPI, data: dict) -> None: for key in data: diff --git a/infrahub_sdk/schema/generated/__init__.py b/infrahub_sdk/schema/generated/__init__.py index f7b3eb1d..3e09a11b 100644 --- a/infrahub_sdk/schema/generated/__init__.py +++ b/infrahub_sdk/schema/generated/__init__.py @@ -1,4 +1,4 @@ # Generated by "invoke backend.generate", do not edit directly -from . import enums, read, write +from . import contract, enums, read, write -__all__ = ["enums", "read", "write"] +__all__ = ["contract", "enums", "read", "write"] diff --git a/infrahub_sdk/schema/generated/contract.py b/infrahub_sdk/schema/generated/contract.py new file mode 100644 index 00000000..c2c5754f --- /dev/null +++ b/infrahub_sdk/schema/generated/contract.py @@ -0,0 +1,35 @@ +# Generated by "invoke backend.generate", do not edit directly +"""Read-only fields of the write contract, keyed by generated write class name. + +A field listed here is one the contract knows at that location but the user may not set: +a field the read API returns, the bookkeeping a schema dumped from the internal models +carries, or a field belonging to a sibling variant of a discriminated union. Submitting one +is reported as a warning and the value is dropped, where an extra field that is not listed +is an error. Each entry already includes what the class inherits, so a lookup is by class +name alone. +""" + +READ_ONLY_FIELDS: dict[str, frozenset[str]] = { + "AttributeParametersWrite": frozenset({"id", "state"}), + "AttributeSchemaBaseWrite": frozenset({"inherited"}), + "BaseNodeSchemaWrite": frozenset({"hash", "kind"}), + "ComputedAttributeJinja2Write": frozenset({"id", "state", "transform"}), + "ComputedAttributeTransformPythonWrite": frozenset({"id", "jinja2_template", "state"}), + "ComputedAttributeUserWrite": frozenset({"id", "jinja2_template", "state", "transform"}), + "DropdownChoiceWrite": frozenset({"id", "state"}), + "GenericAttributeWrite": frozenset({"inherited"}), + "GenericSchemaWrite": frozenset({"hash", "kind", "used_by"}), + "InfrahubSchemaWrite": frozenset({"main", "namespaces", "profiles", "templates"}), + "ListAttributeParametersWrite": frozenset({"id", "state"}), + "ListAttributeWrite": frozenset({"inherited"}), + "NodeExtensionWrite": frozenset({"id", "state"}), + "NodeSchemaWrite": frozenset({"hash", "hierarchy", "kind"}), + "NumberAttributeParametersWrite": frozenset({"id", "state"}), + "NumberAttributeWrite": frozenset({"inherited"}), + "NumberPoolAttributeWrite": frozenset({"inherited"}), + "NumberPoolParametersWrite": frozenset({"id", "state"}), + "RelationshipSchemaWrite": frozenset({"hierarchical", "inherited"}), + "SchemaExtensionWrite": frozenset({"id", "state"}), + "TextAttributeParametersWrite": frozenset({"id", "state"}), + "TextAttributeWrite": frozenset({"inherited"}), +} diff --git a/infrahub_sdk/schema/validate.py b/infrahub_sdk/schema/validate.py index 9bb8ca8b..8cae66cb 100644 --- a/infrahub_sdk/schema/validate.py +++ b/infrahub_sdk/schema/validate.py @@ -3,9 +3,12 @@ This module depends only on pydantic and the generated write models, so a caller can validate a schema payload with just the SDK installed (no server, no backend). The write models omit fields the user may not set (read-level, internal) and set -``extra="ignore"``, so a non-settable or unknown field is dropped silently rather than -rejected; constrained fields set outside their allowed set are still rejected naming -the field and the invalid value, as are missing required fields and unknown enum members. +``extra="ignore"``, so those values never reach the server. Whether an omitted field is +reported depends on what it is: a read-only field -- one the read API returns -- is reported +as a warning so a payload read back from Infrahub still loads, while any other extra field is +an error, because the only ways to get one are a typo and a field that no longer exists. +Constrained fields set outside their allowed set are also rejected naming the field and the +invalid value, as are missing required fields and unknown enum members. """ from __future__ import annotations @@ -15,8 +18,15 @@ from pydantic import BaseModel, Field from pydantic import ValidationError as PydanticValidationError +from .generated.contract import READ_ONLY_FIELDS from .generated.write import InfrahubSchemaWrite +# Payload containers whose items carry the identity used to report a finding. The warning shape +# consumers render is named (kind, field) rather than positional, so the walk tracks the owning +# kind and element alongside the dotted path. +_KIND_CONTAINERS = frozenset({"nodes", "generics"}) +_ELEMENT_CONTAINERS = frozenset({"attributes", "relationships"}) + class SchemaValidationErrorDetail(BaseModel): """A single field-level validation problem in a schema payload.""" @@ -25,6 +35,21 @@ class SchemaValidationErrorDetail(BaseModel): message: str = Field(..., description="Human-readable, field-level error message") +class SchemaValidationWarningDetail(BaseModel): + """A read-only field set in a schema payload: accepted, but the submitted value is dropped.""" + + field: str = Field(..., description="Dotted path to the offending field, e.g. 'nodes[0].attributes[1].inherited'") + name: str = Field( + ..., + description="Field path relative to the owning kind or element, e.g. 'inherited' or 'parameters.id'", + ) + kind: str | None = Field(default=None, description="Kind of the schema node carrying the field, when resolvable") + element: str | None = Field( + default=None, description="Name of the attribute or relationship carrying the field, when applicable" + ) + message: str = Field(..., description="Human-readable, field-level warning message") + + class SchemaValidationResult(BaseModel): """The verdict of validating a schema payload against the write contract.""" @@ -32,11 +57,18 @@ class SchemaValidationResult(BaseModel): errors: list[SchemaValidationErrorDetail] = Field( default_factory=list, description="One entry per field-level problem; empty when valid" ) + warnings: list[SchemaValidationWarningDetail] = Field( + default_factory=list, description="One entry per read-only field set in the payload" + ) @property def messages(self) -> list[str]: return [error.message for error in self.errors] + @property + def warning_messages(self) -> list[str]: + return [warning.message for warning in self.warnings] + def raise_for_status(self) -> None: """Raise when the payload is invalid, joining every field-level message. @@ -79,6 +111,117 @@ def _collect_validation_errors( errors.append(SchemaValidationErrorDetail(field=location, message=message)) +def _descend_context( + field: str, item: dict[str, Any], kind: str | None, element: str | None, qualifier: tuple[str, ...] +) -> tuple[str | None, str | None, tuple[str, ...]]: + """Resolve the owning kind, element and field qualifier for a value nested under a field. + + Entering a kind or element container re-anchors the identity a finding is reported against, so + the qualifier resets there. Anywhere else the field name joins the qualifier, which is what + distinguishes a nested ``parameters.id`` from an ``id`` set directly on the attribute. + """ + if field in _KIND_CONTAINERS: + namespace, name = item.get("namespace"), item.get("name") + # An extension addresses an existing node by kind; a new node is namespace + name. + resolved = f"{namespace}{name}" if namespace and name else item.get("kind") + return (resolved if isinstance(resolved, str) else None), None, () + if field in _ELEMENT_CONTAINERS: + name = item.get("name") + return kind, (name if isinstance(name, str) else None), () + return kind, element, (*qualifier, field) + + +def _collect_extra_fields( + payload: Any, + instance: BaseModel, + errors: list[SchemaValidationErrorDetail], + warnings: list[SchemaValidationWarningDetail], + path: str = "", + field: str | None = None, + kind: str | None = None, + element: str | None = None, + qualifier: tuple[str, ...] = (), +) -> None: + """Report every payload key the write contract does not declare, walking the validated model. + + The submitted payload is walked alongside the model validated from it, because neither on its + own carries what a finding needs. ``extra="ignore"`` means the validated instance no longer + knows which keys were dropped, so the raw payload has to supply them; and only the instance + resolves which model governs a given location -- notably which member of a discriminated union + an attribute matched -- so only it can say which keys that location accepts. Pairing them also + yields the owning kind and element, which a finding is reported against by name rather than by + position. + + ``field`` is the name this payload was reached through, and is None only at the root. + """ + if not isinstance(payload, dict): + # A caller may nest an already-built model rather than plain data; a model cannot carry an + # undeclared key, so there is nothing to compare and nothing below worth walking. + return + + if field is not None: + kind, element, qualifier = _descend_context( + field=field, item=payload, kind=kind, element=element, qualifier=qualifier + ) + + fields = type(instance).model_fields + read_only = READ_ONLY_FIELDS.get(type(instance).__name__, frozenset()) + + for key in sorted(set(payload) - set(fields)): + location = f"{path}.{key}" if path else key + if key in read_only: + warnings.append( + SchemaValidationWarningDetail( + field=location, + name=".".join((*qualifier, key)), + kind=kind, + element=element, + message=f"{location}: Read-only field, the submitted value is ignored (received: {payload[key]!r})", + ) + ) + else: + errors.append( + SchemaValidationErrorDetail( + field=location, + message=f"{location}: Unknown field, it is not part of the schema (received: {payload[key]!r})", + ) + ) + + for name in fields: + if name not in payload: + continue + raw, value = payload[name], getattr(instance, name) + child_path = f"{path}.{name}" if path else name + # Validation succeeded, so a list field is index-aligned with the list it was built from. + # A list of plain values carries no nested model and is skipped. + if isinstance(value, list): + for index, (raw_item, item) in enumerate(zip(raw, value, strict=True)): + if isinstance(item, BaseModel): + _collect_extra_fields( + payload=raw_item, + instance=item, + errors=errors, + warnings=warnings, + path=f"{child_path}[{index}]", + field=name, + kind=kind, + element=element, + qualifier=qualifier, + ) + elif isinstance(value, BaseModel): + _collect_extra_fields( + payload=raw, + instance=value, + errors=errors, + warnings=warnings, + path=child_path, + field=name, + kind=kind, + element=element, + qualifier=qualifier, + ) + + def validate_schema(schema: dict[str, Any], *, raise_on_error: bool = False) -> SchemaValidationResult: """Validate a single schema-root payload against the generated write contract. @@ -87,23 +230,31 @@ def validate_schema(schema: dict[str, Any], *, raise_on_error: bool = False) -> raise_on_error: When True, raise ``ValueError`` instead of returning an invalid result. Returns: - A :class:`SchemaValidationResult` with a field-level message for every field that is not - settable (read-level, internal, or unknown) and for every constrained field set outside - its allowed set. The whole root -- nodes, generics and the attributes/relationships nested - under ``extensions.nodes`` -- is validated against the write document model in one pass. + A :class:`SchemaValidationResult` with a field-level message for every constrained field set + outside its allowed set, every missing required field, and every extra field the contract + does not declare, plus a warning for every read-only field the payload sets. The whole root + -- nodes, generics and the attributes/relationships nested under ``extensions.nodes`` -- is + validated against the write document model in one pass. + + Extra fields are reported only once the payload validates against the write models, since + the validated instance is what resolves the contract applying at each location. A payload + rejected for another reason therefore reports that reason first. Raises: ValueError: When ``raise_on_error`` is True and the payload is invalid. """ errors: list[SchemaValidationErrorDetail] = [] + warnings: list[SchemaValidationWarningDetail] = [] try: - InfrahubSchemaWrite.model_validate(schema) + validated = InfrahubSchemaWrite.model_validate(schema) except PydanticValidationError as exc: _collect_validation_errors(exc=exc, errors=errors) + else: + _collect_extra_fields(payload=schema, instance=validated, errors=errors, warnings=warnings) - result = SchemaValidationResult(valid=not errors, errors=errors) + result = SchemaValidationResult(valid=not errors, errors=errors, warnings=warnings) if raise_on_error: result.raise_for_status() return result diff --git a/tests/unit/ctl/test_schema_app.py b/tests/unit/ctl/test_schema_app.py index 8663b319..bbd498b5 100644 --- a/tests/unit/ctl/test_schema_app.py +++ b/tests/unit/ctl/test_schema_app.py @@ -96,8 +96,9 @@ def test_schema_load_notvalid_namespace() -> None: clean_output = remove_ansi_color(result.stdout.replace("\n", "")) assert "Schema not valid" in clean_output - assert "nodes/0/namespace" in clean_output - assert "string_pattern_mismatch" in clean_output + assert "nodes[0].namespace" in clean_output + assert "String should match pattern" in clean_output + assert "received: 'OuT'" in clean_output def test_load_valid_generic_schema(httpx_mock: HTTPXMock) -> None: diff --git a/tests/unit/test_schema_offline_validation.py b/tests/unit/test_schema_offline_validation.py index 977e21e1..d9353672 100644 --- a/tests/unit/test_schema_offline_validation.py +++ b/tests/unit/test_schema_offline_validation.py @@ -1,10 +1,10 @@ """Offline schema validation: with only the SDK installed (pydantic, no server). Validates a schema payload against the generated write models and asserts the -field-level verdict, without importing the backend/server package. The write models -set ``extra="ignore"``, so non-settable (read-level, internal) and unknown fields are -dropped silently rather than rejected; enum, constraint and required-field violations -are still reported naming the field and the invalid value. +field-level verdict, without importing the backend/server package. Values the user may not +set never reach the server, but they are reported: a read-only field -- one the read API +returns -- as a warning, and any other extra field as an error. Enum, constraint and +required-field violations are reported naming the field and the invalid value. """ from __future__ import annotations @@ -157,91 +157,250 @@ def test_enum_backed_relationship_cardinality_valid_value_passes() -> None: # --------------------------------------------------------------------------- -# Non-write fields are tolerated and dropped, not rejected +# Read-only fields are accepted with a warning # --------------------------------------------------------------------------- @dataclass -class ToleratedCase: +class ReadOnlyCase: name: str schema: dict + # Exact dotted paths expected among the reported warnings. + expected_fields: set[str] -TOLERATED_CASES = [ - # Read-level / internal fields the user may not set: dropped silently on validation. - ToleratedCase(name="attribute-read-level-inherited", schema=_schema_with_attribute_fields(inherited=True)), - ToleratedCase( - name="relationship-read-level", +READ_ONLY_CASES = [ + ReadOnlyCase( + name="attribute-inherited", + schema=_schema_with_attribute_fields(inherited=True), + expected_fields={"nodes[0].attributes[0].inherited"}, + ), + ReadOnlyCase( + name="relationship-inherited-and-hierarchical", schema=_schema_with_relationship_fields(inherited=True, hierarchical="SomeGeneric"), + expected_fields={ + "nodes[0].relationships[0].inherited", + "nodes[0].relationships[0].hierarchical", + }, + ), + ReadOnlyCase( + name="generic-used-by", + schema=_schema_with_generic_fields(used_by=["InfraThing"]), + expected_fields={"generics[0].used_by"}, + ), + ReadOnlyCase( + name="node-hierarchy", + schema=_schema_with_node_fields(hierarchy="SomeGeneric"), + expected_fields={"nodes[0].hierarchy"}, ), - ToleratedCase(name="generic-read-level-used-by", schema=_schema_with_generic_fields(used_by=["InfraThing"])), - ToleratedCase(name="node-read-level-hierarchy", schema=_schema_with_node_fields(hierarchy="SomeGeneric")), - ToleratedCase( - name="extension-attribute-read-level-inherited", + ReadOnlyCase( + name="node-derived-kind-and-hash", + schema=_schema_with_node_fields(kind="InfraDevice", hash="abc123"), + expected_fields={"nodes[0].kind", "nodes[0].hash"}, + ), + ReadOnlyCase( + name="extension-attribute-inherited", schema=_extension_node_schema( {"kind": "InfraDevice", "attributes": [{"name": "extra", "kind": "Text", "inherited": True}]} ), + expected_fields={"extensions.nodes[0].attributes[0].inherited"}, + ), + ReadOnlyCase( + name="root-keys-of-a-read-api-response", + schema=_schema_with_root_fields(main="abc123", profiles=[], templates=[], namespaces=[]), + expected_fields={"main", "profiles", "templates", "namespaces"}, ), - # Genuinely unknown fields (typos, removed fields): also dropped silently. - ToleratedCase(name="node-unknown-field", schema=_schema_with_node_fields(not_a_field="boom")), - ToleratedCase(name="unknown-top-level-key", schema=_schema_with_root_fields(not_a_root_field="boom")), - ToleratedCase( + # Every internal schema model carries `id` and `state`, so they appear on the nested value + # models of a schema dumped from those models even though they are not settable there. + ReadOnlyCase( + name="parameters-bookkeeping-fields", + schema=_schema_with_parameters({"min_length": 1, "id": None, "state": "present"}), + expected_fields={ + "nodes[0].attributes[0].parameters.id", + "nodes[0].attributes[0].parameters.state", + }, + ), + ReadOnlyCase( + name="choice-bookkeeping-fields", + schema=_schema_with_choices([{"name": "active", "id": None, "state": "present"}]), + expected_fields={ + "nodes[0].attributes[0].choices[0].id", + "nodes[0].attributes[0].choices[0].state", + }, + ), + # `transform` belongs to the TransformPython variant of the computed-attribute union, so it is + # known at this location but not settable on a Jinja2 one. + ReadOnlyCase( + name="computed-attribute-sibling-variant-field", + schema=_schema_with_computed_attribute({"kind": "Jinja2", "jinja2_template": "x", "transform": "t"}), + expected_fields={"nodes[0].attributes[0].computed_attribute.transform"}, + ), +] + + +@pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in READ_ONLY_CASES]) +def test_read_only_field_is_accepted_with_a_warning(case: ReadOnlyCase) -> None: + # A payload read back from Infrahub carries read-only fields, so it must still load; the user + # is told the value is ignored rather than having it dropped silently. + result = validate_schema(schema=case.schema) + + assert result.valid is True, result.messages + assert {warning.field for warning in result.warnings} == case.expected_fields + + +def test_read_only_warning_names_the_owning_kind_and_element() -> None: + # Consumers render a warning as kind + field rather than as a path, so the owning schema kind + # and the attribute/relationship carrying the field travel with the finding. + schema = _schema_with_attribute_fields(inherited=True) + + result = validate_schema(schema=schema) + + assert len(result.warnings) == 1 + warning = result.warnings[0] + assert warning.name == "inherited" + assert warning.kind == "InfraDevice" + assert warning.element == "hostname" + + +def test_nested_read_only_field_is_named_relative_to_its_owner() -> None: + # `id` is settable on an attribute but not on its nested parameters, so reporting the bare name + # would claim the wrong field is read-only -- and would collide with an `id` reported elsewhere. + schema = _schema_with_parameters({"id": None, "state": "present"}) + schema["extensions"] = {"nodes": [{"kind": "InfraDevice"}], "id": None} + + result = validate_schema(schema=schema) + + assert result.valid is True, result.messages + assert {warning.name for warning in result.warnings} == { + "parameters.id", + "parameters.state", + "extensions.id", + } + + +def test_read_only_fields_are_dropped_on_round_trip() -> None: + # A warning must not mean the value is kept: read-only fields are absent from the validated + # model, so they never reach the server. + schema = _valid_schema() + schema["nodes"][0]["hierarchy"] = "SomeGeneric" + schema["nodes"][0]["attributes"][0]["inherited"] = True + + assert validate_schema(schema=schema).valid is True + + dumped = InfrahubSchemaWrite.model_validate(schema).model_dump() + node = dumped["nodes"][0] + assert "hierarchy" not in node + assert "inherited" not in node["attributes"][0] + + +# --------------------------------------------------------------------------- +# Any other extra field is rejected +# --------------------------------------------------------------------------- + + +@dataclass +class UnknownFieldCase: + name: str + schema: dict + # Exact dotted paths expected among the reported error fields. + expected_fields: set[str] + + +UNKNOWN_FIELD_CASES = [ + UnknownFieldCase( + name="node-unknown-field", + schema=_schema_with_node_fields(not_a_field="boom"), + expected_fields={"nodes[0].not_a_field"}, + ), + UnknownFieldCase( + name="unknown-top-level-key", + schema=_schema_with_root_fields(not_a_root_field="boom"), + expected_fields={"not_a_root_field"}, + ), + UnknownFieldCase( name="extension-attribute-unknown-field", schema=_extension_node_schema( {"kind": "InfraDevice", "attributes": [{"name": "extra", "kind": "Text", "not_a_field": "boom"}]} ), + expected_fields={"extensions.nodes[0].attributes[0].not_a_field"}, ), - ToleratedCase( + UnknownFieldCase( name="computed-attribute-unknown-field", schema=_schema_with_computed_attribute({"kind": "Jinja2", "jinja2_template": "x", "not_a_real_field": "x"}), + expected_fields={"nodes[0].attributes[0].computed_attribute.not_a_real_field"}, ), - ToleratedCase( + UnknownFieldCase( name="choice-unknown-field", schema=_schema_with_choices([{"name": "active", "not_a_real_field": "x"}]), + expected_fields={"nodes[0].attributes[0].choices[0].not_a_real_field"}, ), - ToleratedCase(name="parameters-unknown-field", schema=_schema_with_parameters({"not_a_real_param": 1})), - # Parameters valid only for a different attribute kind: dropped, not rejected. - ToleratedCase( + UnknownFieldCase( + name="parameters-unknown-field", + schema=_schema_with_parameters({"not_a_real_param": 1}), + expected_fields={"nodes[0].attributes[0].parameters.not_a_real_param"}, + ), + # Parameters only valid for a different attribute kind do nothing on this one, so naming them + # is the only way the author learns the setting had no effect. + UnknownFieldCase( name="number-attribute-number-pool-parameters", schema=_schema_with_kind_and_parameters("Number", {"start_range": 1, "end_range": 9}), + expected_fields={ + "nodes[0].attributes[0].parameters.start_range", + "nodes[0].attributes[0].parameters.end_range", + }, ), - ToleratedCase( + UnknownFieldCase( name="text-attribute-number-parameters", schema=_schema_with_kind_and_parameters("Text", {"min_value": 1}), + expected_fields={"nodes[0].attributes[0].parameters.min_value"}, ), - ToleratedCase( + UnknownFieldCase( name="generic-attribute-any-parameters", schema=_schema_with_kind_and_parameters("Dropdown", {"regex": "x"}), + expected_fields={"nodes[0].attributes[0].parameters.regex"}, ), ] -@pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in TOLERATED_CASES]) -def test_non_write_field_is_tolerated(case: ToleratedCase) -> None: - # extra="ignore" on the write models drops the field silently, so validation passes. +@pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in UNKNOWN_FIELD_CASES]) +def test_unknown_field_is_rejected_naming_the_field(case: UnknownFieldCase) -> None: result = validate_schema(schema=case.schema) - assert result.valid is True, result.messages + assert result.valid is False + assert _fields_named(result) == case.expected_fields + assert result.warnings == [] -def test_non_write_fields_are_dropped_on_round_trip() -> None: - # Tolerated fields must not round-trip into the payload: read-level and unknown fields are - # absent from the validated model, so they never reach the server. +def test_unknown_fields_are_reported_at_every_nesting_level_at_once() -> None: + # One pass must name every offending key rather than stopping at the first, so a payload is + # corrected in a single round. schema = _valid_schema() schema["not_a_root_field"] = "boom" - schema["nodes"][0]["hierarchy"] = "SomeGeneric" - schema["nodes"][0]["attributes"][0]["inherited"] = True - schema["nodes"][0]["attributes"][0]["not_a_field"] = "boom" + schema["nodes"][0]["not_a_node_field"] = "boom" + schema["nodes"][0]["attributes"][0]["not_an_attribute_field"] = "boom" + schema["nodes"][0]["relationships"][0]["not_a_relationship_field"] = "boom" - assert validate_schema(schema=schema).valid is True + result = validate_schema(schema=schema) - dumped = InfrahubSchemaWrite.model_validate(schema).model_dump() - assert "not_a_root_field" not in dumped - node = dumped["nodes"][0] - assert "hierarchy" not in node - attribute = node["attributes"][0] - assert "inherited" not in attribute - assert "not_a_field" not in attribute + assert result.valid is False + assert _fields_named(result) == { + "not_a_root_field", + "nodes[0].not_a_node_field", + "nodes[0].attributes[0].not_an_attribute_field", + "nodes[0].relationships[0].not_a_relationship_field", + } + + +def test_unknown_fields_are_not_reported_while_the_payload_is_otherwise_invalid() -> None: + # The validated model is what resolves the contract at each location, so a payload that fails + # validation reports that failure first and the extra fields once it is corrected. + schema = _schema_with_node_fields(not_a_field="boom") + schema["nodes"][0]["attributes"][0]["kind"] = "NotARealKind" + + result = validate_schema(schema=schema) + + assert result.valid is False + assert _fields_named(result) == {"nodes[0].attributes[0]"} # ---------------------------------------------------------------------------