[#801] depends_on compat via accessors + query rewriter; daqmetadatareader/daqreader_ndr resolution#802
Merged
Merged
Conversation
Work in progress on issue #801. The depends_on `.id` mirror onto the body is replaced by a tolerant-read / canonical-write pattern inside the ndi.document dependency accessors, and a constructor- level normalisation step that canonicalises the body's depends_on struct array to use the V_delta `document_id` key. Why: the previous mirror extended the depends_on struct array's field schema to `{name, value, id}`, which broke any append or merge site that built a fresh `{name, value}` entry (heterogeneousStrucAssignment). #799 patched two sites but the codebase has more; this approach removes the fragility class entirely instead of auditing every write site forever. This commit changes: - src/ndi/+ndi/+compat/fieldAliases.m * Drop the dependsOn row entirely. Header documentation rewritten to explain the new compat model (entry-key compatibility lives in ndi.document accessors and translateQueryPaths, not on the body). * Add the daqmetadatareader.reader_class field-level alias row (decision A from #801). - src/ndi/+ndi/+compat/augmentRead.m * Strip out the depends_on .id mirror (the old i_augmentDependsOn and i_augmentDependsOnEntry helpers are gone; the main loop no longer touches depends_on). * Docstring updated. - src/ndi/+ndi/+compat/reconcileWrite.m * Same removal on the write side: the i_reconcileDependsOn and i_reconcileDependsOnEntry helpers are gone; the main loop no longer touches depends_on. * Docstring updated. - src/ndi/+ndi/+compat/translateQueryPaths.m * Keeps rewriting depends_on entry-key paths on the query side (`depends_on.id` and `depends_on.value` -> `.document_id`), but the rewrite is now hardcoded in code rather than driven by the now-deleted fieldAliases.dependsOn row. * Indexed `depends_on(N).<key>` paths still preserved. - src/ndi/+ndi/+compat/normalizeDependsOn.m (NEW) * Walks a body's depends_on struct array and rewrites every entry's `id` or `value` key to `document_id` (precedence document_id > value > id when multiple are populated). Drops the legacy keys. Produces a canonical `{name, document_id}` schema. * Called from the ndi.document constructor so internal accessors can rely on the invariant. - src/ndi/+ndi/document.m * Constructor calls ndi.compat.normalizeDependsOn after ndi.compat.augmentRead. * Revert the plus() / set_dependency_value workarounds that routed through ndi.compat.dependsOnAppend (now deleted). - src/ndi/+ndi/+compat/dependsOnAppend.m -- DELETED - tests/+ndi/+unittest/+compat/DependsOnAppendTest.m -- DELETED Both existed only to paper over the field-schema fragility that this commit removes. With the mirror gone, the helper has no callers; with the helper gone, its test is moot. TODO before this PR is ready to land: - Update ndi.document dependency-accessor methods (set_dependency_value, dependency_value, add_dependency_value_n, remove_dependency_value_n, dependency) to use the document_id key. The constructor invariant means they don't need per-call tolerance, but the literal field name in the helpers is still `.value`. - Sweep src/ndi/ for direct depends_on field access. Known offenders from earlier audit: tuningcurve.m:525, stimulus_response.m:112. - Hardcode `'ndi.daq.reader.mfdaq.ndr'` for daqreader_ndr docs in ndi.database.fun.ndi_document2ndi_object (decision B). - Update affected test files (the AugmentReadTest / ReconcileWriteTest / TranslateQueryPathsTest cases that asserted depends_on .id mirroring or test fragility-helper behavior). - Companion did-matlab PR for did.document's parallel dependency-accessor surface.
Continues from 0c7d288. Completes items 1-4 of the #801 plan (decisions A and B; defers (5) the did-matlab companion to a separate PR). Item 1 - ndi.document dependency accessors use document_id The constructor's ndi.compat.normalizeDependsOn pass guarantees the body's depends_on entries use the V_delta canonical key. The accessors can therefore use `document_id` directly: - dependency_value, dependency_value_n: read via the new i_readDependencyTarget static helper (tolerant of all three spellings -- document_id / value / id -- for callers that bypass the constructor with hand-built structs). - set_dependency_value: writes `document_id`; the matches-found branch updates `(matches(1)).document_id`; the append branch uses a `struct('name', ..., 'document_id', ...)` and direct struct-array assignment (the constructor invariant keeps the schema uniform). No more dependsOnAppend roundtrip. - add_dependency_value_n: routes through set_dependency_value without building a `value`-keyed intermediate struct. - plus(): direct struct-array append/overwrite -- the same code shape we had before #799's regression fix, restored now that the schema-extension fragility is gone. Item 2 - Sweep src/ndi/ for direct depends_on field access Migrated every read site that touched `.value` directly to use ndi.document.i_readDependencyTarget. Sites: - src/ndi/+ndi/validate.m:156 (dependency-resolution loop) - src/ndi/+ndi/session.m:127 (database_rm cascade) - src/ndi/+ndi/calculator.m:199, 245 (parameters.depends_on) - src/ndi/+ndi/+database/+metadata_ds_core/convertFormDataToDocuments.m (recursive session-id stamping) - src/ndi/+ndi/+database/+fun/finddocs_missing_dependencies.m - src/ndi/+ndi/+database/+fun/docs2graph.m - src/ndi/+ndi/+example/+tutorial/tutorial_02_05.m Migrated every write/build site to produce canonical `document_id`: - src/ndi/+ndi/+calc/+stimulus/tuningcurve.m:525 - src/ndi/+ndi/+mock/+fun/stimulus_response.m:112 Migrated one query-path that hardcoded `depends_on.value`: - src/ndi/+ndi/+setup/+NDIMaker/imageDocMaker.m:94 (`ndi.query('','depends_on',name,value)` operation-style calls are unaffected -- the op uses param1/param2 for name/value, not a `.field` path, so no rewrite needed.) Item 3 - daqreader_ndr reconstruction class ndi.database.fun.ndi_document2ndi_object now resolves the MATLAB constructor class via a small helper that: - Prefers the legacy `ndi_<parent>_class` field if present on the body (covers every class whose V_delta migrator passes the field through OR is aliased back via fieldAliases -- e.g., daqmetadatareader.ndi_daqmetadatareader_class is restored by augmentRead's new alias row). - Falls back to a hardcoded `overrides` map for classes whose V_delta migrator drops the field. Currently only daqreader_ndr -> 'ndi.daq.reader.mfdaq.ndr' (audited: every v1 daqreader_ndr instance stored the same constant string, so no per-instance information is lost). Future drops can extend the map. - Errors loudly if neither path resolves, with a message naming the document class and the missing field so the next dropped class surfaces clearly in CI. Item 4 - Tests - FieldAliasesTest: dropped test_depends_on_value_id_rename; added test_daqmetadatareader_reader_class_row. The returns_struct test now asserts dependsOn is NOT a field of aliases. - AugmentReadTest: removed all depends_on-mirroring tests (the mirror is gone). Added test_daqmetadatareader_reader_class_mirrored (new alias row) and test_does_not_touch_depends_on (regression guard so the mirror never comes back). - ReconcileWriteTest: removed all depends_on-rewrite tests (reconcile no longer touches depends_on). Added test_does_not_touch_depends_on regression guard. Updated idempotency + round-trip tests to use the `document_id` key on the input fixtures. - TranslateQueryPathsTest: depends_on.id and depends_on.value both rewrite to depends_on.document_id; depends_on.document_id is identity. ndi.query constructor acceptance test updated. - NormalizeDependsOnTest (new): 11 tests covering precedence, empty fallback, idempotency, missing/empty depends_on, multi-entry struct array, and the ndi.document-constructor acceptance.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if isfield(ndi_document_obj.document_properties.depends_on, 'id') | ||
| ndi_document_obj.document_properties.depends_on(matches(1)).id = value; | ||
| end | ||
| ndi_document_obj.document_properties.depends_on(matches(1)).document_id = value; |
| if numel(matches)>0 | ||
| notfound = 0; | ||
| d{i} = getfield(ndi_document_obj.document_properties.depends_on(matches(1)),'value'); | ||
| d{i} = ndi.document.i_readDependencyTarget( ... |
Contributor
The resolveReconstructorClass helper introduced in 33ea0df mixed function-definition styles. The file's outer function used the implicit-end convention, but I added an explicit `end` to it when extracting the helper -- MATLAB rejects mixed styles with 'MATLAB:m_mixed_closed_and_open_function_defs' (surfaced in PR #802 CI). Drop the explicit `end` on the outer function so both functions use the implicit-end convention matching the rest of the file.
Contributor
Test Results674 tests - 4 638 ✅ - 40 1m 58s ⏱️ - 3m 1s For more details on these failures and errors, see this check. Results for commit e5399e4. ± Comparison against base commit ff62f21. ♻️ This comment has been updated with latest results. |
CI failure on PR #802: NormalizeDependsOnTest/test_empty_depends_on_array_canonicalises_schema asserted that an empty `struct('name', {}, 'value', {})` would be canonicalised to `{name, document_id}`. The original logic used `rmfield(deps, 'value')` to drop the legacy key, but the loop that adds `document_id` doesn't run for n=0 -- so the schema ended up as just `{name}`, neither legacy nor canonical. Fix: detect n==0 early and rebuild the 0x0 struct array with exactly `{name, document_id}`. Same shape the test expects; matches the same fix landing concurrently in did.document/i_normalizeDependsOn on did-matlab#138.
…fires The previous fix (e5399e4) added an n==0 branch that rebuilds the struct array with the canonical {name, document_id} schema, but my top-level guard early-returned before reaching it: if ~isfield(body, 'depends_on') || isempty(body.depends_on) return; end For an empty struct array `struct('name', {}, 'value', {})`, `isempty(...)` is true so we returned without canonicalising and the test_empty_depends_on_array_canonicalises_schema test stayed red on PR #802. Drop the `isempty` part of the early-return; the inner i_normalize handles the n==0 case correctly now that the branch exists.
Last remaining symmetry-test failure on #802: MATLAB:catenate:structFieldBad in ndi.calculator/search_for_input_parameters line 161 The cat(1, fixed_depends_on, extra_depends) trips struct-array schema-mismatch because: - fixed_depends_on came from a normalized doc (so its depends_on uses {name, document_id} after the #801 work). - extra_depends was being built fresh with vlt.data.emptystruct( 'name','value') and s = struct('name',..., 'value',...) -- so it carried {name, value}. Two fields differ; cat fails. Fixes: - vlt.data.emptystruct seed at lines 137 + 149 now uses 'document_id' instead of 'value'. - struct(...) build at line 156 likewise uses 'document_id', and the is_valid_dependency_input call reads s.document_id. - After fetching fixed_depends_on from parameters_specification, wrap-and-normalize via ndi.compat.normalizeDependsOn so older callers that built depends_on with `value` or `id` keys flow through cleanly (defensive; not strictly required given the init change above but cheap and removes the latent class of bug at this call site).
Two more sites the previous symmetry-test pass surfaced after the calculator fix: 1. ndi.calc.example.simple.calculate line 42 read `parameters.depends_on(1).value` directly. Migrated to ndi.document.i_readDependencyTarget. 2. ndi.calc.stimulus.tuningcurve.calculate line 33 called vlt.db.struct_name_value_search(depends_on, name) -- that helper hard-requires a `.value` field on the struct, but depends_on is now V_delta-canonical `.document_id`. Inlined the name-lookup + tolerant-target-read via i_readDependencyTarget so all three spellings (document_id / value / id) flow.
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
3 participants
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.
Closes #801.
Summary
Implements the #801 audit's C-route design: depends_on entry-key compatibility moves off the body and into accessors + query rewriter, eliminating the struct-array schema-extension fragility that bit us in PR #800. Plus the two other decisions from the audit:
daqmetadatareader.reader_class ← ndi_daqmetadatareader_classalias row.daqreader_ndr → ndi.daq.reader.mfdaq.ndrreconstruction class (the V_delta migrator drops the legacy field; the value was always the same constant, so no information is lost).What changed
Compat layer
ndi.compat.fieldAliases—dependsOnrow removed. Addeddaqmetadatareader.reader_classrow. Documentation rewritten to explain the new model.ndi.compat.augmentRead/ndi.compat.reconcileWrite— depends_on handling stripped from both. Field-level alias rows still drive the field-by-field copy/strip.ndi.compat.translateQueryPaths— keeps rewritingdepends_on.id/depends_on.value→depends_on.document_idon the query side. The rewrite is now hardcoded rather than driven by the (removed)aliases.dependsOnrow.ndi.compat.normalizeDependsOn(new) — walks a body'sdepends_onstruct array and canonicalises entries to{name, document_id}. Acceptsid/value/document_idon input with precedencedocument_id > value > id. Called from thendi.documentconstructor.ndi.compat.dependsOnAppend— deleted. The struct-array fragility class it papered over no longer exists.ndi.document
ndi.compat.normalizeDependsOnafteraugmentRead, establishing the invariant: "after construction, depends_on entries usedocument_id."i_readDependencyTarget(entry)— tolerant single-entry reader for callers that bypass the constructor with hand-built structs.set_dependency_value: writesdocument_id; direct struct-array assignment/append; no compat-helper roundtrip.dependency_value,dependency_value_n: read via the tolerant helper.add_dependency_value_n: routes throughset_dependency_valuecleanly.plus(): direct struct-array append/overwrite (the schema invariant from the constructor makes this safe).Sweep: direct depends_on field access migrated
i_readDependencyTarget:validate.m,session.m,calculator.m,convertFormDataToDocuments.m,finddocs_missing_dependencies.m,docs2graph.m,tutorial_02_05.m.document_id:tuningcurve.m,stimulus_response.m.imageDocMaker.m(depends_on.value→depends_on.document_id).ndi.query('','depends_on',name,value)calls untouched (the op uses param1/param2, not a.fieldpath).Decision B: ndi_document2ndi_object
New
resolveReconstructorClasshelper. Prefers the legacyndi_<parent>_classfield if present (which is the case for every class whose V_delta migrator preserves the field OR aliases it back viaaugmentRead). Falls back to a hardcodedoverridesmap for classes whose migrator drops the field. Currentlydaqreader_ndr → 'ndi.daq.reader.mfdaq.ndr'. Errors loudly if neither path resolves so the next dropped class surfaces clearly.Tests
FieldAliasesTest— droppeddependsOnassertion, addeddaqmetadatareader.reader_classrow test.AugmentReadTest— removed depends_on-mirror tests (mirror is gone); added newdaqmetadatareader.reader_classtest +test_does_not_touch_depends_onregression guard.ReconcileWriteTest— removed depends_on-rewrite tests; addedtest_does_not_touch_depends_onregression guard; fixtures updated to usedocument_id.TranslateQueryPathsTest— depends_on rewrites now targetdocument_id(both.idand.valuerewrite,.document_idis identity).NormalizeDependsOnTest(new) — 11 tests covering precedence, empty fallback, idempotency, empty/missing depends_on, multi-entry arrays, andndi.documentconstructor acceptance.Companion did-matlab PR (next, separate)
did.documenthas the same dependency-accessor surface (parallel class in+didpackage; not an inheritance relationship). Same treatment will follow on a sibling PR invh-lab/did-matlabto keep DID-only users in sync.Test plan
CI runs the new + updated unit tests plus every downstream test that touches
ndi.databasereads/writes/queries. The corpus tests (PRED, 20211116, etc.) end up exercising the full pipeline throughdid2.convert.v1_to_v2 → ndi.document(body)so the new normalizeDependsOn pass gets stress-tested on real data.Generated by Claude Code