From 9b433566a3de36279722461142bf9f98b8416170 Mon Sep 17 00:00:00 2001 From: David Fahlander Date: Mon, 14 Oct 2024 11:55:01 +0200 Subject: [PATCH 1/4] Repro of #2058 --- test/tests-live-query.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/tests-live-query.js b/test/tests-live-query.js index b9cc3bdc9..0489abe24 100644 --- a/test/tests-live-query.js +++ b/test/tests-live-query.js @@ -828,4 +828,21 @@ promisedTest("Issue 2067: useLiveQuery does not update when multiple items are d db.items.where('id').above(0).delete(); items = await promise; deepEqual(items, [{id: 0},{id: -10}], "Should have deleted items where id > 0"); + tester.subscription.unsubscribe(); +}); + +promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { + const tester = liveQueryUnitTester(()=>db.items.toArray()); + let items = await tester.waitNextValue(); + deepEqual(items, [{id:1},{id:2},{id:3}], "Initial items are correct"); + let promise = tester.waitNextValue(); + await db.items.bulkAdd([ + {id:3}, // This one won't be added (constraint violation) + {id:88} // This one will be added + ]).catch(error => { + equal(error.failuresByPos[0].name, "ConstraintError", "Expected constraint error for the first operation"); + }); + items = await promise; + deepEqual(items, [{id:1},{id:2},{id:3},{id:88}], "The livequery emitted correct result after bulk operation"); + tester.subscription.unsubscribe(); }); From d1d6a092c8ee42f760befbcfcbb53d5514a48c2a Mon Sep 17 00:00:00 2001 From: David Fahlander Date: Mon, 14 Oct 2024 15:19:24 +0200 Subject: [PATCH 2/4] Added more code coverage for the repro of 2058 --- test/tests-live-query.js | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/test/tests-live-query.js b/test/tests-live-query.js index 0489abe24..673602ca9 100644 --- a/test/tests-live-query.js +++ b/test/tests-live-query.js @@ -42,7 +42,7 @@ class Signal { promise = new Promise(resolve => this.resolve = resolve); } -function liveQueryUnitTester(lq) { +function liveQueryUnitTester(lq, {graceTime}={graceTime: 0}) { let currentVal; let currentError = null; let signal = ()=>{}; @@ -52,6 +52,7 @@ function liveQueryUnitTester(lq) { ++querying; try { currentVal = await lq(); + console.log("Emitted", currentVal); currentError = null; } catch (error) { currentError = error; @@ -63,7 +64,7 @@ function liveQueryUnitTester(lq) { else signal(currentVal); } - }, 0); + }, graceTime); }).subscribe(x => x); return { waitNextValue(timeout=500) { @@ -832,7 +833,9 @@ promisedTest("Issue 2067: useLiveQuery does not update when multiple items are d }); promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { - const tester = liveQueryUnitTester(()=>db.items.toArray()); + const tester = liveQueryUnitTester( + ()=>db.items.toArray() + ); let items = await tester.waitNextValue(); deepEqual(items, [{id:1},{id:2},{id:3}], "Initial items are correct"); let promise = tester.waitNextValue(); @@ -844,5 +847,35 @@ promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { }); items = await promise; deepEqual(items, [{id:1},{id:2},{id:3},{id:88}], "The livequery emitted correct result after bulk operation"); + // Now making sure we go through a different code path (where the number of items > 50 in cache-middleware.ts) + const itemsToAdd = new Array(51) + .fill({id: 1}, 0, 40) // Positions 0..40 is constraint violations agains existing data + themselves + .fill({id:99}, 40, 49) // Positions 40 is new value but 41...49 is constraint violation of pos 40. + .fill({id:100}, 49, 50) // Position 50 is ok. + .fill({id:101}, 50) // Position 51 is ok. + + await db.items.bulkAdd(itemsToAdd).catch(error => { + deepEqual(Object.keys(error.failuresByPos).map(Number),[ + 0,1,2,3,4,5,6,7,8,9, + 10,11,12,13,14,15,16,17,18,19, + 20,21,22,23,24,25,26,27,28,29, + 30,31,32,33,34,35,36,37,38,39, + 41,42,43,44,45,46,47,48 + ] + , "We get errors on the expected positions"); + }); + + console.log("Before await promise", performance.now()); + items = await tester.waitNextValue(); + console.log("After await promise", performance.now()); + deepEqual(items, [ + {id:1}, + {id:2}, + {id:3}, + {id:88}, + {id:99}, + {id:100}, + {id:101} + ], "Correct state after trying to add these half baked entries"); tester.subscription.unsubscribe(); }); From 5d74ab282276cef7e6c2ce823e8fab75e09f7ed7 Mon Sep 17 00:00:00 2001 From: David Fahlander Date: Mon, 14 Oct 2024 15:21:19 +0200 Subject: [PATCH 3/4] Filter away duplicates from optimistic results. Resolves #2058 --- src/live-query/cache/apply-optimistic-ops.ts | 32 ++++++++++++++++---- src/live-query/cache/cache-middleware.ts | 1 + 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/live-query/cache/apply-optimistic-ops.ts b/src/live-query/cache/apply-optimistic-ops.ts index 6088ff2fa..505eaa913 100644 --- a/src/live-query/cache/apply-optimistic-ops.ts +++ b/src/live-query/cache/apply-optimistic-ops.ts @@ -48,14 +48,30 @@ export function applyOptimisticOps( } } switch (op.type) { - case 'add': + case 'add': { + const existingKeys = new RangeSet().addKeys( + req.values ? result.map((v) => extractPrimKey(v)) : result + ); + modifedResult = result.concat( req.values - ? includedValues - : includedValues.map((v) => extractPrimKey(v)) + ? includedValues.filter((v) => { + const key = extractPrimKey(v); + if (existingKeys.hasKey(key)) return false; + existingKeys.addKey(key); + return true; + }) + : includedValues + .map((v) => extractPrimKey(v)) + .filter((k) => { + if (existingKeys.hasKey(k)) return false; + existingKeys.addKey(k); + return true; + }) ); break; - case 'put': + } + case 'put': { const keySet = new RangeSet().addKeys( op.values.map((v) => extractPrimKey(v)) ); @@ -71,16 +87,20 @@ export function applyOptimisticOps( : includedValues.map((v) => extractPrimKey(v)) ); break; + } case 'delete': const keysToDelete = new RangeSet().addKeys(op.keys); modifedResult = result.filter( - (item) => !keysToDelete.hasKey(req.values ? extractPrimKey(item) : item) + (item) => + !keysToDelete.hasKey(req.values ? extractPrimKey(item) : item) ); break; case 'deleteRange': const range = op.range; - modifedResult = result.filter((item) => !isWithinRange(extractPrimKey(item), range)); + modifedResult = result.filter( + (item) => !isWithinRange(extractPrimKey(item), range) + ); break; } return modifedResult; diff --git a/src/live-query/cache/cache-middleware.ts b/src/live-query/cache/cache-middleware.ts index a86131a79..61dda0dda 100644 --- a/src/live-query/cache/cache-middleware.ts +++ b/src/live-query/cache/cache-middleware.ts @@ -168,6 +168,7 @@ export const cacheMiddleware: Middleware = { const reqWithResolvedKeys = { ...req, values: req.values.map((value, i) => { + if (res.failures[i]) return value; // No need to rewrite a failing value const valueWithKey = primKey.keyPath?.includes('.') ? deepClone(value) : { From 4f95efc1663d43fb19a69162f1263869bf330e65 Mon Sep 17 00:00:00 2001 From: David Fahlander Date: Mon, 14 Oct 2024 15:35:04 +0200 Subject: [PATCH 4/4] Real repro of #2058 - turned out to work as expected --- test/tests-live-query.js | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/test/tests-live-query.js b/test/tests-live-query.js index 673602ca9..9c7e74d04 100644 --- a/test/tests-live-query.js +++ b/test/tests-live-query.js @@ -14,7 +14,8 @@ db.version(2).stores({ outbound: "++,name", friends: "++id, name, age", multiEntry: "id, *tags", - issue1946: "++id, [name+age], [name+age+id]" + issue1946: "++id, [name+age], [name+age+id]", + issue2058: "id, &[a+b]" }); db.on('populate', ()=> { @@ -832,7 +833,7 @@ promisedTest("Issue 2067: useLiveQuery does not update when multiple items are d tester.subscription.unsubscribe(); }); -promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { +promisedTest("Issue 2058 - related but with bulkAdd and constraint error on duplicate primary keys.", async () => { const tester = liveQueryUnitTester( ()=>db.items.toArray() ); @@ -879,3 +880,35 @@ promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { ], "Correct state after trying to add these half baked entries"); tester.subscription.unsubscribe(); }); + +promisedTest("Issue 2058: Cache error after bulkPut failures", async () => { + const tester = liveQueryUnitTester( + ()=>db.issue2058.toArray(), + {graceTime: 10} + ); + await db.issue2058.bulkAdd([ + {id:"1.1",a:1,b:1}, + {id:"1.2",a:1,b:2}, + {id:"2.1",a:2,b:1}, + ]); + let items = await tester.waitNextValue(); + deepEqual(items, [ + {id:"1.1",a:1,b:1}, + {id:"1.2",a:1,b:2}, + {id:"2.1",a:2,b:1} + ], "Initial items are correct"); + await db.issue2058.bulkPut([ + {id:"2.2",a:2,b:2}, + {id:"foo", a: 1, b: 1} + ]).catch(error => { + equal(error.failuresByPos[1].name, "ConstraintError", "Expected constraint error for the first operation"); + }); + items = await tester.waitNextValue(); + deepEqual(items, [ + {id:"1.1",a:1,b:1}, + {id:"1.2",a:1,b:2}, + {id:"2.1",a:2,b:1}, + {id:"2.2",a:2,b:2} + ], "The livequery emitted correct result after bulk operation"); +}); +