From 8a6c8c95b294f2cfd531b42985e5a2baa0f0dfa9 Mon Sep 17 00:00:00 2001 From: ehennestad Date: Fri, 28 Aug 2026 02:03:58 +0200 Subject: [PATCH] fix: keep every element when constructing controlled terms from a struct array A multi-valued property that links to controlled instances kept only its first entry when read back. An AtlasAnnotation with laterality set to both left and right reloaded with left alone. Deserializing such a property assigns a struct array of references to it, which MATLAB converts by calling the controlled term constructor with the whole array. initializeControlledTerm expands obj to one element per struct, but it is a method on a handle class that returned nothing, so the expansion applied only to the local copy. The constructor kept the original scalar handle, which is the first element, and the rest were discarded. The method now returns the object array and the three constructors that call it take the returned value. This takes the round-trip suite from 261 to 288 of 292 types, and removes 30 entries from the known-gap list. The four remaining failures are unrelated to each other and are now listed individually: two types fail because serializing an unresolved MixedTypeReference is rejected by the meta type registry, ChemicalSubstance fails because a controlled instance whose name is not a valid MATLAB identifier cannot be looked up from a bare reference, and TermSuggestion fails because of the missing LINKED_PROPERTIES entry described in the previous PR. Co-Authored-By: Claude Opus 5 --- .../+openminds/+abstract/ControlledTerm.m | 2 +- .../+openminds/+abstract/ControlledTermBase.m | 8 ++++- .../controlledTerms/v2/ControlledTerm.m | 2 +- .../controlledTerms/v3/ControlledTerm.m | 2 +- .../+ommtest/+helper/knownRoundTripGap.m | 33 ++++++------------- tools/tests/unitTests/ControlledTermTest.m | 32 ++++++++++++++++++ 6 files changed, 52 insertions(+), 27 deletions(-) diff --git a/code/internal/+openminds/+abstract/ControlledTerm.m b/code/internal/+openminds/+abstract/ControlledTerm.m index 5beb166b..fb6bae36 100644 --- a/code/internal/+openminds/+abstract/ControlledTerm.m +++ b/code/internal/+openminds/+abstract/ControlledTerm.m @@ -35,7 +35,7 @@ propValues.id (1,1) string end - obj.initializeControlledTerm(instanceSpec, propValues) + obj = obj.initializeControlledTerm(instanceSpec, propValues); end end end diff --git a/code/internal/+openminds/+abstract/ControlledTermBase.m b/code/internal/+openminds/+abstract/ControlledTermBase.m index 14d69693..81fc29c2 100644 --- a/code/internal/+openminds/+abstract/ControlledTermBase.m +++ b/code/internal/+openminds/+abstract/ControlledTermBase.m @@ -19,7 +19,13 @@ end methods (Access = protected) - function initializeControlledTerm(obj, instanceSpec, propValues) + function obj = initializeControlledTerm(obj, instanceSpec, propValues) + % initializeControlledTerm - Populate one or more terms from a spec + % + % The object array is returned because a struct array spec + % produces one term per element. Expanding obj inside this method + % only grows the local copy, so a caller that ignores the return + % value keeps just the first element. if isstring(instanceSpec) && isscalar(instanceSpec) && instanceSpec == "" instanceSpec = string.empty; end diff --git a/code/internal/+openminds/+abstract/private/controlledTerms/v2/ControlledTerm.m b/code/internal/+openminds/+abstract/private/controlledTerms/v2/ControlledTerm.m index fe4ed0c3..c1c19d1f 100644 --- a/code/internal/+openminds/+abstract/private/controlledTerms/v2/ControlledTerm.m +++ b/code/internal/+openminds/+abstract/private/controlledTerms/v2/ControlledTerm.m @@ -32,7 +32,7 @@ propValues.id (1,1) string end - obj.initializeControlledTerm(instanceSpec, propValues) + obj = obj.initializeControlledTerm(instanceSpec, propValues); end end end diff --git a/code/internal/+openminds/+abstract/private/controlledTerms/v3/ControlledTerm.m b/code/internal/+openminds/+abstract/private/controlledTerms/v3/ControlledTerm.m index 5beb166b..fb6bae36 100644 --- a/code/internal/+openminds/+abstract/private/controlledTerms/v3/ControlledTerm.m +++ b/code/internal/+openminds/+abstract/private/controlledTerms/v3/ControlledTerm.m @@ -35,7 +35,7 @@ propValues.id (1,1) string end - obj.initializeControlledTerm(instanceSpec, propValues) + obj = obj.initializeControlledTerm(instanceSpec, propValues); end end end diff --git a/tools/tests/+ommtest/+helper/knownRoundTripGap.m b/tools/tests/+ommtest/+helper/knownRoundTripGap.m index c5ba8f64..ede3ef4d 100644 --- a/tools/tests/+ommtest/+helper/knownRoundTripGap.m +++ b/tools/tests/+ommtest/+helper/knownRoundTripGap.m @@ -23,9 +23,16 @@ reason = ""; - if ismember(typeName, multiValuedControlledInstanceGap()) - reason = "Multi-valued properties linking to controlled instances " + ... - "lose all but the first entry on reload."; + if ismember(typeName, ["ParcellationTerminologyVersion", "QuantitativeRelationAssessment"]) + reason = "Serializing a property that holds an unresolved " + ... + "MixedTypeReference fails, because the meta type registry does " + ... + "not recognise it as a metadata type."; + + elseif typeName == "ChemicalSubstance" + reason = "A controlled instance whose name contains characters that " + ... + "are not valid in a MATLAB identifier cannot be looked up from a " + ... + "bare reference, so it reloads as an empty term with a new " + ... + "blank node identifier."; elseif typeName == "TermSuggestion" reason = "addExistingTerminology is typed as an openMINDS type but " + ... @@ -35,23 +42,3 @@ "at all, so fixing it means changing the generator."; end end - -function typeNames = multiValuedControlledInstanceGap() -% Types holding a multi-valued property that links to controlled instances. -% -% There is no clean structural predicate for these, because many types -% with such a property round trip correctly, so they are listed -% explicitly. Determined by sweeping every type through save and load. - - typeNames = [ ... - "Accessibility", "AtlasAnnotation", "ChemicalSubstance", ... - "ContentType", "CustomAnnotation", "DataAnalysis", "DataCopy", ... - "DatasetVersion", "Dependency", "File", "FileBundle", ... - "FilePathPattern", "GenericComputation", "LocalFile", ... - "ModelValidation", "Optimization", "ParcellationTerminologyVersion", ... - "QuantitativeRelationAssessment", "Setup", "Simulation", ... - "SoftwareVersion", "SubjectGroup", "SubjectGroupState", ... - "SubjectState", "TissueSample", "TissueSampleCollection", ... - "TissueSampleCollectionState", "TissueSampleState", ... - "ValidationTest", "Visualization"]; -end diff --git a/tools/tests/unitTests/ControlledTermTest.m b/tools/tests/unitTests/ControlledTermTest.m index dc07f088..167d96f6 100644 --- a/tools/tests/unitTests/ControlledTermTest.m +++ b/tools/tests/unitTests/ControlledTermTest.m @@ -30,6 +30,38 @@ function testUserDefinedTermSurvivesDeserialization(testCase) testCase.verifyEqual(string(term.id), "_:a-user-defined-term") end + function testStructArrayProducesOneTermPerElement(testCase) + % A multi-valued property deserializes to a struct array of + % references. Each element must become its own term, or every + % entry after the first is lost. + + references = struct('at_id', { ... + "https://openminds.om-i.org/instances/laterality/left", ... + "https://openminds.om-i.org/instances/laterality/right"}); + + terms = openminds.controlledterms.Laterality(references); + + testCase.assertNumElements(terms, 2) + testCase.verifyEqual(string(terms(1).id), ... + "https://openminds.om-i.org/instances/laterality/left") + testCase.verifyEqual(string(terms(2).id), ... + "https://openminds.om-i.org/instances/laterality/right") + end + + function testMultiValuedControlledPropertyKeepsEveryEntry(testCase) + % The same case reached through a property rather than the + % constructor, which is how deserialization gets there. + + references = struct('at_id', { ... + "https://openminds.om-i.org/instances/laterality/left", ... + "https://openminds.om-i.org/instances/laterality/right"}); + + annotation = openminds.sands.AtlasAnnotation(); + annotation.laterality = references; + + testCase.verifyNumElements(annotation.laterality, 2) + end + function testReferenceToKnownTermIsLookedUp(testCase) % A document carrying only an identifier describes nothing, so the % term is populated from the controlled instance library instead.