Repository navigation
Instrument record PATCH silently discards arrays supplied for existing array fields #1639
Description
PATCH /v1/instrument-records/:id silently discards any array the request supplies for a field that already holds an array. The mergeWith customizer in updateById returns the existing (destination) value whenever the source value is an array, so the stored array never changes. The request still returns 200 with the record, so the client is told the edit succeeded when it was not applied. This affects record-array fields and any other array-valued data. An array for a field that is currently absent is written, because the customizer then returns undefined and lodash falls back to its default merge.
Where
apps/api/src/instrument-records/instrument-records.service.ts:381-386
const updatedData = mergeWith(instrumentRecord.data, data, (updatedValue: unknown, sourceValue: unknown) => {
if (Array.isArray(sourceValue)) {
return updatedValue; // the existing value, not the submitted one
}
return undefined;
});Reproduce
Using the same customizer with lodash-es 4.18.1:
- existing
{ meds: [{ name: 'a' }, { name: 'b' }], x: 1 }, update{ meds: [{ name: 'c' }], x: 2 }→{ meds: [{ name: 'a' }, { name: 'b' }], x: 2 } - Expected:
{ meds: [{ name: 'c' }], x: 2 }
Through the API: PATCH a record whose data has answers: [1, 2, 3] with { "answers": [9] }. The response is 200 and the stored value is still [1, 2, 3].
Tests
apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts, test should ignore an array the update supplies for an existing array field (line 1150, with a "Pins a reported defect" comment). When fixing, invert it: expect data: { answers: [9] } and rename it (for example, "should replace an existing array field with the array the update supplies"). The neighbouring test should merge an array update element by element into array data covers records whose root data is an array. The customizer is not called for the root, so that behaviour is unaffected.
Suggested fix
Return sourceValue instead of updatedValue from the customizer, so a submitted array replaces the stored one rather than being merged index by index or dropped.
Found while raising unit test coverage in #1632.
Activity
joshunrau commentedon Oct 26, 2026
Still reproduces on
main(d86740e). The customizer is now atinstrument-records.service.ts:455-460, unchanged since f29adf1. The pinned test is at__tests__/instrument-records.service.test.ts:1121. efa9070 renamed the api.spec.tsunit tests to.test.ts.Scope
The bug is wider than existing arrays. lodash falls back to its default merge only when the customizer returns
undefined(baseMergeDeep), so a submitted array is dropped whenever the stored field is present, whatever it holds. Checked with lodash-es 4.18.1, the versionvendor/lodash-es@4.xpins:{ answers: null }patched with{ answers: [9] }stays{ answers: null }.[{ tags: [1, 2] }]patched with[{ tags: [7] }]keeps[1, 2].return sourceValuefixes both. Deleting the customizer does not: the default merge goes index by index, so[1, 2, 3]patched with[9]gives[9, 2, 3](the same trap as #932). Root-array data itself is still merged that way after the fix, so a PATCH cannot remove an element from it. That is a separate change.Related
#1723 revives both sides before the merge. A revived
SetorDateis neither an array nor a plain object, so lodash assigns it wholesale and the two fixes are independent. #1723 and #1633 also changeupdateByIdand the samedescribe('updateById')block, so they are easiest to land as one PR with onefix(api)commit each.The route has no caller in
apps/web. Its only in-repo client isodc-cli instrument-records update, whose help atcli/odc-cli:571says the patch is "merged recursively". Change it to say arrays are replaced, in the same commit.cli/AGENTS.mdasks for this because nothing else checks that file.Tests
nullstored value and one with an array nested in a root-array element. Add a record-array case: stored{ meds: [{ name: 'a', dose: 1 }, { name: 'b' }] }, patch{ meds: [{ name: 'c' }] }, expect exactly[{ name: 'c' }].ARBITRARY_SINGLE_FILEvalidates withz.any(). In a newtesting/src/specs/instrument-record-update.spec.ts, create a record with{ answers: [1, 2, 3] }throughPOST /api/v1/instrument-records, asrecord-file-deletion.spec.tsdoes. PATCH it with{ data: { answers: [9] } }, expect 200, thenGETit and expect[9]. Instrument record PATCH is scoped by delete rules instead of update rules #1633 and Instrument record PATCH rejects every update to a record with a date field, because stored dates are never revived #1723 can add their cases to this file.Triage note written with Claude Code. Code references point at
main@d86740e.