feat(python): add document loaders that reject a property set to null by ayushtkn · Pull Request #563 · apache/ossie

ayushtkn · 2026-10-11T09:38:05Z

Summary

Followup from #557 covering other fields as well

Three rules in ossie-schema.json cannot be expressed by the model fields, so the SDK
accepts documents the schema rejects and quietly rewrites them:

input	schema	SDK
`{"is_time": null}`	rejected — no property is nullable	accepted, serialized as `{}`
`{"score": NaN}` / `.nan`	not expressible in JSON	accepted, comes back as `null`
no `version`	rejected — `required: [version, name, datasets]`	accepted, gains `0.2.0.dev0`

Each is laundered rather than reported: the invalid input becomes valid output, so the
mistake disappears.

Why not in `model_validate`

Building a model in Python routinely passes a value that may be None and means "omit
it" — label=dim.label in dbt, description=data.get(...) in Sigma. An AST scan found
143 such call sites, 119 at field level and 24 at document level, including
converter source: sigma_to_ossie.py (19), wisdom_to_ossie.py (16),
msi_to_ossie.py (9). Rejecting a null there breaks callers rather than improving
conformance, and the version default is wanted there for the same reason.

What the loaders do

A document is the artefact the schema governs, and to_ossie_yaml / to_ossie_json
had no input counterpart:

OssieDocument.from_ossie_yaml(text)
OssieDocument.from_ossie_json(text)

They reject, naming the path to each: a property set to null; a non-finite number
(yaml.safe_load reads .nan and .inf, json.loads accepts the non-standard NaN,
Infinity, -Infinity); a missing version; and a cyclic alias, which
yaml.safe_load resolves into a container holding itself.

property set to null, which the schema does not allow; omit it instead:
datasets[0].fields[0].dimension.is_time, metrics[0].datatype

A diagnostic lists at most 20 paths and counts the rest, so fixing the named ones can
reveal more on the next run. A container reached again through an alias is reported
once, at the path where the anchor is defined: an anchor is one property in the source,
and a chain shaped [*previous, *previous] expands to 2^n copies, so expanding every
occurrence is unbounded.

model_validate stays lenient, pinned by a test, so the converters keep working.

What they deliberately do not do

The free-form content of ai_context is not audited — its object form sets
additionalProperties: true. A value there outside JSON's data model (a YAML set or
date, a non-string mapping key) is passed through and serialized as whatever it maps
to, and a shared alias graph still expands on the way out. This is unchanged from
main, where model_validate accepts all of it:

!!set {a: null} -> ["a"] 2026-01-01 -> "2026-01-01" {1: one} -> {"1": "one"}
18 anchors in an ai_context extra: to_ossie_json takes 586 ms, emits 4,194,456 bytes
a null extra is dropped by exclude_none=True

A test pins that passthrough with the exact serialized forms, so widening the contract
later is a visible change. Auditing free-form value types, bounding output expansion
and preserving a null extra are properties of the models and the serializer rather than
of parsing; they affect main today and are tracked separately.

Related Issues

Checklist

Specification

Spec changes are included in core-spec/ and follow the existing structure
Spec changes have been discussed on the mailing list or in a linked issue
Breaking changes to the spec are clearly called out in the summary

Ontology

Ontology changes in ontology/ are consistent with spec changes
New or modified terms are defined and documented

Converters

Converter logic in converters/ is updated to reflect spec or ontology changes
New converters include tests under the converter's test directory
If adding a new converter, .github/labeler.yml is updated with the new path

Validation

Validation rules in validation/ are updated if the spec changed
New validation cases are covered by tests

Documentation

docs/ is updated to reflect any user-facing changes
New features or behaviors are documented with examples where appropriate
CONTRIBUTING.md is updated if the contribution process changed

Examples

examples/ are added or updated for any new spec constructs or converter support

Tests

All existing tests pass (pytest / CI green)
New functionality is covered by tests

Compliance

ASF license headers are present on all new source files
No third-party dependencies are added without PMC/IPMC approval
If third-party source code is vendored/copied (not just declared as a dependency), NOTICE and/or LICENSE have been updated per ASF policy

Copilot

🟡 Changes recommended

Cyclic YAML aliases can crash the new loader with an unhandled recursion error.

2 open findings

What changed in this PR

Adds YAML/JSON document loaders that reject schema-invalid null properties while preserving lenient programmatic model construction.

Changes:

Adds recursive null-property detection with ai_context exemptions.
Adds loader APIs and comprehensive serialization tests.

File	Description
`python/src/ossie/models.py`	Implements YAML/JSON loaders and null detection.
`python/tests/test_serialization.py`	Tests loader behavior and schema-null handling.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Alias traversal permits exponential resource consumption and misses cycles inside free-form AI context values.

2 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Shared aliases containing nulls can cause exponential memory and CPU consumption.

1 open finding

Bound exponential alias path expansion to prevent resource exhaustion · New

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

The sample-completeness guard can silently miss newly added fields whose names already exist on another model.

1 open finding

Validate expected fields per model, not globally · New

1 resolved since last review

Bound exponential alias path expansion to prevent resource exhaustion

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

The loaders accept a missing required version and non-standard JSON constants.

2 open findings

1 resolved since last review

Validate expected fields per model, not globally

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

YAML-native values and highly shared alias graphs can still cause lossy serialization or resource exhaustion.

2 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Accepted null-valued AI context extras are silently removed by default serialization.

5 open findings

🧠 Review effort: Balanced

…ot 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.

jbonofre

LGTM. I checked the cycle detection, alias reuse, and null handling in ai_context, and they look all correct.

One nit: in _from_document, the message is built as f"property {reason}: ...". For non-infinite numbers it reads "property not a number JSON can represent, so it cannot round-trip: path", which is a bit ungrammatical imho. If you reword it, the tests that match on "cannot round-trip" will need updating too.

Copilot AI balanced review requested due to automatic review settings October 11, 2026 09:38

Copilot started reviewing on behalf of ayushtkn October 11, 2026 09:38

View session