Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-getkey-collision.md
Original file line number Diff line number Diff line change
@@ -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
31 changes: 30 additions & 1 deletion packages/db/src/query/live/collection-config-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that this is only used when there is a getKey 👍

const merged = new Map<unknown, Changes<TResult>>()
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()
}
Expand Down
126 changes: 125 additions & 1 deletion packages/db/tests/query/join.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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<Player> = [
{ name: `player1`, club_id: `club1`, position: `forward` },
{ name: `player2`, club_id: `club1`, position: `midfielder` },
{ name: `player3`, club_id: `club1`, position: `defender` },
]

const sampleClients: Array<Client> = [
{ 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<Balance> = [
{ name: `balance1`, client: `client1`, amount: 1000 },
{ name: `balance2`, client: `client2`, amount: 2000 },
{ name: `balance3`, client: `client3`, amount: 1500 },
]

const playersCollection = createCollection(
mockSyncCollectionOptions<Player>({
id: `test-players-getkey-collision-${autoIndex}`,
getKey: (player) => player.name,
initialData: samplePlayers,
autoIndex,
}),
)

const clientsCollection = createCollection(
mockSyncCollectionOptionsNoInitialState<Client>({
id: `test-clients-getkey-collision-${autoIndex}`,
getKey: (client) => client.name,
autoIndex,
}),
)

const balancesCollection = createCollection(
mockSyncCollectionOptions<Balance>({
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`, () => {
Expand Down
Loading