fix(db): canonicalize inArray value order in normalizeExpressionPaths#1566
Draft
v-anton wants to merge 1 commit into
Draft
fix(db): canonicalize inArray value order in normalizeExpressionPaths#1566v-anton wants to merge 1 commit into
v-anton wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSorts values inside ChangesPredicate canonicalization for in-array queries
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/collection/subscription.ts (1)
492-495:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize the paginated
loadSubsetpath as well.
requestLimitedSnapshot()still forwardsthis.options.whereExpressionunchanged intoloadSubset. Any ordered/limited query with aninArraypredicate can therefore keep producing different serialized predicates for the same set when the value order changes, so the duplicate-cache-key bug remains on this path.Suggested fix
- const where = this.options.whereExpression + const where = this.options.whereExpression + const canonicalWhere = where ? canonicalizeInValues(where) : undefined const whereFilterFn = where ? createFilterFunctionFromExpression(where) : undefined @@ const loadOptions: LoadSubsetOptions = { - where, // Main filter only, no cursor + where: canonicalWhere, // Main filter only, no cursor limit, orderBy, cursor: cursorExpressions, // Cursor expressions passed separately offset: offset ?? currentOffset, // Use provided offset, or auto-tracked offset subscription: this,Also applies to: 646-653
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/collection/subscription.ts` around lines 492 - 495, requestLimitedSnapshot currently passes this.options.whereExpression unchanged into loadSubset causing non-canonical predicates (e.g., inArray order variations) to produce different cache keys; canonicalize the where expression before creating whereFilterFn and before passing into loadSubset by normalizing any set-like predicates (such as inArray) to a deterministic order. Update the logic around where / whereFilterFn (and the similar block later near the other loadSubset call) to call a canonicalize function on this.options.whereExpression (or produce a canonicalized copy) and use that canonical expression when creating createFilterFunctionFromExpression and when calling loadSubset/requestLimitedSnapshot so identical sets always serialize the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/db/src/collection/subscription.ts`:
- Around line 492-495: requestLimitedSnapshot currently passes
this.options.whereExpression unchanged into loadSubset causing non-canonical
predicates (e.g., inArray order variations) to produce different cache keys;
canonicalize the where expression before creating whereFilterFn and before
passing into loadSubset by normalizing any set-like predicates (such as inArray)
to a deterministic order. Update the logic around where / whereFilterFn (and the
similar block later near the other loadSubset call) to call a canonicalize
function on this.options.whereExpression (or produce a canonicalized copy) and
use that canonical expression when creating createFilterFunctionFromExpression
and when calling loadSubset/requestLimitedSnapshot so identical sets always
serialize the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bfabfb6-b54b-4d60-b199-95d7ad2411eb
📒 Files selected for processing (3)
.changeset/fix-canonicalize-in-values.mdpackages/db/src/collection/subscription.tspackages/db/tests/query/canonicalize-in-values.test.ts
v-anton
force-pushed
the
fix/canonicalize-in-values
branch
from
8013a4d to
361c719
Compare
v-anton
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/query/compiler/joins.ts (1)
313-319: ⚡ Quick winClarify
target.pathis collection-level; normalization alias-stripping only applies in rare name-collision cases.
getLazyLoadTargetsconstructsLazyLoadTarget.pathby destructuringref.pathas[alias, ...path]and, forcollectionRef, returning{ alias: source.alias, collection: source.collection, path }, so the storedpathexcludes the ref alias and starts with the collection field name. That meansnormalizeExpressionPaths(..., target.alias)should not normally hit thepath[0] === collectionAliasalias-stripping branch—only a rare collision is possible if a collection field name exactly equalstarget.alias(and the ref path has length > 1), in which case the stripping could incorrectly alter the ref used forloadSubset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/compiler/joins.ts` around lines 313 - 319, The code is mistakenly set up to potentially strip a leading alias from collection-level paths: getLazyLoadTargets stores LazyLoadTarget.path without the ref alias (collection-level), so normalizeExpressionPaths(..., target.alias) should not perform the alias-stripping branch; update the call or function to avoid removing the first segment for collection-level paths—either add a boolean parameter (e.g., skipAliasStrip / pathIsCollectionLevel) to normalizeExpressionPaths and pass it from the getLazyLoadTargets usage, or change the call site to pass the correct alias/flag so the alias-stripping branch is skipped; ensure the unchanged path is then used by loadSubset and related code (references: getLazyLoadTargets, LazyLoadTarget.path, normalizeExpressionPaths, loadSubset).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/db/src/query/compiler/joins.ts`:
- Around line 313-319: The code is mistakenly set up to potentially strip a
leading alias from collection-level paths: getLazyLoadTargets stores
LazyLoadTarget.path without the ref alias (collection-level), so
normalizeExpressionPaths(..., target.alias) should not perform the
alias-stripping branch; update the call or function to avoid removing the first
segment for collection-level paths—either add a boolean parameter (e.g.,
skipAliasStrip / pathIsCollectionLevel) to normalizeExpressionPaths and pass it
from the getLazyLoadTargets usage, or change the call site to pass the correct
alias/flag so the alias-stripping branch is skipped; ensure the unchanged path
is then used by loadSubset and related code (references: getLazyLoadTargets,
LazyLoadTarget.path, normalizeExpressionPaths, loadSubset).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13611c1d-d833-414a-b199-1962d5b95c7c
📒 Files selected for processing (4)
.changeset/fix-canonicalize-in-values.mdpackages/db/src/query/compiler/expressions.tspackages/db/src/query/compiler/joins.tspackages/db/tests/query/canonicalize-in-values.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-canonicalize-in-values.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/tests/query/canonicalize-in-values.test.ts
Author
|
Thanks @coderabbitai — the suggested diff references |
v-anton
force-pushed
the
fix/canonicalize-in-values
branch
from
361c719 to
75ec05f
Compare
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/db/tests/query/canonicalize-in-values.test.ts (1)
47-91: ⚡ Quick winExtract common assertion logic to reduce duplication.
Both test cases share identical assertion logic (extract filter, find
inoperator, assert sorted values). Consider extracting this into a helper function to improve maintainability.♻️ Proposed refactor
+ function assertInArraySorted( + loadSubsetCalls: Array<LoadSubsetOptions>, + expectedValues: Array<number> + ) { + expect(loadSubsetCalls.length).toBeGreaterThan(0) + const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) + const inFilter = filters.find((f) => f.operator === `in`) + expect(inFilter).toBeDefined() + expect(inFilter!.value).toEqual(expectedValues) + } it(`sorts inArray values in the predicate passed to loadSubset`, async () => { const { collection: children, loadSubsetCalls } = createChildrenCollectionWithTracking() const liveQuery = createLiveQueryCollection((q) => q.from({ c: children }).where(({ c }) => inArray(c.parentId, [3, 1, 2])), ) await liveQuery.preload() - expect(loadSubsetCalls.length).toBeGreaterThan(0) - - const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) - const inFilter = filters.find((f) => f.operator === `in`) - - expect(inFilter).toBeDefined() - expect(inFilter!.value).toEqual([1, 2, 3]) + assertInArraySorted(loadSubsetCalls, [1, 2, 3]) }) it(`sorts inArray values for ordered/limited queries too`, async () => { const { collection: children, loadSubsetCalls } = createChildrenCollectionWithTracking() const liveQuery = createLiveQueryCollection((q) => q .from({ c: children }) .where(({ c }) => inArray(c.parentId, [3, 1, 2])) .orderBy(({ c }) => c.id) .limit(2), ) await liveQuery.preload() - expect(loadSubsetCalls.length).toBeGreaterThan(0) - - const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) - const inFilter = filters.find((f) => f.operator === `in`) - - expect(inFilter).toBeDefined() - expect(inFilter!.value).toEqual([1, 2, 3]) + assertInArraySorted(loadSubsetCalls, [1, 2, 3]) })As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/canonicalize-in-values.test.ts` around lines 47 - 91, Extract the repeated assertion logic (calling extractSimpleComparisons on loadSubsetCalls[0]!.where, finding the filter with operator `in`, and asserting its value equals [1,2,3]) into a small helper function (e.g., assertInFilterSorted) and call it from both tests; update both tests (`sorts inArray values in the predicate passed to loadSubset` and `sorts inArray values for ordered/limited queries too`) to call that helper with the relevant loadSubsetCalls array, keeping references to extractSimpleComparisons and the `in` operator lookup so the helper encapsulates the filter extraction and expectation steps.packages/db/src/query/compiler/expressions.ts (1)
53-60: 💤 Low valueConsider edge cases for empty and single-element arrays.
While empty arrays (
[]) and single-element arrays ([x]) sort correctly, add a guard to skip sorting when unnecessary for clarity and to avoid allocating a new array for trivial cases.⚡ Optimization for trivial cases
if ( whereClause.name === `in` && args.length === 2 && args[1]?.type === `val` && - Array.isArray(args[1].value) + Array.isArray(args[1].value) && + args[1].value.length > 1 ) { args[1] = new Value([...args[1].value].sort(...)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/compiler/expressions.ts` around lines 53 - 60, The current branch that normalizes IN-list values blindly creates a new sorted array even for empty or single-element lists; update the condition in the whereClause.name === `in` branch (the check that inspects args and args[1].value) to only perform the allocation and sorting when args[1].value is an array with length > 1, otherwise leave args[1] untouched, so use Array.isArray(args[1].value) && args[1].value.length > 1 before replacing args[1] with new Value([...args[1].value].sort()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/compiler/expressions.ts`:
- Around line 53-60: The current canonicalization uses Array.prototype.sort()
which sorts lexicographically and breaks numeric ordering; update the branch
that checks whereClause.name === `in` to sort the args[1].value with a
type-aware comparator before wrapping in new Value: if all items are numbers
sort numerically (a - b), if all are strings sort lexically (localeCompare),
otherwise convert items to a consistent string representation and sort
deterministically; use a copied array (e.g., [...args[1].value]) and then pass
the sorted array into new Value(...) so the canonical form for inArray (handled
whereClause.name/args and Value) is stable for numeric IDs and mixed types.
In `@packages/db/tests/query/canonicalize-in-values.test.ts`:
- Around line 16-92: Add tests covering empty and single-element inArray edge
cases inside the existing describe(`canonicalize inArray value order`) block:
use createChildrenCollectionWithTracking() and createLiveQueryCollection((q) =>
q.from({ c: children }).where(({ c }) => inArray(c.parentId, []))) and similarly
with [1], call liveQuery.preload(), assert loadSubsetCalls.length > 0, then use
extractSimpleComparisons(loadSubsetCalls[0]!.where) to find the `in` filter and
assert its value equals [] for the empty case and [1] for the single-element
case; duplicate both assertions for the ordered/limited path (add .orderBy(({ c
}) => c.id).limit(2)) to cover the requestLimitedSnapshot branch.
- Around line 47-66: The test currently only uses single-digit values so it
misses a numeric-sorting bug; add a second assertion case that calls
createLiveQueryCollection(... where inArray(c.parentId, [10, 2, 1])) (or similar
multi-digit set like [12,3,4]), preload it, extract the in-filter from
loadSubsetCalls (using extractSimpleComparisons and find where operator ===
'in') and assert that inFilter!.value equals the numerically sorted array
[1,2,10] (or [3,4,12]); this will catch the bug in expressions.ts where .sort()
is used without a numeric comparator while still using the existing helpers
loadSubset, createLiveQueryCollection, and inArray.
---
Nitpick comments:
In `@packages/db/src/query/compiler/expressions.ts`:
- Around line 53-60: The current branch that normalizes IN-list values blindly
creates a new sorted array even for empty or single-element lists; update the
condition in the whereClause.name === `in` branch (the check that inspects args
and args[1].value) to only perform the allocation and sorting when args[1].value
is an array with length > 1, otherwise leave args[1] untouched, so use
Array.isArray(args[1].value) && args[1].value.length > 1 before replacing
args[1] with new Value([...args[1].value].sort()).
In `@packages/db/tests/query/canonicalize-in-values.test.ts`:
- Around line 47-91: Extract the repeated assertion logic (calling
extractSimpleComparisons on loadSubsetCalls[0]!.where, finding the filter with
operator `in`, and asserting its value equals [1,2,3]) into a small helper
function (e.g., assertInFilterSorted) and call it from both tests; update both
tests (`sorts inArray values in the predicate passed to loadSubset` and `sorts
inArray values for ordered/limited queries too`) to call that helper with the
relevant loadSubsetCalls array, keeping references to extractSimpleComparisons
and the `in` operator lookup so the helper encapsulates the filter extraction
and expectation steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f932c254-b9ca-4070-a2c3-fa6c8d0edad3
📒 Files selected for processing (4)
.changeset/fix-canonicalize-in-values.mdpackages/db/src/query/compiler/expressions.tspackages/db/src/query/compiler/joins.tspackages/db/tests/query/canonicalize-in-values.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-canonicalize-in-values.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/query/compiler/joins.ts
Comment on lines
+53
to
+60
| if ( | ||
| whereClause.name === `in` && | ||
| args.length === 2 && | ||
| args[1]?.type === `val` && | ||
| Array.isArray(args[1].value) | ||
| ) { | ||
| args[1] = new Value([...args[1].value].sort()) | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Default .sort() breaks canonicalization for multi-digit numbers.
JavaScript's default .sort() uses lexicographic (string) comparison, which produces incorrect ordering for numbers. For example, [10, 2, 1].sort() yields [1, 10, 2] instead of [1, 2, 10]. Since inArray is commonly used with numeric IDs and foreign keys, two semantically identical predicates inArray(id, [1, 2, 10]) and inArray(id, [1, 10, 2]) would canonicalize to different orderings, defeating the entire purpose of this PR (cache hits, deduplication).
🔧 Proposed fix with type-aware comparison
if (
whereClause.name === `in` &&
args.length === 2 &&
args[1]?.type === `val` &&
Array.isArray(args[1].value)
) {
- args[1] = new Value([...args[1].value].sort())
+ args[1] = new Value([...args[1].value].sort((a, b) => {
+ // Handle primitive types with type-aware comparison
+ if (typeof a === 'number' && typeof b === 'number') {
+ return a - b
+ }
+ if (typeof a === 'string' && typeof b === 'string') {
+ return a.localeCompare(b)
+ }
+ // Fallback: stringify for comparison (matches serialization behavior)
+ const aStr = String(a)
+ const bStr = String(b)
+ return aStr < bStr ? -1 : aStr > bStr ? 1 : 0
+ }))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/query/compiler/expressions.ts` around lines 53 - 60, The
current canonicalization uses Array.prototype.sort() which sorts
lexicographically and breaks numeric ordering; update the branch that checks
whereClause.name === `in` to sort the args[1].value with a type-aware comparator
before wrapping in new Value: if all items are numbers sort numerically (a - b),
if all are strings sort lexically (localeCompare), otherwise convert items to a
consistent string representation and sort deterministically; use a copied array
(e.g., [...args[1].value]) and then pass the sorted array into new Value(...) so
the canonical form for inArray (handled whereClause.name/args and Value) is
stable for numeric IDs and mixed types.
Comment on lines
+16
to
+92
| describe(`canonicalize inArray value order`, () => { | ||
| function createChildrenCollectionWithTracking() { | ||
| const loadSubsetCalls: Array<LoadSubsetOptions> = [] | ||
|
|
||
| const collection = createCollection<Child>({ | ||
| id: `canon-children`, | ||
| getKey: (child) => child.id, | ||
| syncMode: `on-demand`, | ||
| autoIndex: `eager`, | ||
| defaultIndexType: BasicIndex, | ||
| sync: { | ||
| sync: ({ begin, write, commit, markReady }) => { | ||
| begin() | ||
| for (const child of sampleChildren) { | ||
| write({ type: `insert`, value: child }) | ||
| } | ||
| commit() | ||
| markReady() | ||
| return { | ||
| loadSubset: vi.fn((options: LoadSubsetOptions) => { | ||
| loadSubsetCalls.push(options) | ||
| return Promise.resolve() | ||
| }), | ||
| } | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| return { collection, loadSubsetCalls } | ||
| } | ||
|
|
||
| it(`sorts inArray values in the predicate passed to loadSubset`, async () => { | ||
| const { collection: children, loadSubsetCalls } = | ||
| createChildrenCollectionWithTracking() | ||
|
|
||
| // Out-of-order literal — survives normalizeExpressionPaths unsorted, so | ||
| // without canonicalization loadSubset would receive [3, 1, 2]. | ||
| const liveQuery = createLiveQueryCollection((q) => | ||
| q.from({ c: children }).where(({ c }) => inArray(c.parentId, [3, 1, 2])), | ||
| ) | ||
|
|
||
| await liveQuery.preload() | ||
|
|
||
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | ||
|
|
||
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | ||
| const inFilter = filters.find((f) => f.operator === `in`) | ||
|
|
||
| expect(inFilter).toBeDefined() | ||
| expect(inFilter!.value).toEqual([1, 2, 3]) | ||
| }) | ||
|
|
||
| it(`sorts inArray values for ordered/limited queries too`, async () => { | ||
| const { collection: children, loadSubsetCalls } = | ||
| createChildrenCollectionWithTracking() | ||
|
|
||
| // orderBy + limit takes the requestLimitedSnapshot path; the where is still | ||
| // canonicalized because it is normalized at subscription creation. | ||
| const liveQuery = createLiveQueryCollection((q) => | ||
| q | ||
| .from({ c: children }) | ||
| .where(({ c }) => inArray(c.parentId, [3, 1, 2])) | ||
| .orderBy(({ c }) => c.id) | ||
| .limit(2), | ||
| ) | ||
|
|
||
| await liveQuery.preload() | ||
|
|
||
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | ||
|
|
||
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | ||
| const inFilter = filters.find((f) => f.operator === `in`) | ||
|
|
||
| expect(inFilter).toBeDefined() | ||
| expect(inFilter!.value).toEqual([1, 2, 3]) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add edge case coverage for empty and single-element arrays.
The test suite is missing corner cases that should be verified per coding guidelines: empty arrays (inArray(c.parentId, [])), single-element arrays (inArray(c.parentId, [1])), and potentially string values if applicable.
🧪 Proposed edge case tests
+ it(`handles empty inArray values`, async () => {
+ const { collection: children, loadSubsetCalls } =
+ createChildrenCollectionWithTracking()
+
+ const liveQuery = createLiveQueryCollection((q) =>
+ q.from({ c: children }).where(({ c }) => inArray(c.parentId, [])),
+ )
+
+ await liveQuery.preload()
+
+ expect(loadSubsetCalls.length).toBeGreaterThan(0)
+
+ const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where)
+ const inFilter = filters.find((f) => f.operator === `in`)
+
+ expect(inFilter).toBeDefined()
+ expect(inFilter!.value).toEqual([])
+ })
+
+ it(`handles single-element inArray values`, async () => {
+ const { collection: children, loadSubsetCalls } =
+ createChildrenCollectionWithTracking()
+
+ const liveQuery = createLiveQueryCollection((q) =>
+ q.from({ c: children }).where(({ c }) => inArray(c.parentId, [2])),
+ )
+
+ await liveQuery.preload()
+
+ expect(loadSubsetCalls.length).toBeGreaterThan(0)
+
+ const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where)
+ const inFilter = filters.find((f) => f.operator === `in`)
+
+ expect(inFilter).toBeDefined()
+ expect(inFilter!.value).toEqual([2])
+ })As per coding guidelines: "Test corner cases including: empty arrays/sets, single-element collections."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/query/canonicalize-in-values.test.ts` around lines 16 - 92,
Add tests covering empty and single-element inArray edge cases inside the
existing describe(`canonicalize inArray value order`) block: use
createChildrenCollectionWithTracking() and createLiveQueryCollection((q) =>
q.from({ c: children }).where(({ c }) => inArray(c.parentId, []))) and similarly
with [1], call liveQuery.preload(), assert loadSubsetCalls.length > 0, then use
extractSimpleComparisons(loadSubsetCalls[0]!.where) to find the `in` filter and
assert its value equals [] for the empty case and [1] for the single-element
case; duplicate both assertions for the ordered/limited path (add .orderBy(({ c
}) => c.id).limit(2)) to cover the requestLimitedSnapshot branch.
Comment on lines
+47
to
+66
| it(`sorts inArray values in the predicate passed to loadSubset`, async () => { | ||
| const { collection: children, loadSubsetCalls } = | ||
| createChildrenCollectionWithTracking() | ||
|
|
||
| // Out-of-order literal — survives normalizeExpressionPaths unsorted, so | ||
| // without canonicalization loadSubset would receive [3, 1, 2]. | ||
| const liveQuery = createLiveQueryCollection((q) => | ||
| q.from({ c: children }).where(({ c }) => inArray(c.parentId, [3, 1, 2])), | ||
| ) | ||
|
|
||
| await liveQuery.preload() | ||
|
|
||
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | ||
|
|
||
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | ||
| const inFilter = filters.find((f) => f.operator === `in`) | ||
|
|
||
| expect(inFilter).toBeDefined() | ||
| expect(inFilter!.value).toEqual([1, 2, 3]) | ||
| }) |
Contributor
There was a problem hiding this comment.
Critical: Test data doesn't catch multi-digit number sorting bug.
The test uses single-digit values [3, 1, 2], which sort correctly even with JavaScript's default lexicographic sort. However, the implementation in expressions.ts uses .sort() without a comparator, which fails for multi-digit numbers (e.g., [10, 2, 1] would sort to [1, 10, 2] instead of [1, 2, 10]). Add a test case with multi-digit numeric IDs to catch this critical bug.
🧪 Proposed additional test case
+ it(`sorts multi-digit numeric inArray values correctly`, async () => {
+ const { collection: children, loadSubsetCalls } =
+ createChildrenCollectionWithTracking()
+
+ // Multi-digit numbers expose lexicographic vs numeric sort bugs
+ const liveQuery = createLiveQueryCollection((q) =>
+ q.from({ c: children }).where(({ c }) => inArray(c.parentId, [100, 20, 3])),
+ )
+
+ await liveQuery.preload()
+
+ expect(loadSubsetCalls.length).toBeGreaterThan(0)
+
+ const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where)
+ const inFilter = filters.find((f) => f.operator === `in`)
+
+ expect(inFilter).toBeDefined()
+ expect(inFilter!.value).toEqual([3, 20, 100])
+ })As per coding guidelines: "Test corner cases including: empty arrays/sets, single-element collections, undefined vs null values, resolved promises, async race conditions, and limit/offset edge cases."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it(`sorts inArray values in the predicate passed to loadSubset`, async () => { | |
| const { collection: children, loadSubsetCalls } = | |
| createChildrenCollectionWithTracking() | |
| // Out-of-order literal — survives normalizeExpressionPaths unsorted, so | |
| // without canonicalization loadSubset would receive [3, 1, 2]. | |
| const liveQuery = createLiveQueryCollection((q) => | |
| q.from({ c: children }).where(({ c }) => inArray(c.parentId, [3, 1, 2])), | |
| ) | |
| await liveQuery.preload() | |
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | |
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | |
| const inFilter = filters.find((f) => f.operator === `in`) | |
| expect(inFilter).toBeDefined() | |
| expect(inFilter!.value).toEqual([1, 2, 3]) | |
| }) | |
| it(`sorts inArray values in the predicate passed to loadSubset`, async () => { | |
| const { collection: children, loadSubsetCalls } = | |
| createChildrenCollectionWithTracking() | |
| // Out-of-order literal — survives normalizeExpressionPaths unsorted, so | |
| // without canonicalization loadSubset would receive [3, 1, 2]. | |
| const liveQuery = createLiveQueryCollection((q) => | |
| q.from({ c: children }).where(({ c }) => inArray(c.parentId, [3, 1, 2])), | |
| ) | |
| await liveQuery.preload() | |
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | |
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | |
| const inFilter = filters.find((f) => f.operator === `in`) | |
| expect(inFilter).toBeDefined() | |
| expect(inFilter!.value).toEqual([1, 2, 3]) | |
| }) | |
| it(`sorts multi-digit numeric inArray values correctly`, async () => { | |
| const { collection: children, loadSubsetCalls } = | |
| createChildrenCollectionWithTracking() | |
| // Multi-digit numbers expose lexicographic vs numeric sort bugs | |
| const liveQuery = createLiveQueryCollection((q) => | |
| q.from({ c: children }).where(({ c }) => inArray(c.parentId, [100, 20, 3])), | |
| ) | |
| await liveQuery.preload() | |
| expect(loadSubsetCalls.length).toBeGreaterThan(0) | |
| const filters = extractSimpleComparisons(loadSubsetCalls[0]!.where) | |
| const inFilter = filters.find((f) => f.operator === `in`) | |
| expect(inFilter).toBeDefined() | |
| expect(inFilter!.value).toEqual([3, 20, 100]) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/query/canonicalize-in-values.test.ts` around lines 47 - 66,
The test currently only uses single-digit values so it misses a numeric-sorting
bug; add a second assertion case that calls createLiveQueryCollection(... where
inArray(c.parentId, [10, 2, 1])) (or similar multi-digit set like [12,3,4]),
preload it, extract the in-filter from loadSubsetCalls (using
extractSimpleComparisons and find where operator === 'in') and assert that
inFilter!.value equals the numerically sorted array [1,2,10] (or [3,4,12]); this
will catch the bug in expressions.ts where .sort() is used without a numeric
comparator while still using the existing helpers loadSubset,
createLiveQueryCollection, and inArray.
inArray is set membership, but normalizeExpressionPaths only canonicalized ref paths, not value order, so the same value set in a different order produced a distinct serialized predicate (and loadSubset queryKey/cache key for adapters that key by it) and refetched identical data. normalizeExpressionPaths now also sorts in value arrays, and the join lazy-load predicate — previously built and combined without normalization — is routed through it so its keys are canonicalized too.
v-anton
force-pushed
the
fix/canonicalize-in-values
branch
from
75ec05f to
08b140b
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
1 participant
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.
Problem
inArrayis set membership, but the predicate handed to a collection'sloadSubsetwas not canonical in value order.normalizeExpressionPaths— the pass that canonicalizes predicates for subscriptions — only normalized ref paths, notinArrayvalue order. And the join lazy-load predicate (inArray(joinKey, [...]), built injoins.ts) was passed torequestSnapshotwithout going throughnormalizeExpressionPathsat all.So the same value set arriving in a different order — a join re-emitting its lazy-load keys across pipeline batches, or two subscriptions for the same data — serialized to a distinct predicate. Adapters that dedupe by serialized predicate / queryKey (e.g.
@tanstack/query-db-collection) then treat it as a new subset and refetch identical data (and miss CDN cache cells keyed on the request URL).Fix
normalizeExpressionPathsnow also canonicalizes set-membership value order — it sortsinvalue arrays.inis unordered, andisPredicateSubset/ index lookups already treat it order-independently, so sorting is safe.normalizeExpressionPathsinjoins.ts(using thetarget.aliasalready in scope), so its keys are canonicalized like every other predicate — fixing the inconsistency where it bypassed normalization entirely.Tests
tests/query/canonicalize-in-values.test.ts: an on-demand collection queried with an out-of-orderinArrayliteral assertsloadSubsetreceives the values sorted. Verified it fails without the change (loadSubsetgets[3, 1, 2]) and passes with it.@tanstack/dbsuite green (2372 tests).Changeset included (
patch).Summary by CodeRabbit
Bug Fixes
in-filter value ordering so identical value sets produce the same query/cache keys, including for join lazy-load queries—eliminates duplicate queries and improves cache consistency.Tests
infilter values are consistently sorted in query requests.Documentation
inpredicates are canonicalized by sorting their value lists.