Repository navigation
fix(query): stop query planning from mutating the caller's conditions - #1911
Conversation
Table.search()/get() took the caller's conditions by reference and annotated
them in place as it planned the query: it pushes a `{ comparator: 'sort' }`
pseudo-condition for index-order alignment, sets `descending`, caches
`estimated_count`, collapses chained conditions, and coerces values — all on
the caller's entry objects. A caller that reuses the same array or condition
objects across queries (a natural pattern for a module-level `const`) then hits
leaked state: a kept sort pseudo-condition is treated as a real valueless
condition and throws `Invalid value for attribute … "undefined"`; a stale
`descending` silently reverses a later scan; a cached `estimated_count`
misplans. Whether it surfaced depended on live index estimates, so it read as
phantom nondeterminism.
Clone the conditions array and every entry (recursing into nested and/or groups)
at intake, so all downstream planning mutation happens on our own objects and
never reaches the caller. Entries are small and shallow, so the copy is
negligible next to the query itself.
Fixes harper#1572.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
|
Reviewed; no blockers found. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents query planning from mutating the caller's conditions array in place by shallow-cloning each condition entry using a new cloneConditions helper function in resources/Table.ts. It also adds a comprehensive suite of unit tests to verify this behavior. The review feedback suggests two improvements: first, importing the plain node:assert module instead of node:assert/strict in the test file to comply with the repository's style guide; second, hoisting the cloneConditions helper function to the module scope to avoid unnecessary allocations on the query hot path.
Patch cherry-pick: cancelledThe |
Post-review follow-up. The array-form-target regression case only checked array length + absence of a sort pseudo-condition, which don't change on that path (no sort → no push) — so it passed with or without the fix. Assert instead that the caller's condition entry is untouched: its Date-typed bound stays the original string (not coerced in place) and no estimated_count is annotated. Now fails on origin/main and passes with the fix, like the other three cases. Also note in cloneConditions why chainedConditions sub-entries are left shared (read-only during planning). Comment generated by kAIle (Claude Opus 4.8)
…sert in test Review follow-up (both non-blocking): - cloneConditions is stateless (no closure over search/makeTable), so hoist it to module scope rather than re-creating the function on every search() call. - Use plain node:assert in the regression test per house style, with explicit strictEqual/deepStrictEqual where strict semantics are wanted. Comment generated by kAIle (Claude Opus 4.8)
| */ | ||
| // Shallow-clone each condition entry (recursing into nested `and`/`or` groups) so | ||
| // query planning never writes through to the caller's objects (harper#1572). | ||
| // Array-form entries (`[attribute, value]`) and primitives pass through as-is; |
There was a problem hiding this comment.
Minor wording nit: array-form entries do not “pass through as-is” here—they are intentionally cloned below. Could this say they “remain array-form” (while primitives pass through as-is) so the comment matches the ownership guarantee?
| assert.deepStrictEqual(seen, ascending, 'reused query scanned in reverse from a leaked descending flag'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The implementation adds special cloning paths for tuple entries and nested groups, but every condition exercised here is a top-level object. Could we add one nested group that gets reordered/estimated and one [attribute, value] condition to lock down those two branches? This is non-blocking for #1572's reported path, but it would protect the stated recursive/tuple guarantees.
|
I just noticed this was labeled as a patch. Is that really necessary? |
…nditions" (#1911) from v5.1 PR #1911 targeted main (5.2) and was cherry-picked onto v5.1 by mistake; it was never labeled for the patch train. This reverts all three of its commits (39f0dd4, 30477f2, 3af8eef), restoring resources/Table.ts to its v5.1.23 state and removing unitTests/resources/conditionsArrayMutation.test.js. The fix remains on main and can be cherry-picked deliberately later if we decide it belongs in a 5.1.x patch.
…nditions" (#1911) from v5.1 PR #1911 targeted main (5.2) and was cherry-picked onto v5.1 by mistake; it was never labeled for the patch train. This reverts all three of its commits (39f0dd4, 30477f2, 3af8eef), restoring resources/Table.ts to its v5.1.23 state and removing unitTests/resources/conditionsArrayMutation.test.js. The fix remains on main and can be cherry-picked deliberately later if we decide it belongs in a 5.1.x patch.
3 participants
Summary
Table.search()/get()took the caller's conditions by reference and annotated them in place while planning the query — it pushes a{ comparator: 'sort' }pseudo-condition for index-order alignment, setsdescending, cachesestimated_count, collapses chained conditions, and coerces values, all on the caller's entry objects. A caller that reuses the same array or condition objects across queries (a natural pattern for a module-levelconst BASE = [...]) then reads leaked state.Purpose
Fixes #1572. The most visible symptom is the reported throw: a kept sort pseudo-condition is treated as a real, valueless condition on reuse and fails coercion with
Invalid value for attribute … "undefined". Because whether the pseudo-condition is kept depends on live index estimates (it's kept when it sorts first), it presented as phantom nondeterminism — the reporter lost a day to it. Kris confirmed and escalated it.The fix
Clone the conditions array and every entry (recursing into nested
and/orgroups) at intake, so all downstream planning mutation lands on our own objects and never reaches the caller. Two hunks inresources/Table.tsplus a regression test.Where to look
descending,estimated_count, chained-condition collapse, and coercion still leak onto the caller's objects, and a bare array-alias check also misses thesearch(conditionsArray)(array-as-target) form. Hence the full entry clone. Worth confirming the clone depth is right (array + entries + nested.conditions; array-form[attr, value]entries preserved viaObject.assign).Tests
New
unitTests/resources/conditionsArrayMutation.test.js— four cases: array not mutated after a sort query, reuse doesn't throw, array-form target not mutated, and a leakeddescendingdoesn't reverse a later reused query. Three of the four fail on unfixed code (verified by revertingresources/Table.tstoorigin/main).query.test.js(105) +crud+dataLoader+getRecordCountpass unchanged.Note
Small, safe, defensive change to a real correctness bug present since ≥5.1.15 — a v5.1 patch candidate.
Generated by an LLM (Claude Opus 4.8).