Repository navigation
feat(core-spec): nonportable_extensions object data with portability and quoting rules (breaking) - #522
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Updates the Ossie spec and Python model to represent custom_extensions[].data as a structured object (vendor-defined map) instead of a JSON-encoded string, aligning validation and examples with the new shape.
Changes:
- Update JSON Schema / YAML spec to require
custom_extensions[].dataas anobject - Update Python model (
OssieCustomExtension) and tests to accept objects and reject non-objects - Rewrite documentation and examples to use native YAML mappings for extension data
| File | Description |
|---|---|
core-spec/ossie-schema.json |
Changes CustomExtension.data schema type to object and updates description |
core-spec/spec.yaml |
Updates the spec type annotations from string → object for custom_extensions[].data |
core-spec/spec.md |
Updates the Custom Extensions documentation and examples to use YAML objects |
docs/index.md |
Updates FAQ/glossary wording to match object-based extension data |
examples/tpcds_semantic_model.yaml |
Migrates example extension data from JSON strings to native YAML objects |
python/src/ossie/models.py |
Updates OssieCustomExtension.data from str → dict[str, Any] |
python/tests/test_validation.py |
Updates/extends validation tests for object-based data |
python/tests/test_serialization.py |
Adds round-trip tests for nested custom extension data |
python/tests/test_models.py |
Updates fixtures/examples using custom_extensions.data |
validation/tests/test_validate.py |
Adds schema-level validation tests for object vs non-object extension data |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ekut
left a comment
There was a problem hiding this comment.
Ran the converter suites against this branch merged onto current main (2491533), since the converter workflows on this PR are still waiting for approval. Spec, schema, example and the Python package agree with each other, and the core checks listed in the description pass. What breaks on merge is the converters, which still treat data as a string:
- 7 of the 14 Python converter suites fail (172 tests); all 14 are green on main. In six of them the converter writes
datawithjson.dumps(...)and the schema or the model now rejects it; Sigma buildsOssieCustomExtension(data=json.dumps(...))on the in-repo package, so Sigma imports raise wherever they write an extension. The seventh, the ontology converter, fails the other way: its ownCustomExtensionhasdata: str, so it can no longer loadexamples/tpcds_semantic_model.yamlas updated here. - A green suite doesn't mean the converter handles the new form. Given its own vendor's extension with
dataas an object, Cube, Omni, Microsoft, Databricks and Sigma raiseTypeErrorfromjson.loads, ThoughtSpot raises "expected a JSON string", and Honeydew, NVIDIA and OrionBelt catch the error and drop the extension silently. For OrionBelt, round-tripping its TPC-DS OBML fixture with objectdatadrops all 36commententries (the string path keeps them) and renames the columncustomer_full_nameto its SQL expression, with no warning. GoodData already accepts both forms (_get_gooddata_extension). converters/README.md, section "Custom Extensions", still tells converter authors thatdatais a JSON string to parse. ThoughtSpot'sREADME.mdanddocs/vendor-payload.mdsay it is never a nested object.
How would you like to sequence this: the converter changes in this PR, or readers first accept both forms the way GoodData does and then the schema flips? The order also interacts with #523: once the converters move to apache/ossie-converters, their CI no longer runs here, and these failures show up at the first submodule bump there instead. Open #378 adds new json.dumps writes for DBT, so it needs the same change whichever lands first.
Per-converter results
| Converter | Suite, main → this PR | Writes data as |
Reading object data |
|---|---|---|---|
| cube | 864 passed → 124 failed | JSON string | TypeError |
| databricks (python) | 103 passed → 103 passed | JSON string | TypeError |
| dbt | 176 passed → 176 passed | — | — |
| gooddata | 94 passed → 94 passed | JSON string | works |
| honeydew | 151 passed → 151 passed | JSON string | dropped silently |
| microsoft | 410 passed → 1 failed | JSON string | TypeError |
| nvidia | 89 passed → 2 failed | JSON string | dropped silently |
| omni | 105 passed → 105 passed | JSON string | TypeError |
| ontology | 170 passed → 1 failed | own model, data: str |
fails to load |
| orionbelt | 269 passed → 7 failed | JSON string | dropped silently |
| sigma | 103 passed → 34 failed | JSON string, rejected by the model | TypeError |
| snowflake | 186 passed → 186 passed | — | drops all extensions |
| thoughtspot | 977 passed → 3 failed | JSON string | ConversionError |
| wisdom | 37 passed → 37 passed | — | reports, doesn't parse |
Repro: cd converters/<name> && uv sync && uv run pytest (needs uv >= 0.9). Every failure is data rejected as a string by the schema or the model, except ontology, which rejects the object. The Java converters (Databricks Java, Polaris, Salesforce) were not run; Polaris models data as String.
|
@ekut We discussed in the last two OSSIE meetings that because we are currently in incubation that breaking changes occur now. Thus, the move to the new Converter Repo makes the most sense to me. Enforce the new standard in the test in the new converter repo. Tagging @jbonofre about the communication in our meeting notes about breaking changes occurring now. Here is how I would see the next steps moving:
This enables a clean cut between the two bodies of work required. This PR is in response to this discussion: |
christianeu-db
left a comment
There was a problem hiding this comment.
+1 to JB's comment in #481 - please bring this to a [DISCUSS] on the dev list so the community can weigh in before merging.
The opaque-string friction within converters is real, so I'm sympathetic to wanting to lower the per-converter cost. My main concern is that this could weaken portability in practice. Today, vendor extensions live in opaque strings. Implementations can ignore those strings and still operate on the portable parts of the model, assuming more semantics continue to land in the portable core.
Making vendor extensions structured and easily parseable could create an incentive to put more semantics there rather than in the portable core. Instead of a vendor's state being one opaque value, it can now be N individually addressable values. Over time, these could become a de facto extension mechanism that dominates the file format. We're seeing a bit of this with expressions already: we have a portable set, but around 1/3 of converters write vendor-specific dialects that other implementations may not understand (test for simple metrics in #576).
Vendors also tend to have features that are almost the same but not quite. With opaque strings, these differences stay isolated. Once those keys become structured, even under vendor-specific sections, implementations may start depending on subtly different representations of the same concepts. Over time, that could make interoperability harder rather than easier.
I also wonder whether having named vendors in the core spec implies more standardization of these extensions than actually exists. The spec says vendor names can be arbitrary strings, and there's no enforcement of the structure or semantics of their contents. I think it's reasonable for the core spec to define an extension mechanism, but I'd prefer we avoid giving the impression that vendor-specific contents are part of the portable semantic contract. #146 is adding support for annotations in the ontology layer, but that's explicitly not intended as a vendor-specific escape hatch.
More broadly, I'd like us to preserve the incentive for vendors to contribute common semantics to the portable core rather than build out parallel vendor-specific representations. Otherwise, we risk Ossie becoming a collection of vendor-specific schemas rather than a common semantic interchange format.
Separately, it's probably worth a PR to clarify how vendor extensions are documented. For example, we could use obviously fictional vendors in the core spec and move real vendor examples into a separate vendors/ folder, where vendors or individuals can register their extensions without requiring a core-spec change. I can take a pass at that.
None of this is meant to dismiss the ergonomics problem - the per-consumer parsing pain is real. If the main goal is to reduce that burden, could we address some of it through shared parsing/accessor utilities for custom_extensions in ossie-converters, without promoting the payload into the core schema? Happy to collaborate on that!
christianeu-db
left a comment
There was a problem hiding this comment.
Following #522 (review), let's run this through a discussion. This does seem to make vendor extensions internals appear as top-level extensions in Ossie
|
As we have already a thread (before the GH discussion), better to use it and route contributors to the dev mailing list. |
`_v: true` read as version 1, because Python evaluates `True == 1`. The version gate exists to refuse a payload shape this converter does not recognise, so a spelling that walks past it defeats the one check whose whole job is that refusal. This is not theoretical under apache/ossie#522. With `data` an object the payload is YAML, where a bare `true` is a boolean under both 1.1 and 1.2 rules. `_yaml.Yaml12Loader` already narrows the 1.1-only spellings, so `_v: yes` is refused -- but narrowing the resolver moves which spelling bites rather than stopping one biting, which is the point worth taking from Kyoung Min Kim's measurements on the dev@ thread. The exclusion is bool-shaped, not type-shaped: `_v: 1.0` stays accepted, since a float spelling of a version we do recognise is not the hazard and a JSON producer may legitimately write one. A test pins that distinction, because the obvious over-correction -- reject every non-int -- otherwise passes the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1397254 to
fd2e93e
Compare
kayemkim
left a comment
There was a problem hiding this comment.
One gap between the two readers the spec now names. OssieDocument.from_ossie_yaml accepts a date or timestamp under data, and model_dump_json writes it back as a string, so the document validate.py fails with date is not a JSON value comes out of the SDK as one that passes, with the value retyped on the way. The loader already walks data in _invalid_below (models.py:82) to refuse .inf and .nan there, and the reason it gives, that JSON cannot express the value, covers a date too. The docstring leaves a date under ai_context out of scope on purpose, and data is walked with the same free-form flag, but data now has a spec sentence that ai_context does not, so a date/datetime branch in that walk (and bytes from !!binary, which dumps as "hi") would make the two readers agree on a document while model construction stays lenient as the docstring intends. Keys below the top level are the smaller half of the same gap: dict[str, Any] rejects {1: x} directly under data but accepts it one level down and dumps the key as "1".
The rest holds. A date, a timestamp, .inf, .nan, a non-string key, !!binary, a reused anchor, the old field name and a string data each produce one [Schema] line at the right path at model, dataset, field and metric level; yes, 010 and 1e3 pass as the section says; the core examples, the ontology documents and flights.mapping.yaml validate unchanged. The tree merges clean onto main and onto #697 and #703, and no custom_extensions is left outside the migration text.
af249e0 to
1b8a634
Compare
christianeu-db
left a comment
There was a problem hiding this comment.
@MikeCarlo thanks for incorporating the feedback from the mailing list! This resolves all my core semantics concerns.
I've left a few comments on the spec text, mostly around trimming wording. The biggest opportunity is removing migration details and references to the old custom_extensions name/shape. Given we're still pre-release, I think it's fine for the spec to describe only the new structure. As we get closer to merge, we can file issues in ossie-converters to track any necessary migration work.
I'll defer to @kayemkim for the initial signoff on validation.
I'll also update the mailing list with the current state: JB's three proposed changes have been incorporated, and I recommend moving forward with lazy consensus.
kayemkim
left a comment
There was a problem hiding this comment.
The validation side is good from me. Merged onto current main with the README hunk below resolved, the validation and Python suites pass, each block in the new field guide loads and validates the way its heading says, and the cases from my last review still give one [Schema] line each at the right path.
The SDK loader difference from that review is unchanged. from_ossie_yaml takes the guide's released and stamped lines and writes them back as strings, while validate.py rejects both. It doesn't need to hold this PR, and I can send it as a follow-up after the merge if you'd rather not widen this one.
The ! in the new title fails the PR title check: the pattern in pr-title-check.yml allows no ! before the colon, and feat(core-spec): … with the same text passes. The branch also conflicts with main since #491 landed after your rebase. It is one hunk in extensions/README.md, where both sides add rows to the file table, and keeping main's dialects/ row above your two resolves it.
christianeu-db
left a comment
There was a problem hiding this comment.
@MikeCarlo this looks good to merge. I updated the title to remove the "!" so the title check passes. The only remaining issue I see is the merge conflict. Once that's resolved, let's target merging by EOD Tuesday Pacific time to give consensus a bit more time to settle.
…red vendor-specific data
…-specific metadata
…-specific metadata
… rules Add a migration note for string data, require JSON-representable data, require consumers to preserve unknown data, note null preservation, align Vendor examples with extensions/vendors.md, and test non-object rejection at every custom_extensions location.
Core semantics do not depend on any property in the extension area. The old field name is not accepted. validate.py rejects dates, timestamps, and non-finite floats under data, and the spec states the YAML retyping that check cannot catch. BREAKING CHANGE: custom_extensions is renamed to nonportable_extensions with no alias. data remains an object. A date, timestamp, or non-finite float under data is a schema error. Co-authored-by: Mike Carlo <mike@powerbi.tips>
Add a field guide for nonportable_extensions with the YAML cases validate.py rejects and the ones it still accepts after retyping. Co-authored-by: Mike Carlo <mike@powerbi.tips>
Keep the custom_extensions rename only in the 0.3.0.dev0 version history. Point the schema data description at the spec section, and leave quoting details in the field guide. Co-authored-by: Mike Carlo <mike@powerbi.tips>
Add the unreleased 0.3.0.dev0 rename and object-data change to the significant-change history, which is where migration impact belongs. Co-authored-by: Mike Carlo <mike@powerbi.tips>
b6555b9 to
c794470
Compare
Keep nested extension payloads from being mutated after validation, and serialize them back to plain dicts and lists. Co-authored-by: Mike Carlo <mike@powerbi.tips>
| @model_validator(mode="after") | ||
| def _freeze_data(self) -> "OssieNonportableExtension": | ||
| # Pydantic rebuilds a Mapping annotation as a dict, which would leave | ||
| # the payload mutable. Replace that copy with a proxy of a new tree. | ||
| object.__setattr__(self, "data", _freeze_json(self.data)) | ||
| return self |
There was a problem hiding this comment.
@MikeCarlo - let's keep this mutable this now. Addressing mutability can be covered in a followup PR
Since data became a MappingProxyType, a document with an extension can't be copied or pickled, and json.dumps(ext.data) fails:
copy.deepcopy(doc): TypeError: cannot pickle 'mappingproxy' object
doc.model_copy(deep=True): TypeError: cannot pickle 'mappingproxy' object
pickle.dumps(doc): TypeError: cannot pickle 'mappingproxy' object
json.dumps(ext.data): TypeError: Object of type mappingproxy is not JSON serializable
With the TPC-DS example, all four worked before this commit. Other frozen models hold plain lists (datasets, primary_key), so could we drop this commit and keep data a dict?
…a separate PR) This reverts commit 77659ec. Co-authored-by: Mike Carlo <mike@powerbi.tips>
apache/ossie#522 renames `custom_extensions` to `nonportable_extensions` and does not accept the old name. Unlike the object form of `data` — where a reader could in principle wait, because the string form stays valid — there is no version of that change under which waiting is free: every document written after it lands would be unreadable here until we shipped a release of our own. So `read_stash` now takes the entry list from either name, the same readers-first order this PR already applies to the shape of `data`. The two axes are independent: a document may use either name with either shape. `write_stash` has NOT moved. It emits today's name, with one exception that is not a move: a document already carrying the new name is kept on it. Writing the old name there would hand back a document under both names, which the reader refuses — we would have broken our own round trip. A document arriving with both is refused rather than resolved by precedence, because reading one list while extending the other drops whatever was in the loser, silently. Error messages now name the field the document actually used; one that named the other name would send the reader to the wrong line. Mutations checked, each turning a test red: writer always emits today's name; reader knows only today's name; both-present refusal removed; presence tested by truthiness instead of by key. 1106 tests pass. docs/vendor-payload.md regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
christianeu-db
left a comment
There was a problem hiding this comment.
lgtm - thanks for going through a few iterations on this @MikeCarlo
apache/ossie#522 renamed custom_extensions to nonportable_extensions and made its data an object instead of a string holding JSON, keeping the schema version 0.3.0.dev0. Vendor apache-ossie at af7630c, read and write the entry in the new form, and name the change when a model still uses the old one: the SDK refuses it, so it is not read. Messages show data values without json.dumps on them directly, so a frozen data object (apache/ossie#713) still prints. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Summary
Changes
custom_extensions[].datafrom a JSON-encoded string to a structured object with vendor-defined keys, implementing Proposal A from discussion #481.This is an intentional hard break in the unreleased
0.3.0.dev0spec. There is no deprecation period, since the project is still in early incubation. There is no 0.2.0 release: documents that declareversion: 0.2.0.dev0must declare0.3.0.dev0. The field is also renamed tononportable_extensions. The old name is not accepted.Motivation
Today
datais an opaque JSON string:Problems with this:
JSON.parse.Converters increasingly use extensions to carry unmapped or partially mapped properties, so this friction keeps growing.
Change
datais now an object, andcustom_extensionshas been renamednonportable_extensionsto enforce the ideal that "Core semantics MUST NOT depend on extension contents":data. Vendor schemas live outside core-spec.null, arrays, nested objects.datais still required.{}is valid.data: { text: '{"…"}' }. A bare string is no longer valid.YAML quoting
datamust be JSON after YAML loads it.validation/validate.pyrejects values that are not JSON. Quote scalars that YAML would otherwise read as a boolean, a number, or a date. Worked examples: extensions/nonportable_extensions.md.Unquoted dates, timestamps, and the non-finite forms
.infand.nanare rejected. The same characters in quotes are strings and pass:yes,NO,true,false, and010also pass when quoted ("yes","NO","true","false","010"). Left unquoted they still pass validation, but as a boolean or an integer (and010as the integer 8), not as the text that was written. The validator cannot catch that. Unquoted1e3stays the string1e3under this project's PyYAML, but other loaders may read 1000, so quote"1e3"when the characters matter.Files changed
core-spec/ossie-schema.jsonnonportable_extensions.$defs/NonportableExtension.datais"type": "object"with no keys defined underdata. Documentversionis0.3.0.dev0.core-spec/spec.mdcustom_extensions.core-spec/spec.yamldata: stringbecomesdata: object.extensions/nonportable_extensions.mddataexamples.examples/tpcds_semantic_model.yamlpython/src/ossie/models.pyOssieNonportableExtension.dataisdict[str, Any].docs/index.mdvalidation/validate.py,python/tests/*,validation/tests/test_validate.pydata, rejection of dates and non-finite floats, and the residual retyping cases.Validation
Local checks after the rebase onto
0.3.0.dev0:uv run validation/test_validate.pyuv run --with pytest --with pyyaml --with jsonschema --with sqlglot -m pytest validation/tests/uv run --with pytest --with jsonschema --with ./python -m pytest python/tests/uv run scripts/generate-spec-types.py --checkuv run validation/validate.pyon TPC-DS, flights, and renamed-fields examplesGitHub required workflows are still waiting on fork-workflow approval, so CI is not green yet.
Checklist
Specification
core-spec/and follow the existing structureOntology
Ontology files were updated for the field rename (
ontology/mapping.json,ontology/ontology.md).Converters (Out of scope)
Validation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changed (no process change)Examples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green) - core tests (local pytest passed; GitHub CI is waiting on fork-workflow approval)Compliance