From 9b28da5a664cf66cbf5ffe35ac40eee931b874ec Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 10:58:26 +0200 Subject: [PATCH 01/13] fix: remove "NOT NULL" constraint from SQLite database --- lib/storage/providers/SQLiteStorage.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index aa91a900..ad36ca04 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -9,7 +9,7 @@ import _ from 'underscore'; const DB_NAME = 'OnyxDB'; const db = open({name: DB_NAME}); -db.execute('CREATE TABLE IF NOT EXISTS keyvaluepairs (record_key TEXT NOT NULL PRIMARY KEY , valueJSON JSON NOT NULL) WITHOUT ROWID;'); +db.execute('CREATE TABLE IF NOT EXISTS keyvaluepairs (record_key TEXT NOT NULL PRIMARY KEY , valueJSON JSON) WITHOUT ROWID;'); // All of the 3 pragmas below were suggested by SQLite team. // You can find more info about them here: https://www.sqlite.org/pragma.html From 3f780963d664bde569616ed3d63ce529a5feee6b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 11:22:52 +0200 Subject: [PATCH 02/13] fix: instead of removing "NOT NULL" constraint, delete nullish keys manually --- lib/storage/providers/SQLiteStorage.js | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index ad36ca04..4b14f31d 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -9,7 +9,7 @@ import _ from 'underscore'; const DB_NAME = 'OnyxDB'; const db = open({name: DB_NAME}); -db.execute('CREATE TABLE IF NOT EXISTS keyvaluepairs (record_key TEXT NOT NULL PRIMARY KEY , valueJSON JSON) WITHOUT ROWID;'); +db.execute('CREATE TABLE IF NOT EXISTS keyvaluepairs (record_key TEXT NOT NULL PRIMARY KEY , valueJSON JSON NOT NULL) WITHOUT ROWID;'); // All of the 3 pragmas below were suggested by SQLite team. // You can find more info about them here: https://www.sqlite.org/pragma.html @@ -88,11 +88,27 @@ const provider = { ON CONFLICT DO UPDATE SET valueJSON = JSON_PATCH(valueJSON, JSON(:value)); `; - const queryArguments = _.map(pairs, (pair) => { + + const queryArguments = []; + const keysToDelete = []; + _.forEach(pairs, (pair) => { + if (pair[1] == null) { + keysToDelete.push(pair[0]); + return; + } + const value = JSON.stringify(pair[1]); - return [pair[0], value]; + + queryArguments.push([pair[0], value]); }); - return db.executeBatchAsync([[query, queryArguments]]); + + const deletePromise = this.removeItems(keysToDelete); + const mergePromise = queryArguments.length !== 0 ? db.executeBatchAsync([[query, queryArguments]]) : Promise.resolve(); + + return Promise.all([ + mergePromise, + deletePromise, + ]).then(() => mergePromise); }, /** From 25896925047c9a160f360eebd817463bc9b0236f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 18:46:37 +0200 Subject: [PATCH 03/13] fix: use lodash instead of JS non-strict null check --- lib/storage/providers/SQLiteStorage.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 4b14f31d..474968cf 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -92,14 +92,13 @@ const provider = { const queryArguments = []; const keysToDelete = []; _.forEach(pairs, (pair) => { - if (pair[1] == null) { + const value = pair[1]; + if (_.isUndefined(value) || _.isNull(value)) { keysToDelete.push(pair[0]); return; } - const value = JSON.stringify(pair[1]); - - queryArguments.push([pair[0], value]); + queryArguments.push([pair[0], JSON.stringify(value)]); }); const deletePromise = this.removeItems(keysToDelete); From 68382fff6a0c73956024d3b2e8568bfdd1923019 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 19:11:24 +0200 Subject: [PATCH 04/13] fix: move key removal to Onyx layer --- lib/Onyx.js | 4 ++++ lib/storage/providers/SQLiteStorage.js | 19 +++---------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 8f9f099c..5cb2d71d 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1092,6 +1092,10 @@ function merge(key, changes) { // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge) let batchedChanges = applyMerge(undefined, mergeQueue[key]); + if (_.isUndefined(batchedChanges) || _.isNull(batchedChanges)) { + return remove(key); + } + // Clean up the write queue so we // don't apply these changes again delete mergeQueue[key]; diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 474968cf..d6b9806f 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -89,25 +89,12 @@ const provider = { SET valueJSON = JSON_PATCH(valueJSON, JSON(:value)); `; - const queryArguments = []; - const keysToDelete = []; - _.forEach(pairs, (pair) => { + const queryArguments = _.map(pairs, (pair) => { const value = pair[1]; - if (_.isUndefined(value) || _.isNull(value)) { - keysToDelete.push(pair[0]); - return; - } - - queryArguments.push([pair[0], JSON.stringify(value)]); + return [pair[0], JSON.stringify(value)]; }); - const deletePromise = this.removeItems(keysToDelete); - const mergePromise = queryArguments.length !== 0 ? db.executeBatchAsync([[query, queryArguments]]) : Promise.resolve(); - - return Promise.all([ - mergePromise, - deletePromise, - ]).then(() => mergePromise); + return db.executeBatchAsync([[query, queryArguments]]); }, /** From 91acf99aa99812be8d90a9968e1a1e9674b899b5 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 19:11:38 +0200 Subject: [PATCH 05/13] fix: improve how Onyx merges a batch of objects --- lib/Onyx.js | 17 ++++++++++++++--- lib/utils/countObjectsAtEnd.js | 11 +++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 lib/utils/countObjectsAtEnd.js diff --git a/lib/Onyx.js b/lib/Onyx.js index 5cb2d71d..f9d8dee1 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -9,6 +9,7 @@ import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; import DevTools from './DevTools'; +import countObjectsAtEndOfArray from './utils/countObjectsAtEnd'; // Method constants const METHOD = { @@ -962,7 +963,7 @@ function removeNullObjectValues(value) { * @returns {Promise} */ function set(key, value) { - if (_.isNull(value)) { + if (_.isUndefined(value) || _.isNull(value)) { return remove(key); } @@ -1044,11 +1045,21 @@ function applyMerge(existingValue, changes) { return lastChange; } - if (_.isObject(existingValue) || _.every(changes, _.isObject)) { + if (_.isObject(existingValue) || _.isObject(lastChange)) { + // We want to merge multiple object delates together + // If all of the changes are objects, we can simply merge all of them together + // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch + let numberOfObjectsAtEndOfArray; + if (_.every(changes, _.isObject)) { + numberOfObjectsAtEndOfArray = changes.length; + } else { + numberOfObjectsAtEndOfArray = countObjectsAtEndOfArray(changes); + } + // Object values are merged one after the other // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), + return _.reduce(changes.slice(-numberOfObjectsAtEndOfArray), (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } diff --git a/lib/utils/countObjectsAtEnd.js b/lib/utils/countObjectsAtEnd.js new file mode 100644 index 00000000..b53c988a --- /dev/null +++ b/lib/utils/countObjectsAtEnd.js @@ -0,0 +1,11 @@ +export default function countObjectsAtEndOfArray(array) { + let count = 0; + for (let i = array.length - 1; i >= 0; i--) { + if (typeof array[i] === 'object' && array[i] !== null) { + count++; + } else { + break; + } + } + return count; +} From e2dd1190a4c369c51fdb2c99db8acce3a9e87ad9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 11 Sep 2023 19:17:13 +0200 Subject: [PATCH 06/13] fix: move utils to "utils.js" --- lib/Onyx.js | 4 ++-- lib/{utils/countObjectsAtEnd.js => utils.js} | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) rename lib/{utils/countObjectsAtEnd.js => utils.js} (72%) diff --git a/lib/Onyx.js b/lib/Onyx.js index f9d8dee1..18013bee 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -9,7 +9,7 @@ import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; import DevTools from './DevTools'; -import countObjectsAtEndOfArray from './utils/countObjectsAtEnd'; +import Utils from './utils'; // Method constants const METHOD = { @@ -1053,7 +1053,7 @@ function applyMerge(existingValue, changes) { if (_.every(changes, _.isObject)) { numberOfObjectsAtEndOfArray = changes.length; } else { - numberOfObjectsAtEndOfArray = countObjectsAtEndOfArray(changes); + numberOfObjectsAtEndOfArray = Utils.countObjectsAtEndOfArray(changes); } // Object values are merged one after the other diff --git a/lib/utils/countObjectsAtEnd.js b/lib/utils.js similarity index 72% rename from lib/utils/countObjectsAtEnd.js rename to lib/utils.js index b53c988a..3d7618fe 100644 --- a/lib/utils/countObjectsAtEnd.js +++ b/lib/utils.js @@ -1,4 +1,4 @@ -export default function countObjectsAtEndOfArray(array) { +function countObjectsAtEndOfArray(array) { let count = 0; for (let i = array.length - 1; i >= 0; i--) { if (typeof array[i] === 'object' && array[i] !== null) { @@ -9,3 +9,5 @@ export default function countObjectsAtEndOfArray(array) { } return count; } + +export default {countObjectsAtEndOfArray}; From 2f15c1981a442190e3a82df2ab83068574344c11 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 10:29:22 +0200 Subject: [PATCH 07/13] fix: typo --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 18013bee..1b1afb0b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1046,7 +1046,7 @@ function applyMerge(existingValue, changes) { } if (_.isObject(existingValue) || _.isObject(lastChange)) { - // We want to merge multiple object delates together + // We want to merge multiple object deltas together // If all of the changes are objects, we can simply merge all of them together // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch let numberOfObjectsAtEndOfArray; From bf5035a6bea50a770cd3c0d81408016caf1be114 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 10:39:35 +0200 Subject: [PATCH 08/13] fix: don't remove keys when set/merge value is undefined --- lib/Onyx.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 1b1afb0b..329ea200 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -963,7 +963,7 @@ function removeNullObjectValues(value) { * @returns {Promise} */ function set(key, value) { - if (_.isUndefined(value) || _.isNull(value)) { + if (_.isNull(value)) { return remove(key); } @@ -1103,7 +1103,7 @@ function merge(key, changes) { // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge) let batchedChanges = applyMerge(undefined, mergeQueue[key]); - if (_.isUndefined(batchedChanges) || _.isNull(batchedChanges)) { + if (_.isNull(batchedChanges)) { return remove(key); } From cb1de9359f01de925cecc543cf1c71dd605c33ab Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 10:39:50 +0200 Subject: [PATCH 09/13] fix: don't allow setting undefined in SQLite (causes crash) --- lib/storage/providers/SQLiteStorage.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index d6b9806f..1e1ec0c2 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -89,10 +89,7 @@ const provider = { SET valueJSON = JSON_PATCH(valueJSON, JSON(:value)); `; - const queryArguments = _.map(pairs, (pair) => { - const value = pair[1]; - return [pair[0], JSON.stringify(value)]; - }); + const queryArguments = _.map(pairs, pair => [pair[0], _.isUndefined(pair[1]) ? null : pair[1]]); return db.executeBatchAsync([[query, queryArguments]]); }, From 00e2660ada79bd9e12591e8b873e3b33fdb76cd7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 17:22:05 +0200 Subject: [PATCH 10/13] fix: handle nullish values in a batch of changes have to reset the existing value --- lib/Onyx.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 60c925a0..173ad1c6 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1030,17 +1030,17 @@ function applyMerge(existingValue, changes) { // We want to merge multiple object deltas together // If all of the changes are objects, we can simply merge all of them together // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch - let numberOfObjectsAtEndOfArray; + let numberOfObjectsAtTheEnd; if (_.every(changes, _.isObject)) { - numberOfObjectsAtEndOfArray = changes.length; + numberOfObjectsAtTheEnd = changes.length; } else { - numberOfObjectsAtEndOfArray = Utils.countObjectsAtEndOfArray(changes); + numberOfObjectsAtTheEnd = Utils.countObjectsAtEndOfArray(changes); } // Object values are merged one after the other // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes.slice(-numberOfObjectsAtEndOfArray), (modifiedData, change) => fastMerge(modifiedData, change), + return _.reduce(changes.slice(-numberOfObjectsAtTheEnd), (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } @@ -1088,12 +1088,21 @@ function merge(key, changes) { return remove(key); } + // The batchedChanges is the result of all changes in the mergeQueue merged together. + // If the mergeQueue contains nullish values (anywhere) and last value(s), it means that the user + // intends to reset the existing value or remove the key + // If there are other changes queued afterwards, we cannot simply merge the remaining changes + // and merge it with the existingValue. Instead, we have to omit the existingValue and only set + // the batchedChanges after any nullish values. + // This only affects objects, because arrays and nullish values are always replaced by the newest value. + const omitExistingValue = _.some(mergeQueue[key], change => _.isUndefined(change) || _.isNull(change)); + // Clean up the write queue so we // don't apply these changes again delete mergeQueue[key]; // After that we merge the batched changes with the existing value - const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); + const modifiedData = Utils.removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. From 0f54a211fe4d4d2e72e7a5aac6771482b659fe35 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 18:29:35 +0200 Subject: [PATCH 11/13] fix: remove batched object merging improvements --- lib/Onyx.js | 30 +++--------------------------- lib/utils.js | 13 ------------- 2 files changed, 3 insertions(+), 40 deletions(-) delete mode 100644 lib/utils.js diff --git a/lib/Onyx.js b/lib/Onyx.js index 173ad1c6..c8ffcf82 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -8,7 +8,6 @@ import createDeferredTask from './createDeferredTask'; import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; -import Utils from './utils'; // Method constants const METHOD = { @@ -1026,21 +1025,11 @@ function applyMerge(existingValue, changes) { return lastChange; } - if (_.isObject(existingValue) || _.isObject(lastChange)) { - // We want to merge multiple object deltas together - // If all of the changes are objects, we can simply merge all of them together - // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch - let numberOfObjectsAtTheEnd; - if (_.every(changes, _.isObject)) { - numberOfObjectsAtTheEnd = changes.length; - } else { - numberOfObjectsAtTheEnd = Utils.countObjectsAtEndOfArray(changes); - } - + if (_.isObject(existingValue) || _.every(changes, _.isObject)) { // Object values are merged one after the other // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes.slice(-numberOfObjectsAtTheEnd), (modifiedData, change) => fastMerge(modifiedData, change), + return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } @@ -1084,25 +1073,12 @@ function merge(key, changes) { // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge) let batchedChanges = applyMerge(undefined, mergeQueue[key]); - if (_.isNull(batchedChanges)) { - return remove(key); - } - - // The batchedChanges is the result of all changes in the mergeQueue merged together. - // If the mergeQueue contains nullish values (anywhere) and last value(s), it means that the user - // intends to reset the existing value or remove the key - // If there are other changes queued afterwards, we cannot simply merge the remaining changes - // and merge it with the existingValue. Instead, we have to omit the existingValue and only set - // the batchedChanges after any nullish values. - // This only affects objects, because arrays and nullish values are always replaced by the newest value. - const omitExistingValue = _.some(mergeQueue[key], change => _.isUndefined(change) || _.isNull(change)); - // Clean up the write queue so we // don't apply these changes again delete mergeQueue[key]; // After that we merge the batched changes with the existing value - const modifiedData = Utils.removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); + const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. diff --git a/lib/utils.js b/lib/utils.js deleted file mode 100644 index 3d7618fe..00000000 --- a/lib/utils.js +++ /dev/null @@ -1,13 +0,0 @@ -function countObjectsAtEndOfArray(array) { - let count = 0; - for (let i = array.length - 1; i >= 0; i--) { - if (typeof array[i] === 'object' && array[i] !== null) { - count++; - } else { - break; - } - } - return count; -} - -export default {countObjectsAtEndOfArray}; From 2b7350aefa1ead2156fbbf878718afdf2300a1cf Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 13 Sep 2023 11:36:04 +0200 Subject: [PATCH 12/13] fix: filter out any undefined keypairs --- lib/storage/providers/SQLiteStorage.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 1e1ec0c2..7a20b66b 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -89,7 +89,7 @@ const provider = { SET valueJSON = JSON_PATCH(valueJSON, JSON(:value)); `; - const queryArguments = _.map(pairs, pair => [pair[0], _.isUndefined(pair[1]) ? null : pair[1]]); + const queryArguments = _.filter(pairs, pair => !_.isUndefined(pair[1])); return db.executeBatchAsync([[query, queryArguments]]); }, From 3f9a42c81ef7677a8116c79756dfe350547f1be6 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 14 Sep 2023 10:12:31 +0200 Subject: [PATCH 13/13] fix: stringify value in SQLite --- lib/storage/providers/SQLiteStorage.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 7a20b66b..880c3af9 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -89,7 +89,11 @@ const provider = { SET valueJSON = JSON_PATCH(valueJSON, JSON(:value)); `; - const queryArguments = _.filter(pairs, pair => !_.isUndefined(pair[1])); + const nonNullishPairs = _.filter(pairs, pair => !_.isUndefined(pair[1])); + const queryArguments = _.map(nonNullishPairs, (pair) => { + const value = JSON.stringify(pair[1]); + return [pair[0], value]; + }); return db.executeBatchAsync([[query, queryArguments]]); },