Skip to content

fix: preserve controlled term values when deserializing - #107

Open
ehennestad wants to merge 1 commit into
fix-schema-resolve-array-handlingfrom
fix-controlled-term-deserialization
Open

fix: preserve controlled term values when deserializing#107
ehennestad wants to merge 1 commit into
fix-schema-resolve-array-handlingfrom
fix-controlled-term-deserialization

Conversation

@ehennestad

@ehennestad ehennestad commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

A controlled term read back from a JSON-LD document lost every property. This affected all 112 controlled term types, so no controlled term survived a save and load cycle.

The defect

Saving a user-defined MRIFatSuppressionTechnique produced a complete document, and loading it produced this:

{ "@id": "_:6322ef38-...", "@type": "https://openminds.om-i.org/types/MRIFatSuppressionTechnique" }

Name, definition, description, synonyms, cross-references and ontology identifiers were all gone, and the identifier had changed.

initializeControlledTerm received the decoded structure and took only the identifier from it:

elseif isstruct( instanceSpec ) && (isfield(instanceSpec, 'at_id') || ...)
    ...
    obj(i).deserializeFromName(iri);   % the rest of the structure is dropped

deserializeFromName then looks the identifier up in the controlled instance library. For a term defined by a user that identifier is a blank node identifier which matches nothing, so the method warned and returned, leaving an empty object. The constructor had already generated a new identifier, which is why the @id changed too.

The fix

The two cases a structure can represent are now distinguished.

A structure that carries property values is a serialized instance. Its values are authoritative and are used as they stand, which is the only way a term that is not in the controlled instance library can survive being written out and read back.

A structure that carries nothing but JSON-LD keywords is a reference. It describes no values, so there is nothing to populate from and the term is resolved against the controlled instance library exactly as before.

Terms constructed from a name or an instance IRI are unaffected: that path does not go through the structure branch.

Effect on coverage

The round-trip suite goes from 150 to 261 of 292 types. 111 of the 112 controlled terms now round trip, and the controlled term predicate is removed from ommtest.helper.knownRoundTripGap.

What is still failing

TermSuggestion is the one controlled term that still fails, for an unrelated reason. Its addExistingTerminology property is declared as openminds.controlledterms.Terminology but is not registered in LINKED_PROPERTIES, so the serializer does not recognise it as a link. It serializes inline as a bare property bag with no @type and no @id, and on reload the value cannot be converted back, so the whole instance is dropped with a warning.

The cause is that none of the 112 generated controlled term classes declare LINKED_PROPERTIES or EMBEDDED_PROPERTIES. They inherit hardcoded empty structs from ControlledTermBase, which encodes the assumption that a controlled term never points at another openMINDS type. That assumption holds for 111 of them. Fixing it means changing the generator, so it is out of scope here and is now listed on its own in the known-gap list.

Worth noting separately: because the instance is dropped, the collection ends up empty, and Collection.getAll then fails on an unset dictionary rather than returning nothing. That is a small independent defect.

Tests

Two cases added to ControlledTermTest, one for each branch of the new distinction: a structure carrying values populates the term from those values, and a structure carrying only an identifier is looked up in the library. They construct the structure directly rather than going through the synthesizer, so they document the contract independently of the round-trip suite.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Test Results (R2022a)

745 tests  +2   742 ✅ +15   2m 27s ⏱️ -6s
 19 suites ±0     3 💤  - 13 
  1 files   ±0     0 ❌ ± 0 

Results for commit 84aee20. ± Comparison against base commit c6812d6.

♻️ This comment has been updated with latest results.

@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch from 8973203 to 80eb833 Compare August 27, 2026 23:51
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (fix-schema-resolve-array-handling@c6812d6). Learn more about missing BASE report.

Additional details and impacted files
@@                         Coverage Diff                          @@
##             fix-schema-resolve-array-handling     #107   +/-   ##
====================================================================
  Coverage                                     ?   79.05%           
====================================================================
  Files                                        ?      417           
  Lines                                        ?     4048           
  Branches                                     ?        0           
====================================================================
  Hits                                         ?     3200           
  Misses                                       ?      848           
  Partials                                     ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch 6 times, most recently from c9ab714 to 80eb833 Compare August 28, 2026 12:59
@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch from aeb96e7 to f2b4b43 Compare August 31, 2026 09:01
@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch from f2b4b43 to 93d9372 Compare August 31, 2026 10:39
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Test Results (R2026a)

745 tests  +745   742 ✅ +742   2m 19s ⏱️ + 2m 19s
 19 suites + 19     3 💤 +  3 
  1 files   +  1     0 ❌ ±  0 

Results for commit 84aee20. ± Comparison against base commit c6812d6.

♻️ This comment has been updated with latest results.

@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch from 93d9372 to 20a5dbd Compare September 1, 2026 14:31
A controlled term read back from a document lost every property. All 112
controlled term types were affected, so no controlled term survived a
save and load cycle.

initializeControlledTerm received the decoded structure, took only the
identifier from it, and passed that to deserializeFromName. The rest of
the structure was discarded. For a term defined by a user, the identifier
is a blank node identifier that matches nothing in the controlled
instance library, so deserializeFromName warned and returned, leaving an
empty object with a freshly generated identifier.

A structure is now treated as authoritative when it carries property
values, because that is a serialized instance and its values are the only
record of a user-defined term. A structure carrying nothing but JSON-LD
keywords describes no values, so it is still resolved against the
controlled instance library as a reference.

This takes the round-trip suite from 150 to 261 of 292 types. The
controlled term predicate is removed from the known-gap list.

TermSuggestion still fails, for an unrelated reason: its
addExistingTerminology property is typed as an openMINDS type but is not
registered in LINKED_PROPERTIES, because the generated controlled term
classes do not declare that constant at all. It serializes inline without
a type or an identifier and cannot be read back. That needs a generator
change and is now listed on its own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad force-pushed the fix-controlled-term-deserialization branch from 20a5dbd to 84aee20 Compare September 1, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant