From d2124e9841de34619005e852433a9164cf0498db Mon Sep 17 00:00:00 2001 From: ehennestad Date: Fri, 28 Aug 2026 01:07:34 +0200 Subject: [PATCH] fix: correct array handling in Schema.resolve Three defects in the same loop, all of which only show up when resolve is called on an array of instances. An element that was already resolved and had no link depth left returned from the method instead of moving to the next element, so every reference after the first resolved element was silently left unresolved. The link depth was decremented on the shared options struct inside the loop, so depth spent on one element was taken away from the next. Each element now derives its own remaining depth. The already-resolved case wrote to the command window. Resolving an instance that needs no work is not an event worth reporting, and the message appeared three times in a full test run. Resolver selection moves to a private method, which removes a level of nesting from the loop. Note that the recursive call still discards the instance returned by resolve, so a resolver that replaces a node rather than populating it in place has no effect. That needs the traversal rework in openMINDS-MATLAB issue #69 and is not addressed here. Co-Authored-By: Claude Opus 5 --- code/internal/+openminds/+abstract/Schema.m | 72 ++++++++++++--------- tools/tests/unitTests/ResolverTest.m | 61 +++++++++++++++++ 2 files changed, 102 insertions(+), 31 deletions(-) diff --git a/code/internal/+openminds/+abstract/Schema.m b/code/internal/+openminds/+abstract/Schema.m index 24755424..6f61a9b0 100644 --- a/code/internal/+openminds/+abstract/Schema.m +++ b/code/internal/+openminds/+abstract/Schema.m @@ -117,41 +117,34 @@ % options.IsEmbedded = false - Todo? end - instance = obj; % Initialize output for i = 1:numel(obj) - if ~obj(i).IsReference % Instance is resolved (not a reference) - if options.NumLinksToResolve == 0 - fprintf('Instance is already resolved.\n') - instance(i) = obj(i); - return - else - options.NumLinksToResolve = options.NumLinksToResolve-1; - nvPairs = namedargs2cell(options); - linkedInstances = obj(i).getLinkedInstances(); - for j = 1:numel(linkedInstances) - linkedInstances{j}.resolve(nvPairs{:}); - end - embeddedInstances = obj(i).getEmbeddedInstances(); - for j = 1:numel(embeddedInstances) - embeddedInstances{j}.resolve(nvPairs{:}); - end - end - else - if isfield(options, 'LinkResolver') - resolver = options.LinkResolver; - else - resolver = openminds.internal.getLinkResolver([obj(i).id]); - end + if obj(i).IsReference + resolver = obj(i).selectLinkResolver(options); + obj(i) = resolver.resolve(obj(i), ... + "NumLinksToResolve", options.NumLinksToResolve); + obj(i).IsReference = false; % Update state: mark as resolved - if isempty(resolver) - error(... - 'openMINDS:LinkResolver:NotFound', ... - 'No link resolver found for object with id "%s".', obj(i).id); + elseif options.NumLinksToResolve > 0 + % The instance itself is resolved, so spend one unit of + % depth following its links. The remaining depth is + % derived per element rather than by decrementing + % options, which would leak the budget already spent on + % one element into the next. + childOptions = options; + childOptions.NumLinksToResolve = options.NumLinksToResolve - 1; + nvPairs = namedargs2cell(childOptions); + + linkedInstances = obj(i).getLinkedInstances(); + for j = 1:numel(linkedInstances) + linkedInstances{j}.resolve(nvPairs{:}); + end + embeddedInstances = obj(i).getEmbeddedInstances(); + for j = 1:numel(embeddedInstances) + embeddedInstances{j}.resolve(nvPairs{:}); end - - obj(i) = resolver.resolve(obj(i), "NumLinksToResolve", options.NumLinksToResolve); - obj(i).IsReference = false; % Update state: mark as resolved end + % An instance that is already resolved and has no depth left + % to spend needs no work. end instance = obj; % Set output end @@ -668,6 +661,23 @@ end end + methods (Access = private) + function resolver = selectLinkResolver(obj, options) + % selectLinkResolver - Resolver for this instance, from options or registry + + if isfield(options, 'LinkResolver') + resolver = options.LinkResolver; + else + resolver = openminds.internal.getLinkResolver(obj.id); + end + + if isempty(resolver) + error('openMINDS:LinkResolver:NotFound', ... + 'No link resolver found for object with id "%s".', obj.id); + end + end + end + methods (Access = private) % Introspective utility methods function tf = isSubsForProperty(obj, subs) diff --git a/tools/tests/unitTests/ResolverTest.m b/tools/tests/unitTests/ResolverTest.m index c78f6567..52a50713 100644 --- a/tools/tests/unitTests/ResolverTest.m +++ b/tools/tests/unitTests/ResolverTest.m @@ -206,6 +206,67 @@ function testResolveWithNumLinksToResolve(testCase) testCase.verifyEqual(authors.givenName, "Mock"); end + function testResolveArrayContinuesPastResolvedElement(testCase) + % Every element of an array must be considered. An element that is + % already resolved must not stop the loop, or references later in + % the array are silently left unresolved. + + mockResolver = ommtest.helper.mock.MockLinkResolver(); + openminds.registerLinkResolver(mockResolver); + + resolvedPerson = openminds.core.Person(); + resolvedPerson.givenName = "Already"; + + referencePerson = openminds.core.Person('id', 'https://mock.io/person_after'); + + instances = [resolvedPerson, referencePerson]; + instances.resolve(); + + testCase.verifyEqual(instances(1).givenName, "Already", ... + 'The resolved element should be left alone.') + testCase.verifyEqual(instances(2).givenName, "Mock", ... + 'The reference after a resolved element should still be resolved.') + end + + function testResolveDepthIsPerArrayElement(testCase) + % The link depth is a budget for each element of the array, not a + % budget shared across the whole array. Resolving the links of one + % element must not exhaust the depth available to the next. + + mockResolver = ommtest.helper.mock.MockLinkResolver(); + openminds.registerLinkResolver(mockResolver); + + firstDataset = ResolverTest.createDatasetWithAuthors( ... + openminds.core.Person('id', 'https://mock.io/author_first'), ... + "First Dataset"); + secondDataset = ResolverTest.createDatasetWithAuthors( ... + openminds.core.Person('id', 'https://mock.io/author_second'), ... + "Second Dataset"); + + datasets = [firstDataset, secondDataset]; + datasets.resolve( ... + 'NumLinksToResolve', ResolverTest.datasetAuthorResolveDepth()); + + firstAuthor = ResolverTest.getDatasetAuthors(datasets(1)); + secondAuthor = ResolverTest.getDatasetAuthors(datasets(2)); + + testCase.verifyEqual(firstAuthor.givenName, "Mock") + testCase.verifyEqual(secondAuthor.givenName, "Mock", ... + 'The second element should get the same depth budget as the first.') + end + + function testResolveIsQuiet(testCase) + % Resolving an instance that is already resolved is a no-op and must + % not write to the command window. + + resolvedPerson = openminds.core.Person(); + resolvedPerson.givenName = "Already"; + + output = evalc('resolvedPerson.resolve();'); + testCase.verifyEmpty(strtrim(output), ... + 'resolve should not print to the command window.') + end + function testResolveMultipleLinkedInstances(testCase) % Test resolving a node with multiple linked instances mockResolver = ommtest.helper.mock.MockLinkResolver();