Skip to content

Commit 179d666

Browse files
fix(db): complete DeduplicatedLoadSubset follow-up coverage (#1352)
* fix(db): complete DeduplicatedLoadSubset follow-up fixes Finish the DeduplicatedLoadSubset follow-up by deduping in-flight narrowed requests against the original request intent and cloning stored pagination options defensively. Add regressions covering repeated unfiltered loads, filtered-then-unfiltered coverage, in-flight load-all dedupe, and caller mutation of stored limited query options. Made-with: Cursor * ci: apply automated fixes * chore: add changeset for subset dedupe follow-up Add a short patch changeset for the DeduplicatedLoadSubset follow-up fixes so PR #1352 can version correctly. Made-with: Cursor * ci: apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 parent 1270156 commit 179d666

File tree

3 files changed

+256
-7
lines changed

3 files changed

+256
-7
lines changed

.changeset/fuzzy-ties-dream.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@tanstack/db': patch
3+
---
4+
5+
Fix `loadSubset` dedupe follow-up edge cases and add regression coverage.

packages/db/src/query/subset-dedupe.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -126,10 +126,8 @@ export class DeduplicatedLoadSubset {
126126
return prom
127127
}
128128

129-
// Not fully covered by existing data — load the missing subset.
130-
// We need two clones: trackingOptions preserves the original predicate for
131-
// accurate tracking (e.g., where=undefined means "all data"), while loadOptions
132-
// may be narrowed with a difference expression for the actual backend request.
129+
// Preserve the original request for tracking and in-flight dedupe, but allow
130+
// the backend request to be narrowed to only the missing subset.
133131
const trackingOptions = cloneOptions(options)
134132
const loadOptions = cloneOptions(options)
135133
if (this.unlimitedWhere !== undefined && options.limit === undefined) {
@@ -147,7 +145,6 @@ export class DeduplicatedLoadSubset {
147145

148146
// Handle both sync (true) and async (Promise<void>) return values
149147
if (resultPromise === true) {
150-
// Sync return - update tracking with the original predicate
151148
this.updateTracking(trackingOptions)
152149
return true
153150
} else {
@@ -159,7 +156,7 @@ export class DeduplicatedLoadSubset {
159156

160157
// We need to create a reference to the in-flight entry so we can remove it later
161158
const inflightEntry = {
162-
options: loadOptions, // Store load options for subset matching of in-flight requests
159+
options: trackingOptions,
163160
promise: resultPromise
164161
.then((result) => {
165162
// Only update tracking if this request is still from the current generation
@@ -238,5 +235,12 @@ export class DeduplicatedLoadSubset {
238235
* would reflect the mutated values rather than what was actually loaded.
239236
*/
240237
export function cloneOptions(options: LoadSubsetOptions): LoadSubsetOptions {
241-
return { ...options }
238+
return {
239+
...options,
240+
orderBy: options.orderBy?.map((clause) => ({
241+
...clause,
242+
compareOptions: { ...clause.compareOptions },
243+
})),
244+
cursor: options.cursor ? { ...options.cursor } : undefined,
245+
}
242246
}

packages/db/tests/query/subset-dedupe.test.ts

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,194 @@ describe(`createDeduplicatedLoadSubset`, () => {
602602
expect(callCount).toBe(2)
603603
})
604604

605+
describe(`hasLoadedAllData after loading filtered + unfiltered data`, () => {
606+
it(`should set hasLoadedAllData after a filtered load followed by an unfiltered load`, async () => {
607+
let callCount = 0
608+
const calls: Array<LoadSubsetOptions> = []
609+
const mockLoadSubset = (options: LoadSubsetOptions) => {
610+
callCount++
611+
calls.push(cloneOptions(options))
612+
return Promise.resolve()
613+
}
614+
615+
const deduplicated = new DeduplicatedLoadSubset({
616+
loadSubset: mockLoadSubset,
617+
})
618+
619+
await deduplicated.loadSubset({
620+
where: inOp(ref(`task_id`), [`id1`, `id2`, `id3`]),
621+
})
622+
expect(callCount).toBe(1)
623+
624+
await deduplicated.loadSubset({})
625+
expect(callCount).toBe(2)
626+
expect(calls[1]).toEqual({
627+
where: not(inOp(ref(`task_id`), [`id1`, `id2`, `id3`])),
628+
})
629+
630+
const result = await deduplicated.loadSubset({})
631+
expect(result).toBe(true)
632+
expect(callCount).toBe(2)
633+
})
634+
635+
it(`should set hasLoadedAllData after a filtered load followed by an unfiltered load (with eq)`, async () => {
636+
let callCount = 0
637+
const mockLoadSubset = () => {
638+
callCount++
639+
return Promise.resolve()
640+
}
641+
642+
const deduplicated = new DeduplicatedLoadSubset({
643+
loadSubset: mockLoadSubset,
644+
})
645+
646+
await deduplicated.loadSubset({
647+
where: eq(ref(`task_id`), val(`single-id`)),
648+
})
649+
expect(callCount).toBe(1)
650+
651+
await deduplicated.loadSubset({})
652+
expect(callCount).toBe(2)
653+
654+
const result1 = await deduplicated.loadSubset({})
655+
expect(result1).toBe(true)
656+
expect(callCount).toBe(2)
657+
658+
const result2 = await deduplicated.loadSubset({
659+
where: eq(ref(`task_id`), val(`other-id`)),
660+
})
661+
expect(result2).toBe(true)
662+
expect(callCount).toBe(2)
663+
})
664+
665+
it(`should not produce exponentially growing predicates on repeated unfiltered loads`, async () => {
666+
let callCount = 0
667+
const calls: Array<LoadSubsetOptions> = []
668+
const mockLoadSubset = (options: LoadSubsetOptions) => {
669+
callCount++
670+
calls.push(cloneOptions(options))
671+
return Promise.resolve()
672+
}
673+
674+
const deduplicated = new DeduplicatedLoadSubset({
675+
loadSubset: mockLoadSubset,
676+
})
677+
678+
await deduplicated.loadSubset({
679+
where: inOp(ref(`task_id`), [`id1`, `id2`, `id3`]),
680+
})
681+
expect(callCount).toBe(1)
682+
683+
await deduplicated.loadSubset({})
684+
expect(callCount).toBe(2)
685+
686+
const rounds: Array<{ round: number; whereSize: number }> = []
687+
for (let i = 0; i < 10; i++) {
688+
const result = await deduplicated.loadSubset({})
689+
if (result !== true) {
690+
const whereJson = JSON.stringify(calls[calls.length - 1]?.where)
691+
rounds.push({ round: i + 1, whereSize: whereJson.length })
692+
}
693+
}
694+
695+
expect(callCount).toBe(2)
696+
expect(rounds).toEqual([])
697+
})
698+
})
699+
700+
it(`should mark all data as loaded after a narrowed all-data request`, async () => {
701+
const calls: Array<LoadSubsetOptions> = []
702+
const mockLoadSubset = (options: LoadSubsetOptions) => {
703+
calls.push(cloneOptions(options))
704+
return Promise.resolve()
705+
}
706+
707+
const deduplicated = new DeduplicatedLoadSubset({
708+
loadSubset: mockLoadSubset,
709+
})
710+
711+
await deduplicated.loadSubset({
712+
where: eq(ref(`task_id`), val(`uuid-1`)),
713+
})
714+
await deduplicated.loadSubset({
715+
where: eq(ref(`task_id`), val(`uuid-2`)),
716+
})
717+
718+
await deduplicated.loadSubset({})
719+
720+
expect(calls[2]).toEqual({
721+
where: not(inOp(ref(`task_id`), [`uuid-1`, `uuid-2`])),
722+
})
723+
724+
expect((deduplicated as any).hasLoadedAllData).toBe(true)
725+
expect((deduplicated as any).unlimitedWhere).toBeUndefined()
726+
})
727+
728+
it(`should not keep issuing increasingly nested all-data predicates`, async () => {
729+
const calls: Array<LoadSubsetOptions> = []
730+
const mockLoadSubset = (options: LoadSubsetOptions) => {
731+
calls.push(cloneOptions(options))
732+
return Promise.resolve()
733+
}
734+
735+
const deduplicated = new DeduplicatedLoadSubset({
736+
loadSubset: mockLoadSubset,
737+
})
738+
739+
await deduplicated.loadSubset({
740+
where: eq(ref(`task_id`), val(`uuid-1`)),
741+
})
742+
await deduplicated.loadSubset({
743+
where: eq(ref(`task_id`), val(`uuid-2`)),
744+
})
745+
746+
await deduplicated.loadSubset({})
747+
await deduplicated.loadSubset({})
748+
749+
expect(calls[3]).toBeUndefined()
750+
})
751+
752+
it(`should deduplicate identical all-data requests while a narrowed all-data request is in flight`, async () => {
753+
let resolveAllDataLoad: (() => void) | undefined
754+
let callCount = 0
755+
const calls: Array<LoadSubsetOptions> = []
756+
const allDataLoadPromise = new Promise<void>((resolve) => {
757+
resolveAllDataLoad = resolve
758+
})
759+
760+
const mockLoadSubset = (options: LoadSubsetOptions) => {
761+
callCount++
762+
calls.push(cloneOptions(options))
763+
764+
if (callCount === 2) {
765+
return allDataLoadPromise
766+
}
767+
768+
return Promise.resolve()
769+
}
770+
771+
const deduplicated = new DeduplicatedLoadSubset({
772+
loadSubset: mockLoadSubset,
773+
})
774+
775+
await deduplicated.loadSubset({
776+
where: eq(ref(`task_id`), val(`uuid-1`)),
777+
})
778+
779+
const firstAllDataLoad = deduplicated.loadSubset({})
780+
const secondAllDataLoad = deduplicated.loadSubset({})
781+
782+
expect(callCount).toBe(2)
783+
expect(calls[1]).toEqual({
784+
where: not(eq(ref(`task_id`), val(`uuid-1`))),
785+
})
786+
expect(secondAllDataLoad).toBe(firstAllDataLoad)
787+
788+
resolveAllDataLoad?.()
789+
await firstAllDataLoad
790+
await secondAllDataLoad
791+
})
792+
605793
it(`should not produce unbounded WHERE expressions when loading all data after eq accumulation`, async () => {
606794
// This test reproduces the production bug where accumulating many eq predicates
607795
// and then loading all data (no WHERE clause) caused unboundedly growing
@@ -1005,5 +1193,57 @@ describe(`createDeduplicatedLoadSubset`, () => {
10051193
expect(result).toBe(true)
10061194
expect(callCount).toBe(1)
10071195
})
1196+
1197+
it(`should not let caller mutations change stored limited call orderBy`, async () => {
1198+
let callCount = 0
1199+
const mockLoadSubset = () => {
1200+
callCount++
1201+
return Promise.resolve()
1202+
}
1203+
1204+
const deduplicated = new DeduplicatedLoadSubset({
1205+
loadSubset: mockLoadSubset,
1206+
})
1207+
1208+
const mutableOrderBy: OrderBy = [
1209+
{
1210+
expression: ref(`created_at`),
1211+
compareOptions: {
1212+
direction: `asc`,
1213+
nulls: `last`,
1214+
stringSort: `lexical`,
1215+
},
1216+
},
1217+
]
1218+
1219+
await deduplicated.loadSubset({
1220+
where: eq(ref(`status`), val(`active`)),
1221+
orderBy: mutableOrderBy,
1222+
limit: 10,
1223+
})
1224+
expect(callCount).toBe(1)
1225+
1226+
mutableOrderBy[0]!.compareOptions.direction = `desc`
1227+
1228+
const originalOrderBy: OrderBy = [
1229+
{
1230+
expression: ref(`created_at`),
1231+
compareOptions: {
1232+
direction: `asc`,
1233+
nulls: `last`,
1234+
stringSort: `lexical`,
1235+
},
1236+
},
1237+
]
1238+
1239+
const result = await deduplicated.loadSubset({
1240+
where: eq(ref(`status`), val(`active`)),
1241+
orderBy: originalOrderBy,
1242+
limit: 5,
1243+
})
1244+
1245+
expect(result).toBe(true)
1246+
expect(callCount).toBe(1)
1247+
})
10081248
})
10091249
})

0 commit comments

Comments
 (0)