Skip to content

fix: preserve controlled term values when deserializing #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ehennestad wants to merge 1 commit into from
Open

fix: preserve controlled term values when deserializing #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 53 additions & 6 deletions code/internal/+openminds/+abstract/ControlledTermBase.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,7 @@ function initializeControlledTerm(obj, instanceSpec, propValues)
obj(numInstances) = feval(class(obj));
end
for i = 1:numel(instanceSpec)
if isfield(instanceSpec(i), 'at_id')
iri = instanceSpec(i).at_id;
elseif isfield(instanceSpec(i), 'x_id')
iri = instanceSpec(i).x_id;
end
obj(i).deserializeFromName(iri);
obj(i).initializeFromStructure(instanceSpec(i));
end
else
error('openMINDS:ControlledTerm:InvalidInput', ...
Expand Down Expand Up @@ -81,6 +76,36 @@ function initializeControlledTerm(obj, instanceSpec, propValues)
end

methods (Access = private)
function initializeFromStructure(obj, structure)
% initializeFromStructure - Populate this term from a decoded document
%
% A structure that carries property values is a serialized
% instance and is authoritative: its values are used as they
% stand. That is the only way a term defined by a user, which by
% definition is not in the controlled instance library, can
% survive being written out and read back.
%
% A structure that carries nothing but an identifier is a
% reference. There is no data to take from it, so the term is
% looked up in the controlled instance library instead.

identifier = openminds.abstract.ControlledTermBase.getIdentifier(structure);

if openminds.abstract.ControlledTermBase.isBareReference(structure)
obj.deserializeFromName(identifier);
return
end

obj.fromStruct(structure);

% fromStruct assigns the identifier from an at_id or x_id
% field, but a document written without identifiers has
% neither, and the constructor has already generated one.
if ~openminds.abstract.ControlledTermBase.isEmptyValue(identifier)
obj.assignInstanceId(identifier);
end
end

function deserializeFromName(obj, instanceName)

import openminds.internal.getControlledInstance
Expand Down Expand Up @@ -158,6 +183,28 @@ function deserializeFromName(obj, instanceName)
typeName = string(typeName);
end

function identifier = getIdentifier(structure)
% getIdentifier - Identifier carried by a decoded document, if any

identifier = "";
if isfield(structure, 'at_id')
identifier = structure.at_id;
elseif isfield(structure, 'x_id')
identifier = structure.x_id;
end
end

function tf = isBareReference(structure)
% isBareReference - True when a document carries only JSON-LD keywords
%
% Such a document points at a term without describing it, so
% there is nothing to populate the instance from.

jsonLdKeywordFields = ["at_id", "x_id", "at_type", "x_type", "at_context"];
valueFields = setdiff(string(fieldnames(structure))', jsonLdKeywordFields);
tf = isempty(valueFields);
end

function tf = isEmptyValue(value)
if isempty(value)
tf = true;
Expand Down
39 changes: 16 additions & 23 deletions tools/tests/+ommtest/+helper/knownRoundTripGap.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
% explaining why the given openMINDS type currently fails to round trip
% through JSON-LD, or an empty string if the type is expected to succeed.
%
% Every entry here is a defect in the library, not in the test. This list
% is expected to shrink. When a fix lands, the corresponding entry must
% be removed so the round-trip test starts guarding the fixed behaviour.
% Every entry here is a defect, not a property of the test. This list is
% expected to shrink. When a fix lands, the corresponding entry must be
% removed so the round-trip test starts guarding the fixed behaviour.
%
% Input Arguments:
% typeName - Short name of an openMINDS type, e.g. "Person".
Expand All @@ -23,32 +23,25 @@

reason = "";

if isControlledTermType(typeName)
reason = "Controlled terms defined by the user lose every property " + ...
"on reload. ControlledTermBase/initializeControlledTerm discards " + ...
"the decoded struct and passes only the identifier to " + ...
"deserializeFromName, which finds no matching controlled instance " + ...
"and returns an empty object.";
return
end

if ismember(typeName, residualGapTypes())
if ismember(typeName, multiValuedControlledInstanceGap())
reason = "Multi-valued properties linking to controlled instances " + ...
"lose all but the first entry on reload.";
end
end

function tf = isControlledTermType(typeName)
className = openminds.enum.Types(typeName).ClassName;
tf = any(ismember(superclasses(className), {'openminds.abstract.ControlledTerm'}));
elseif typeName == "TermSuggestion"
reason = "addExistingTerminology is typed as an openMINDS type but " + ...
"is not registered in LINKED_PROPERTIES, so it serializes inline " + ...
"without a type or an identifier and cannot be read back. The " + ...
"generated controlled term classes do not declare that constant " + ...
"at all, so fixing it means changing the generator.";
end
end

function typeNames = residualGapTypes()
% Types that fail for reasons other than the controlled term defect.
function typeNames = multiValuedControlledInstanceGap()
% Types holding a multi-valued property that links to controlled instances.
%
% Unlike the controlled term case there is no clean structural predicate
% for these, so they are listed explicitly. Determined by sweeping every
% type through save and load; see the round-trip test for the procedure.
% 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", ...
Expand Down
33 changes: 33 additions & 0 deletions tools/tests/unitTests/ControlledTermTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,39 @@ function testKnownInstanceCreatesLightweightReference(testCase)
openminds.constant.BaseURI + "/instances/contributionType/authoring")
end

function testUserDefinedTermSurvivesDeserialization(testCase)
% A controlled term defined by a user is not in the controlled
% instance library, so there is nothing to look it up by. Its
% values have to be taken from the document it was read from.

structure = struct( ...
'at_id', "_:a-user-defined-term", ...
'at_type', "https://openminds.om-i.org/types/Species", ...
'name', "Novel species", ...
'definition', "A species that is not in the library.", ...
'synonym', {{'first synonym', 'second synonym'}});

term = openminds.controlledterms.Species(structure);

testCase.verifyEqual(term.name, "Novel species")
testCase.verifyEqual(term.definition, "A species that is not in the library.")
testCase.verifyEqual(term.synonym, ["first synonym", "second synonym"])
testCase.verifyEqual(string(term.id), "_:a-user-defined-term")
end

function testReferenceToKnownTermIsLookedUp(testCase)
% A document carrying only an identifier describes nothing, so the
% term is populated from the controlled instance library instead.

structure = struct( ...
'at_id', "https://openminds.om-i.org/instances/species/homoSapiens");

term = openminds.controlledterms.Species(structure);

testCase.verifyEqual(term.name, "Homo sapiens")
testCase.verifyNotEmpty(term.definition)
end

function testOlderControlledTermPropertiesAreAccepted(testCase)
sourceText = fileread(testCase.getControlledTermBasePath("v2"));

Expand Down
Loading