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"}, + } +