diff --git a/.changeset/fix-getkey-collision.md b/.changeset/fix-getkey-collision.md new file mode 100644 index 000000000..3ef4ad751 --- /dev/null +++ b/.changeset/fix-getkey-collision.md @@ -0,0 +1,5 @@ +--- +'@tanstack/db': patch +--- + +fix: avoid DuplicateKeySyncError in join live queries when custom getKey only considers the identity of one of the joined collections diff --git a/packages/db/src/query/live/collection-config-builder.ts b/packages/db/src/query/live/collection-config-builder.ts index 980d26d8c..72efb75d8 100644 --- a/packages/db/src/query/live/collection-config-builder.ts +++ b/packages/db/src/query/live/collection-config-builder.ts @@ -728,8 +728,37 @@ export class CollectionConfigBuilder< if (pendingChanges.size === 0) { return } + + let changesToApply = pendingChanges + + // When a custom getKey is provided, multiple D2 internal keys may map + // to the same user-visible key. Re-accumulate by custom key so that a + // retract + insert for the same logical row merges into an UPDATE + // instead of a separate DELETE and INSERT that can race. + if (this.config.getKey) { + const merged = new Map>() + for (const [, changes] of pendingChanges) { + const customKey = this.config.getKey(changes.value) + const existing = merged.get(customKey) + if (existing) { + existing.inserts += changes.inserts + existing.deletes += changes.deletes + // Keep the value from the insert side (the new value) + if (changes.inserts > 0) { + existing.value = changes.value + if (changes.orderByIndex !== undefined) { + existing.orderByIndex = changes.orderByIndex + } + } + } else { + merged.set(customKey, { ...changes }) + } + } + changesToApply = merged + } + begin() - pendingChanges.forEach(this.applyChanges.bind(this, config)) + changesToApply.forEach(this.applyChanges.bind(this, config)) commit() pendingChanges = new Map() } diff --git a/packages/db/tests/query/join.test.ts b/packages/db/tests/query/join.test.ts index 441be59bc..49751eaa9 100644 --- a/packages/db/tests/query/join.test.ts +++ b/packages/db/tests/query/join.test.ts @@ -11,7 +11,10 @@ import { or, } from '../../src/query/index.js' import { createCollection } from '../../src/collection/index.js' -import { mockSyncCollectionOptions } from '../utils.js' +import { + mockSyncCollectionOptions, + mockSyncCollectionOptionsNoInitialState, +} from '../utils.js' // Sample data types for join testing type User = { @@ -1900,6 +1903,127 @@ function createJoinTests(autoIndex: `off` | `eager`): void { { leftId: `l4`, right: undefined }, ]) }) + + // Regression test for https://github.com/TanStack/db/issues/677 + // When a custom getKey is provided to a left join live query and the right + // collection is populated after initial sync, the system should not throw + // DuplicateKeySyncError. The old row (with undefined right side) should be + // deleted before the new row (with populated right side) is inserted. + test(`left join with custom getKey should not throw DuplicateKeySyncError when right collection is populated after initial sync (autoIndex: ${autoIndex})`, () => { + type Player = { + name: string + club_id: string + position: string + } + + type Client = { + name: string + player: string + email: string + } + + type Balance = { + name: string + client: string + amount: number + } + + const samplePlayers: Array = [ + { name: `player1`, club_id: `club1`, position: `forward` }, + { name: `player2`, club_id: `club1`, position: `midfielder` }, + { name: `player3`, club_id: `club1`, position: `defender` }, + ] + + const sampleClients: Array = [ + { name: `client1`, player: `player1`, email: `client1@example.com` }, + { name: `client2`, player: `player2`, email: `client2@example.com` }, + { name: `client3`, player: `player3`, email: `client3@example.com` }, + ] + + const sampleBalances: Array = [ + { name: `balance1`, client: `client1`, amount: 1000 }, + { name: `balance2`, client: `client2`, amount: 2000 }, + { name: `balance3`, client: `client3`, amount: 1500 }, + ] + + const playersCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-players-getkey-collision-${autoIndex}`, + getKey: (player) => player.name, + initialData: samplePlayers, + autoIndex, + }), + ) + + const clientsCollection = createCollection( + mockSyncCollectionOptionsNoInitialState({ + id: `test-clients-getkey-collision-${autoIndex}`, + getKey: (client) => client.name, + autoIndex, + }), + ) + + const balancesCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-balances-getkey-collision-${autoIndex}`, + getKey: (balance) => balance.name, + initialData: sampleBalances, + autoIndex, + }), + ) + + const chainedJoinQuery = createLiveQueryCollection({ + startSync: true, + getKey: (r) => r.player_name, + query: (q) => + q + .from({ player: playersCollection }) + .join( + { client: clientsCollection }, + ({ client, player }) => eq(client.player, player.name), + `left`, + ) + .join( + { balance: balancesCollection }, + ({ balance, client }) => eq(balance.client, client?.name), + `left`, + ) + .select(({ player, client, balance }) => ({ + player_name: player.name, + client_name: client?.name, + balance_amount: balance?.amount, + })), + }) + + // Initial state: 3 players, no clients, so left join gives undefined for client and balance + expect(chainedJoinQuery.toArray).toHaveLength(3) + expect( + chainedJoinQuery.toArray.every((r) => r.client_name === undefined), + ).toBe(true) + expect( + chainedJoinQuery.toArray.every((r) => r.balance_amount === undefined), + ).toBe(true) + + // Populating the clients collection should not throw DuplicateKeySyncError. + // The IVM retracts old rows (key e.g. player1 with undefined client) and inserts + // new rows (key e.g. player1 with populated client). Since both map to the same + // custom getKey, deletes must be processed before inserts to avoid a collision. + clientsCollection.utils.begin() + sampleClients.forEach((client) => { + clientsCollection.utils.write({ type: `insert`, value: client }) + }) + clientsCollection.utils.commit() + clientsCollection.utils.markReady() + + // Should still have 3 results, now with client and balance data populated + expect(chainedJoinQuery.toArray).toHaveLength(3) + expect( + chainedJoinQuery.toArray.every((r) => r.client_name !== undefined), + ).toBe(true) + expect( + chainedJoinQuery.toArray.every((r) => r.balance_amount !== undefined), + ).toBe(true) + }) } describe(`Query JOIN Operations`, () => {