From 9dce930afc182a2e91da838c3befb5d6fa1e8432 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Sun, 11 Oct 2026 15:15:00 +0530 Subject: [PATCH] feat(python): add document loaders for the rules the field types cannot express No property in `ossie-schema.json` is nullable, but every optional field accepts an explicit null, and `exclude_none=True` then drops the key: a null the schema forbids becomes an omission it allows, so the mistake is never reported. `{"is_time": null}` is rejected by the schema, accepted by the model, and serialized as `{}`, which the schema accepts. A non-finite number launders the same way, and `version` goes the other direction: the schema requires it, the field carries a default, so a document without one gains one it never declared. `model_validate` cannot be the place to fix any of it. Building a model in Python routinely passes a value that may be None and means "omit it" -- `label=dim.label` in the dbt converter, `description=data.get(...)` in Sigma -- and 143 call sites across the converters do this, 119 at field level and 24 at document level, so rejecting a null there would break callers rather than improve schema conformance. The `version` default is wanted there for the same reason. A document is different: it is the artefact the schema governs, and `to_ossie_yaml` and `to_ossie_json` had no input counterpart. `from_ossie_yaml` and `from_ossie_json` parse one and reject, naming the path to each: - a property set to null; - a non-finite number, since `yaml.safe_load` reads `.nan` and `.inf` and `json.loads` accepts the non-standard `NaN`, `Infinity` and `-Infinity`, none of which JSON expresses -- a NaN reaching `to_ossie_json` comes back out as `null`; - a missing `version`; - a cyclic alias, which `yaml.safe_load` resolves into a container holding itself. The field types cover the rest of the schema. The free-form content of `ai_context` is not audited: its object form sets `additionalProperties: true`, so a value outside JSON's own data model -- a YAML set or date, a non-string mapping key -- is passed through and serialized as whatever it maps to, which is what `model_validate` does today. A test pins that, so widening the contract is a visible change rather than an accident. Only the containers on the current branch count as a cycle, so an anchor reused on a sibling branch still walks. A container reached again through another alias is not walked or reported twice: an anchor is one property in the source, so one path locates it, and a chain of anchors shaped `[*previous, *previous]` expands to 2^n copies of the anchored subtree, so a single null inside it would otherwise yield 2^n paths. A long list of paths is truncated with a count. Tested by setting every optional property in one document and nulling each of its 61 paths in turn, requiring every one to be rejected and comparing the reported paths as exact entries, since `description` is also a substring of `datasets[0].description`. The sample is kept complete by walking the models from `OssieDocument` and checking each one's properties against the mapping at that position, so adding a field fails here even where its name is already used on another model. A null array item is rejected by the field type rather than by the walk: unlike an optional property, it is not a value the model would accept and then drop. --- python/src/ossie/models.py | 133 ++++++++ python/tests/test_serialization.py | 481 +++++++++++++++++++++++++++++ 2 files changed, 614 insertions(+) diff --git a/python/src/ossie/models.py b/python/src/ossie/models.py index 3048bdc6..ffac09bf 100644 --- a/python/src/ossie/models.py +++ b/python/src/ossie/models.py @@ -15,6 +15,8 @@ # specific language governing permissions and limitations # under the License. +import json +import math from enum import Enum from typing import Any, Literal, Optional, Union @@ -22,6 +24,86 @@ from pydantic import BaseModel, ConfigDict, Field, SerializerFunctionWrapHandler, StrictBool, model_serializer +_AI_CONTEXT_FIELDS = frozenset({"instructions", "synonyms", "examples"}) + +#: Paths listed in a diagnostic before the rest are counted instead. +_MAX_REPORTED = 20 + +_NULL = "set to null, which the schema does not allow; omit it instead" +_NON_FINITE = "not a number JSON can represent, so it cannot round-trip" + + +def _cyclic(path: str) -> ValueError: + return ValueError( + f"document contains a cyclic alias at {path or 'the root'}, " + f"which cannot be represented as an Ossie document" + ) + + +def _invalid_below( + node: Any, + path: str, + active: tuple[int, ...], + walked: set[tuple[int, bool, bool]], + free_form: bool, + ai_context: bool, +) -> list[tuple[str, str]]: + """Paths below `node` holding a value an Ossie document cannot carry. + + A null is one, because no property in `ossie-schema.json` is nullable. So is a + non-finite number: `yaml.safe_load` reads `.nan` and `.inf`, and `json.loads` + accepts the non-standard `NaN`, `Infinity` and `-Infinity`, none of which JSON can + express -- a `NaN` reaching `to_ossie_json` comes back out as `null`. + + `free_form` marks the parts of `ai_context` where a null is legal: its object form + sets `additionalProperties: true`, so an extra key may hold any value. A non-finite + number is rejected there too, since it still cannot be serialized. + + `active` holds the containers on the current branch, so a self-reference raises + while an anchor reused on a sibling branch still walks. `walked` holds containers + already covered: one reached again through another alias is not reported twice, + because an anchor is one property in the source, and because a chain of anchors + shaped `[*previous, *previous]` expands to 2^n copies of the anchored subtree. + """ + if isinstance(node, float) and not math.isfinite(node): + return [(path, _NON_FINITE)] + if not isinstance(node, (dict, list)): + return [] + if id(node) in active: + raise _cyclic(path) + seen = (id(node), free_form, ai_context) + if seen in walked: + return [] + walked.add(seen) + inner = (*active, id(node)) + found: list[tuple[str, str]] = [] + if isinstance(node, dict): + for key, value in node.items(): + where = f"{path}.{key}" if path else str(key) + legal_null = free_form or (ai_context and key not in _AI_CONTEXT_FIELDS) + if value is None: + if not legal_null: + found.append((where, _NULL)) + else: + found += _invalid_below( + value, where, inner, walked, legal_null, key == "ai_context" + ) + else: + for index, item in enumerate(node): + found += _invalid_below( + item, f"{path}[{index}]", inner, walked, free_form, False + ) + return found + + +def _invalid_properties(node: Any) -> list[tuple[str, str]]: + """Return every property holding a value an Ossie document cannot carry. + + Raises `ValueError` if the document holds a cyclic alias. + """ + return _invalid_below(node, "", (), set(), False, False) + + class OssieDialect(str, Enum): """Supported SQL and expression language dialects.""" @@ -251,3 +333,54 @@ def to_ossie_json(self, **kwargs: Any) -> str: kwargs.setdefault("by_alias", True) kwargs.setdefault("exclude_none", True) return self.model_dump_json(**kwargs) + + @classmethod + def from_ossie_yaml(cls, text: str) -> "OssieDocument": + """Parse an Ossie YAML document, checking what pydantic alone cannot. + + The counterpart of :meth:`to_ossie_yaml`. ``model_validate`` stays lenient, + because a caller building a model in Python routinely passes a value that may + be ``None`` -- ``label=dim.label`` -- and means "omit it". A document is + different: it is the artefact the schema governs, so these rules apply to it + and not to the models. + + Rejected here: a property set to null, a missing ``version``, a non-finite + number, and a cyclic alias. The field types reject the rest of the schema. + + A diagnostic lists at most :data:`_MAX_REPORTED` paths and counts the rest, so + fixing the ones named can reveal more on the next run. + + Not checked: the free-form content of ``ai_context``, whose object form sets + ``additionalProperties: true``. A value there outside JSON's own data model -- + a YAML set or date, a non-string mapping key -- is passed through and + serialized as whatever it maps to, exactly as ``model_validate`` does today. + Auditing that, and bounding how far a shared alias graph expands on the way + out, is a property of the models rather than of parsing, and is not in scope + here. + """ + return cls._from_document(yaml.safe_load(text)) + + @classmethod + def from_ossie_json(cls, text: str) -> "OssieDocument": + """Parse an Ossie JSON document, checking what pydantic alone cannot. + + The counterpart of :meth:`to_ossie_json`; see :meth:`from_ossie_yaml` for what + is and is not checked. + """ + return cls._from_document(json.loads(text)) + + @classmethod + def _from_document(cls, data: Any) -> "OssieDocument": + for reason in (_NULL, _NON_FINITE): + paths = [path for path, why in _invalid_properties(data) if why == reason] + if paths: + # A document may hold many; list enough to act on and count the rest. + listed = ", ".join(paths[:_MAX_REPORTED]) + if len(paths) > _MAX_REPORTED: + listed += f", and {len(paths) - _MAX_REPORTED} more" + raise ValueError(f"property {reason}: {listed}") + if isinstance(data, dict) and "version" in cls.model_fields and "version" not in data: + # The field carries a default so Python construction needs no version, but + # the schema lists it as required, so a document without one is invalid. + raise ValueError("document is missing the required property: version") + return cls.model_validate(data) diff --git a/python/tests/test_serialization.py b/python/tests/test_serialization.py index 78d3fd31..275e9888 100644 --- a/python/tests/test_serialization.py +++ b/python/tests/test_serialization.py @@ -16,10 +16,17 @@ # under the License. import json +from typing import Any, get_args +import pytest import yaml +from pydantic import BaseModel, ValidationError + +from ossie import models from ossie import ( + OssieAIContextObject, + OssieDataset, OssieDialect, OssieDocument, ) @@ -165,3 +172,477 @@ def test_to_ossie_json_accepts_exclude_none_override(document_data: dict) -> Non parsed = json.loads(document.to_ossie_json(exclude_none=False)) assert parsed["description"] is None assert parsed["relationships"] is None + + +def test_from_ossie_yaml_round_trips(document_data: dict) -> None: + text = OssieDocument.model_validate(document_data).to_ossie_yaml() + + assert OssieDocument.from_ossie_yaml(text).to_ossie_yaml() == text + + +def test_from_ossie_json_round_trips(document_data: dict) -> None: + text = OssieDocument.model_validate(document_data).to_ossie_json() + + assert OssieDocument.from_ossie_json(text).to_ossie_json() == text + + +@pytest.mark.parametrize( + ("mutate", "reported"), + [ + (lambda d: d.update(description=None), "description"), + (lambda d: d["datasets"][0].update(description=None), "datasets[0].description"), + (lambda d: d["datasets"][0]["fields"][0].update(label=None), "datasets[0].fields[0].label"), + ( + lambda d: d["datasets"][0]["fields"][0].update(dimension={"is_time": None}), + "datasets[0].fields[0].dimension.is_time", + ), + (lambda d: d["metrics"][0].update(datatype=None), "metrics[0].datatype"), + ( + lambda d: d["datasets"][0].update(ai_context={"instructions": None}), + "datasets[0].ai_context.instructions", + ), + ], +) +def test_loaders_reject_a_property_set_to_null(document_data: dict, mutate, reported: str) -> None: + """No property in the schema is nullable, at any depth. + + Accepted, `exclude_none=True` would drop the key and emit a document the schema + accepts, so the null would never be reported. + """ + mutate(document_data) + text = yaml.safe_dump(document_data) + + with pytest.raises(ValueError, match="set to null") as error: + OssieDocument.from_ossie_yaml(text) + assert _reported_paths(error.value) == {reported} + + with pytest.raises(ValueError, match="set to null"): + OssieDocument.from_ossie_json(json.dumps(document_data)) + + +def test_loaders_allow_a_null_inside_free_form_ai_context(document_data: dict) -> None: + """`ai_context` sets `additionalProperties: true`, so an extra key may hold null.""" + document_data["datasets"][0]["ai_context"] = {"instructions": "use me", "vendor_hint": None} + + document = OssieDocument.from_ossie_yaml(yaml.safe_dump(document_data)) + + ai_context = document.datasets[0].ai_context + assert isinstance(ai_context, OssieAIContextObject) + assert ai_context.instructions == "use me" + assert ai_context.model_extra == {"vendor_hint": None} + + +def test_model_validate_stays_lenient_about_null(document_data: dict) -> None: + """Deliberate asymmetry: building a model in Python may pass None to mean "omit". + + Converters rely on it -- `label=dim.label` -- so only the document loaders, which + read the artefact the schema governs, reject an explicit null. + """ + document_data["description"] = None + + assert OssieDocument.model_validate(document_data).description is None + + +def _reported_paths(error: Exception) -> set[str]: + """The paths a loader named, as exact entries. + + A substring check would not distinguish `description` from + `datasets[0].description`, so an implementation that dropped the root null would + still pass. + """ + listed = str(error).rsplit(": ", 1)[1] + return {entry.strip() for entry in listed.split(",")} + + +def _model_types(annotation: object) -> list[type[BaseModel]]: + """The model classes inside an annotation, through Optional, list and Union.""" + if isinstance(annotation, type) and issubclass(annotation, BaseModel): + return [annotation] + return [model for arg in get_args(annotation) for model in _model_types(arg)] + + +def _missing_properties(model: type[BaseModel], sample: object, where: str) -> list[str]: + """Properties of `model`, and of every model below it, absent from `sample`.""" + if not isinstance(sample, dict): + return [] + missing: list[str] = [] + for name, field in model.model_fields.items(): + key = field.alias or name + if key not in sample: + missing.append(f"{where}.{key}" if where else key) + continue + value = sample[key] + items = value if isinstance(value, list) else [value] + for nested in _model_types(field.annotation): + for index, item in enumerate(items): + at = f"{where}.{key}" if where else key + if isinstance(value, list): + at = f"{at}[{index}]" + missing += _missing_properties(nested, item, at) + return missing + + +def _fully_populated_document() -> dict: + """A document that sets every optional property, so each can be nulled in turn.""" + expression = {"dialects": [{"dialect": "ANSI_SQL", "expression": "amount"}]} + ai_context = {"instructions": "i", "synonyms": ["s"], "examples": ["e"]} + extensions = [{"vendor_name": "COMMON", "data": "{}"}] + return { + "version": "0.2.0.dev0", + "name": "m", + "description": "d", + "ai_context": dict(ai_context), + "custom_extensions": list(extensions), + "datasets": [ + { + "name": "orders", + "source": "db.s.orders", + "description": "d", + "primary_key": ["id"], + "unique_keys": [["id"]], + "ai_context": dict(ai_context), + "custom_extensions": list(extensions), + "fields": [ + { + "name": "amount", + "expression": dict(expression), + "dimension": {"is_time": False}, + "label": "l", + "description": "d", + "datatype": "Decimal", + "ai_context": dict(ai_context), + "custom_extensions": list(extensions), + } + ], + } + ], + "relationships": [ + { + "name": "r", + "from": "orders", + "to": "orders", + "from_columns": ["id"], + "to_columns": ["id"], + "ai_context": dict(ai_context), + "custom_extensions": list(extensions), + } + ], + "metrics": [ + { + "name": "revenue", + "expression": dict(expression), + "description": "d", + "datatype": "Decimal", + "ai_context": dict(ai_context), + "custom_extensions": list(extensions), + } + ], + } + + +def _property_paths(node: object, path: str = "") -> list[str]: + found: list[str] = [] + if isinstance(node, dict): + for key, value in node.items(): + where = f"{path}.{key}" if path else str(key) + found.append(where) + found += _property_paths(value, where) + elif isinstance(node, list): + for index, item in enumerate(node): + found += _property_paths(item, f"{path}[{index}]") + return found + + +def _set_null(document: dict, path: str) -> None: + cursor: Any = document + parts = path.replace("]", "").split(".") + for part in parts[:-1]: + if "[" in part: + key, index = part.split("[") + cursor = cursor[key][int(index)] + else: + cursor = cursor[part] + last = parts[-1] + if "[" in last: + key, index = last.split("[") + cursor[key][int(index)] = None + else: + cursor[last] = None + + +def test_the_sample_document_sets_every_declared_field() -> None: + """Guard the test below, whose reach is only as wide as the hand-built sample. + + The walk in `models.py` is generic over keys, so a field added later is rejected + without touching it. This sample is not: a new field is only exercised once it is + added here, so fail loudly instead of silently covering less. + + Checked per model and position, not as a pool of names: `description` on a metric + is a different property from `description` on a dataset, and a sample that set only + one of them would otherwise look complete. + """ + missing = _missing_properties(OssieDocument, _fully_populated_document(), "") + + assert missing == [], "add these to _fully_populated_document()" + + +def test_every_property_in_a_document_rejects_null() -> None: + """Null each property of a fully populated document in turn; all must be rejected.""" + paths = _property_paths(_fully_populated_document()) + assert len(paths) > 50, "the sample document should exercise every property" + + for path in paths: + document = _fully_populated_document() + _set_null(document, path) + with pytest.raises(ValueError, match="set to null"): + OssieDocument.from_ossie_yaml(yaml.safe_dump(document)) + + +def test_every_null_is_reported_not_just_the_first(document_data: dict) -> None: + document_data["description"] = None + document_data["datasets"][0]["description"] = None + + with pytest.raises(ValueError) as error: + OssieDocument.from_ossie_yaml(yaml.safe_dump(document_data)) + + assert _reported_paths(error.value) == {"description", "datasets[0].description"} + + +def test_ai_context_may_be_a_plain_string(document_data: dict) -> None: + """`AIContext` is a union, so the walk must tolerate the string branch.""" + document_data["ai_context"] = "free text" + + assert OssieDocument.from_ossie_yaml(yaml.safe_dump(document_data)).ai_context == "free text" + + +@pytest.mark.parametrize("text", ["[]\n", "null\n", "42\n", ""]) +def test_loaders_reject_a_document_that_is_not_a_mapping(text: str) -> None: + with pytest.raises(ValidationError): + OssieDocument.from_ossie_yaml(text) + + +def test_a_null_inside_an_array_is_rejected_by_the_field_type(document_data: dict) -> None: + """Array items are typed, so pydantic rejects a null there; the walk need not. + + Nothing is laundered: unlike an optional property, a null item is not a value the + model would accept and then drop on serialization. + """ + document_data["datasets"][0]["primary_key"] = [None] + + with pytest.raises(ValidationError): + OssieDocument.from_ossie_yaml(yaml.safe_dump(document_data)) + + +def test_loaders_reject_a_cyclic_alias() -> None: + """`yaml.safe_load` resolves a self-referencing alias into a container holding itself. + + Walking it unguarded raises RecursionError, which is worse than the + ValidationError `model_validate` gives for the same input. + """ + text = ( + "version: 0.2.0.dev0\n" + "name: m\n" + "datasets: &d\n" + "- name: orders\n" + " source: db.s.orders\n" + " fields: *d\n" + ) + + with pytest.raises(ValueError, match="cyclic alias"): + OssieDocument.from_ossie_yaml(text) + + +def test_loaders_accept_an_alias_reused_without_a_cycle(document_data: dict) -> None: + """Only containers on the current branch count, so a shared anchor still walks.""" + text = ( + "version: 0.2.0.dev0\n" + "name: m\n" + "datasets:\n" + "- name: a\n" + " source: db.s.a\n" + " ai_context: &ctx {instructions: shared}\n" + " primary_key: &pk [id]\n" + " unique_keys: [*pk]\n" + "- name: b\n" + " source: db.s.b\n" + " ai_context: *ctx\n" + ) + + document = OssieDocument.from_ossie_yaml(text) + + second = document.datasets[1].ai_context + assert isinstance(second, OssieAIContextObject) + assert second.instructions == "shared" + + +def test_loaders_reject_a_cycle_inside_free_form_ai_context() -> None: + """A null is legal among `ai_context` extras; a cycle is not. + + `extra="allow"` keeps the value as given, so a self-referential one loads and then + fails in `to_ossie_json`. The extras are checked for cycles without their nulls + being reported. + """ + text = ( + "version: 0.2.0.dev0\n" + "name: m\n" + "datasets:\n" + "- name: o\n" + " source: db.s.o\n" + " ai_context: {vendor: &v [*v]}\n" + ) + + with pytest.raises(ValueError, match="cyclic alias"): + OssieDocument.from_ossie_yaml(text) + + +def test_a_reused_alias_graph_does_not_expand_exponentially() -> None: + """Each anchor doubles the expanded size, so touching every occurrence costs 2^n. + + The anchored subtree holds a null, which is the case that bounding the traversal + alone does not cover: visiting each container once still produced one path per + occurrence. With 64 anchors that is 2^64 paths, so completing at all is the + assertion, and the single path names the anchor the author has to edit. + """ + lines = [ + "version: 0.2.0.dev0", + "name: m", + "datasets:", + "- name: o", + " source: db.s.o", + " x0: &a0 {label: null}", + ] + lines += [f" x{i}: &a{i} [*a{i - 1}, *a{i - 1}]" for i in range(1, 65)] + document = yaml.safe_load("\n".join(lines) + "\n") + + assert models._invalid_properties(document) == [("datasets[0].x0.label", models._NULL)] + + +def test_a_null_under_a_reused_anchor_is_reported_once() -> None: + """An anchor is one property in the source, so one path locates it. + + Reporting each occurrence would be unbounded: an alias graph can hold 2^n copies + of the anchored subtree. The path given is the first the walk reaches, which is + where the anchor is defined. + """ + text = ( + "version: 0.2.0.dev0\n" + "name: m\n" + "datasets:\n" + "- name: a\n" + " source: db.s.a\n" + " fields: &f\n" + " - name: x\n" + " label: null\n" + " expression: {dialects: [{dialect: ANSI_SQL, expression: x}]}\n" + "- name: b\n" + " source: db.s.b\n" + " fields: *f\n" + ) + + with pytest.raises(ValueError) as error: + OssieDocument.from_ossie_yaml(text) + + assert _reported_paths(error.value) == {"datasets[0].fields[0].label"} + + +def test_a_long_list_of_nulls_is_truncated_with_a_count() -> None: + """A diagnostic a reader can use, rather than every path in a large document.""" + document_data = { + "version": "0.2.0.dev0", + "name": "m", + "datasets": [ + {"name": f"d{i}", "source": f"db.s.d{i}", "description": None} for i in range(30) + ], + } + + with pytest.raises(ValueError) as error: + OssieDocument.from_ossie_yaml(yaml.safe_dump(document_data)) + + assert "and 10 more" in str(error.value) + assert str(error.value).count("description") == models._MAX_REPORTED + + +@pytest.mark.parametrize( + "text", + [ + " ai_context: {score: .nan}\n", + " ai_context: {score: .inf}\n", + " ai_context: {score: -.inf}\n", + " ai_context: {scores: [1, .nan]}\n", + ], +) +def test_loaders_reject_a_non_finite_number_from_yaml(text: str) -> None: + """`yaml.safe_load` reads `.nan` and `.inf`, which JSON cannot express. + + They reach an `ai_context` extra untyped, and `to_ossie_json` turns NaN into + `null` -- a different value than was written. + """ + document = "version: 0.2.0.dev0\nname: m\ndatasets:\n- name: o\n source: db.s.o\n" + text + + with pytest.raises(ValueError, match="cannot round-trip"): + OssieDocument.from_ossie_yaml(document) + + +@pytest.mark.parametrize("constant", ["NaN", "Infinity", "-Infinity"]) +def test_loaders_reject_a_non_standard_json_constant(constant: str) -> None: + """`json.loads` accepts these by default, though JSON itself does not define them.""" + text = ( + '{"version":"0.2.0.dev0","name":"m","datasets":[{"name":"o",' + '"source":"db.s.o","ai_context":{"score":' + constant + "}}]}" + ) + + with pytest.raises(ValueError, match="cannot round-trip"): + OssieDocument.from_ossie_json(text) + + +def test_loaders_require_the_version_property() -> None: + """The schema lists `version` as required, but the field carries a default. + + `model_validate` would fill it in, so a document without one would load and then + serialize with a version it never declared. + """ + text = "name: m\ndatasets:\n- name: o\n source: db.s.o\n" + + with pytest.raises(ValueError, match="missing the required property: version"): + OssieDocument.from_ossie_yaml(text) + + +def test_direct_construction_still_defaults_the_version() -> None: + """Only a document must declare it; building a model in Python need not.""" + document = OssieDocument( + name="m", datasets=[OssieDataset(name="o", source="db.s.o")] + ) + + assert document.version == "0.2.0.dev0" + + +def test_free_form_ai_context_content_is_passed_through() -> None: + """The loaders do not audit the types inside `ai_context`, by design. + + Its object form sets `additionalProperties: true`, so a YAML set, a date or a + non-string key loads and serializes as whatever it maps to -- the behaviour + `model_validate` has today. Pinned so narrowing the contract is deliberate rather + than accidental, and so widening it is a visible change. + """ + text = ( + "version: 0.2.0.dev0\n" + "name: m\n" + "datasets:\n" + "- name: o\n" + " source: db.s.o\n" + " ai_context:\n" + " tags: !!set {a: null}\n" + " when: 2026-01-01\n" + " numbered: {1: one}\n" + ) + + document = OssieDocument.from_ossie_yaml(text) + extra = document.datasets[0].ai_context + + assert isinstance(extra, OssieAIContextObject) + assert json.loads(document.to_ossie_json())["datasets"][0]["ai_context"] == { + "tags": ["a"], + "when": "2026-01-01", + "numbered": {"1": "one"}, + } +