From 34ed018ae29975a07d40f1cb6b6932a908b2126a Mon Sep 17 00:00:00 2001 From: dfahlander Date: Fri, 17 Nov 2023 01:54:14 +0100 Subject: [PATCH 1/2] Repro of #1823 --- test/tests-live-query.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/tests-live-query.js b/test/tests-live-query.js index 1ca2237cc..9f0adcaf8 100644 --- a/test/tests-live-query.js +++ b/test/tests-live-query.js @@ -239,6 +239,27 @@ promisedTest("subscribe and error occur", async ()=> { subscription.unsubscribe(); }); +promisedTest("optimistic updates that eventually fail must be reverted (Issue #1823)", async ()=>{ + const log = []; + let subscription = liveQuery( + ()=>db.items.toArray() + ).subscribe({ + next: result => { + log.push(result); + console.log("optimistic result (from #1823 test)", result); + }, + }); + + await db.transaction('rw', db.items, async ()=>{ + await db.items.add({id: 1, iWillFail: true}).catch(()=>{}); + }); + // Wait for a successful readonly transaction to complete after the write transaction. + // This will make sure that the liveQuery has been updated with the final result. + await db.transaction('r', db.items, ()=>db.items.toArray()); + subscription.unsubscribe(); + deepEqual(log.at(-1), [{id: 1},{id:2},{id:3}], "Last log entry contains the correct result. There might be optimistic updates before though."); +}); + /* Use cases to cover: Queries From f3c1e2619dc10aa85e8604e1072d6f3b8bb32697 Mon Sep 17 00:00:00 2001 From: dfahlander Date: Wed, 22 Nov 2023 12:01:42 +0100 Subject: [PATCH 2/2] Resolves #1823 --- ...adjust-optimistic-request-from-failures.ts | 38 +++++++++++++++++++ src/live-query/cache/cache-middleware.ts | 19 ++++++++-- test/tests-live-query.js | 12 +++++- 3 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 src/live-query/cache/adjust-optimistic-request-from-failures.ts diff --git a/src/live-query/cache/adjust-optimistic-request-from-failures.ts b/src/live-query/cache/adjust-optimistic-request-from-failures.ts new file mode 100644 index 000000000..c812e7a30 --- /dev/null +++ b/src/live-query/cache/adjust-optimistic-request-from-failures.ts @@ -0,0 +1,38 @@ +import { delArrayItem, isArray } from '../../functions/utils'; +import { TblQueryCache } from '../../public/types/cache'; +import { + DBCoreMutateRequest, + DBCoreMutateResponse, +} from '../../public/types/dbcore'; + +export function adjustOptimisticFromFailures( + tblCache: TblQueryCache, + req: DBCoreMutateRequest, + res: DBCoreMutateResponse +): DBCoreMutateRequest { + if (res.numFailures === 0) return req; + if (req.type === 'deleteRange') { + // numFailures > 0 means the deleteRange operation failed in its whole. + return null; + } + + const numBulkOps = req.keys + ? req.keys.length + : 'values' in req && req.values + ? req.values.length + : 1; + if (res.numFailures === numBulkOps) { + // Same number of failures as the number of ops. This means that all ops failed. + return null; + } + + const clone: DBCoreMutateRequest = { ...req }; + + if (isArray(clone.keys)) { + clone.keys = clone.keys.filter((_, i) => !(i in res.failures)); + } + if ('values' in clone && isArray(clone.values)) { + clone.values = clone.values.filter((_, i) => !(i in res.failures)); + } + return clone; +} diff --git a/src/live-query/cache/cache-middleware.ts b/src/live-query/cache/cache-middleware.ts index 996ca4c79..a4d2cbb25 100644 --- a/src/live-query/cache/cache-middleware.ts +++ b/src/live-query/cache/cache-middleware.ts @@ -4,10 +4,11 @@ import { deepClone, delArrayItem, setByKeyPath } from '../../functions/utils'; import DexiePromise, { PSD } from '../../helpers/promise'; import { ObservabilitySet } from '../../public/types/db-events'; import { - DBCore, DBCoreQueryRequest, + DBCore, DBCoreMutateRequest, DBCoreMutateResponse, DBCoreQueryRequest, DBCoreQueryResponse } from '../../public/types/dbcore'; import { Middleware } from '../../public/types/middleware'; +import { adjustOptimisticFromFailures } from './adjust-optimistic-request-from-failures'; import { applyOptimisticOps } from './apply-optimistic-ops'; import { cache } from './cache'; import { findCompatibleQuery } from './find-compatible-query'; @@ -127,7 +128,7 @@ export const cacheMiddleware: Middleware = { const primKey = downTable.schema.primaryKey; const tableMW = { ...downTable, - mutate(req) { + mutate(req: DBCoreMutateRequest): Promise { if ( primKey.outbound || // Non-inbound tables are harded to apply optimistic updates on because we can't know primary key of results PSD.trans.db._options.cache === 'disabled' // User has opted-out from caching @@ -157,7 +158,8 @@ export const cacheMiddleware: Middleware = { return valueWithKey; }) }; - tblCache.optimisticOps.push(reqWithResolvedKeys); + const adjustedReq = adjustOptimisticFromFailures(tblCache, reqWithResolvedKeys, res); + tblCache.optimisticOps.push(adjustedReq); // Signal subscribers after the observability middleware has complemented req.mutatedParts with the new keys. // We must queue the task so that we get the req.mutatedParts updated by observability middleware first. // If we refactor the dependency between observability middleware and this middleware we might not need to queue the task. @@ -168,6 +170,17 @@ export const cacheMiddleware: Middleware = { tblCache.optimisticOps.push(req); // Signal subscribers that there are mutated parts signalSubscribersLazily(req.mutatedParts); + promise.then((res) => { + if (res.numFailures > 0) { + // In case the operation failed, we need to remove it from the optimisticOps array. + delArrayItem(tblCache.optimisticOps, req); + const adjustedReq = adjustOptimisticFromFailures(tblCache, req, res); + if (adjustedReq) { + tblCache.optimisticOps.push(adjustedReq); + } + signalSubscribersLazily(req.mutatedParts); // Signal the rolling back of the operation. + } + }); promise.catch(()=> { // In case the operation failed, we need to remove it from the optimisticOps array. delArrayItem(tblCache.optimisticOps, req); diff --git a/test/tests-live-query.js b/test/tests-live-query.js index 9f0adcaf8..5c9ec8ef5 100644 --- a/test/tests-live-query.js +++ b/test/tests-live-query.js @@ -251,13 +251,21 @@ promisedTest("optimistic updates that eventually fail must be reverted (Issue #1 }); await db.transaction('rw', db.items, async ()=>{ - await db.items.add({id: 1, iWillFail: true}).catch(()=>{}); + // Simple test a catched failing operation + await db.items.add( + {id: 1, iWillFail: true} // Contraint error (key 1 already exists) + ).catch(()=>{}); + // Test another code path in adjustOptimisticFromFailures() where some operations succeed and some not. + await db.items.bulkAdd([ + {id: 2, iWillFail: true}, // Constraint error (key 2 already exists) + {id: 99, iWillSucceed: true} + ]).catch(()=>{}); }); // Wait for a successful readonly transaction to complete after the write transaction. // This will make sure that the liveQuery has been updated with the final result. await db.transaction('r', db.items, ()=>db.items.toArray()); subscription.unsubscribe(); - deepEqual(log.at(-1), [{id: 1},{id:2},{id:3}], "Last log entry contains the correct result. There might be optimistic updates before though."); + deepEqual(log.at(-1), [{id: 1},{id:2},{id:3},{id: 99, iWillSucceed: true}], "Last log entry contains the correct result. There might be optimistic updates before though."); }); /* Use cases to cover: