fix: correct array handling in Schema.resolve - #106
Open
ehennestad wants to merge 2 commits into
Open
Conversation
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 <noreply@anthropic.com>
Contributor
Test Results (R2022a)743 tests 726 ✅ 2m 29s ⏱️ For more details on these failures, see this check. Results for commit cf77ac2. ♻️ This comment has been updated with latest results. |
ehennestad
force-pushed
the
fix-schema-resolve-array-handling
branch
from
August 27, 2026 23:40
2cd08b4 to
cf77ac2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix-meta-type-property-introspection #106 +/- ##
=======================================================================
Coverage ? 78.91%
=======================================================================
Files ? 417
Lines ? 4031
Branches ? 0
=======================================================================
Hits ? 3181
Misses ? 850
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ehennestad
force-pushed
the
fix-schema-resolve-array-handling
branch
7 times, most recently
from
August 28, 2026 12:59
932c7e8 to
cf77ac2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects in the loop in
Schema.resolve. All of them only appear whenresolveis called on an array of instances, which is why the existing tests, which all resolve a scalar, did not catch them.The loop stopped at the first already-resolved element
An element that is already resolved and has no link depth left needs no work, but returning ends the whole method. Every reference positioned after a resolved element in the array was silently left unresolved.
[resolvedPerson, referencePerson].resolve()resolved neither.Link depth leaked between array elements
options.NumLinksToResolvewas decremented on the sharedoptionsstruct inside the loop, so the budget spent following the links of one element was taken away from the next. Resolving a two-element array with a depth of 1 gave the first element a depth of 1 and the second a depth of 0. The depth is a budget per element, and each element now derives its own remaining depth from the original value rather than mutating it.The already-resolved case printed to the command window
Resolving an instance that needs no work is not an event worth reporting, and a library function should not write to stdout regardless. The message appeared three times in a full test run.
Other changes
Resolver selection moves into a private
selectLinkResolvermethod. That removes a level of nesting and lets the two branches of the loop read as the two cases they are: resolve this node, or follow its links.Still outstanding
The recursive call discards the value returned by
resolve:linkedInstances{j}.resolve(nvPairs{:});A resolver that populates a node in place therefore works, but one that has to build a new instance and return it has no effect. That is the normal case for a stub whose type is not known until it is probed —
MixedTypeReferencein this repository, and thereferenceNode = []branch ofKGResolverin openminds-kg-sync. Fixing it needs the parent, property name and index at the point of resolution, which is the per-edge traversal protocol in #69. It is not addressed here.Tests
Three cases added to
ResolverTest, one per defect: that the loop continues past a resolved element, that the second array element gets the same depth budget as the first, and that resolving an already-resolved instance produces no command window output.🤖 Generated with Claude Code