From cab2a401dbdba13f29e279d56f16f7e4cd1c8b2f Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Tue, 26 Aug 2025 12:06:47 +0100 Subject: [PATCH] OL2 (fix) Simplify idb (#5174) * Simplify idb usage and collapse the class * fix tests * fmt --- packages/objectloader2/src/core/interfaces.ts | 6 +- .../src/core/objectLoader2.spec.ts | 2 +- .../objectloader2/src/core/objectLoader2.ts | 2 +- .../src/core/objectLoader2Factory.ts | 2 +- .../src/core/stages/ItemStore.ts | 196 ------------------ .../src/core/stages/cacheWriter.spec.ts | 8 +- .../src/core/stages/cacheWriter.ts | 2 +- .../src/core/stages/indexedDatabase.spec.ts | 12 +- .../src/core/stages/indexedDatabase.ts | 179 +++++++++++++--- .../core/stages/memory/memoryDatabase.spec.ts | 12 +- .../src/core/stages/memory/memoryDatabase.ts | 6 +- 11 files changed, 180 insertions(+), 247 deletions(-) delete mode 100644 packages/objectloader2/src/core/stages/ItemStore.ts diff --git a/packages/objectloader2/src/core/interfaces.ts b/packages/objectloader2/src/core/interfaces.ts index add683213..7f7152ea9 100644 --- a/packages/objectloader2/src/core/interfaces.ts +++ b/packages/objectloader2/src/core/interfaces.ts @@ -12,7 +12,7 @@ export interface Downloader extends Queue { } export interface Database { - getAll(keys: string[]): Promise<(Item | undefined)[]> - saveBatch(params: { batch: Item[] }): Promise - disposeAsync(): Promise + getAll(ids: string[]): Promise<(Item | undefined)[]> + putAll(batch: Item[]): Promise + dispose(): void } diff --git a/packages/objectloader2/src/core/objectLoader2.spec.ts b/packages/objectloader2/src/core/objectLoader2.spec.ts index 87b2649f9..996c65b95 100644 --- a/packages/objectloader2/src/core/objectLoader2.spec.ts +++ b/packages/objectloader2/src/core/objectLoader2.spec.ts @@ -1,7 +1,7 @@ import { describe, test, expect } from 'vitest' import { Base, Item } from '../types/types.js' import { ObjectLoader2 } from './objectLoader2.js' -import IndexedDatabase from './stages/indexedDatabase.js' +import { IndexedDatabase } from './stages/indexedDatabase.js' import { IDBFactory, IDBKeyRange } from 'fake-indexeddb' import { MemoryDatabase } from './stages/memory/memoryDatabase.js' import { MemoryDownloader } from './stages/memory/memoryDownloader.js' diff --git a/packages/objectloader2/src/core/objectLoader2.ts b/packages/objectloader2/src/core/objectLoader2.ts index a5ee12b29..61b9c5b7b 100644 --- a/packages/objectloader2/src/core/objectLoader2.ts +++ b/packages/objectloader2/src/core/objectLoader2.ts @@ -147,7 +147,7 @@ export class ObjectLoader2 { } } if (!this.#isRootStored) { - await this.#database.saveBatch({ batch: [rootItem] }) + await this.#database.putAll([rootItem]) this.#isRootStored = true } } diff --git a/packages/objectloader2/src/core/objectLoader2Factory.ts b/packages/objectloader2/src/core/objectLoader2Factory.ts index fc5f950d3..e7e11f06a 100644 --- a/packages/objectloader2/src/core/objectLoader2Factory.ts +++ b/packages/objectloader2/src/core/objectLoader2Factory.ts @@ -1,7 +1,7 @@ import { CustomLogger, getFeatureFlag, ObjectLoader2Flags } from '../types/functions.js' import { Base } from '../types/types.js' import { ObjectLoader2 } from './objectLoader2.js' -import IndexedDatabase from './stages/indexedDatabase.js' +import { IndexedDatabase } from './stages/indexedDatabase.js' import { MemoryDatabase } from './stages/memory/memoryDatabase.js' import { MemoryDownloader } from './stages/memory/memoryDownloader.js' import ServerDownloader from './stages/serverDownloader.js' diff --git a/packages/objectloader2/src/core/stages/ItemStore.ts b/packages/objectloader2/src/core/stages/ItemStore.ts deleted file mode 100644 index de57d193e..000000000 --- a/packages/objectloader2/src/core/stages/ItemStore.ts +++ /dev/null @@ -1,196 +0,0 @@ -/* eslint-disable @typescript-eslint/no-base-to-string */ -/* eslint-disable @typescript-eslint/prefer-promise-reject-errors */ -/* eslint-disable @typescript-eslint/no-explicit-any */ -/* eslint-disable @typescript-eslint/no-unsafe-function-type */ -import { isSafari } from '@speckle/shared' -import { Item } from '../../types/types.js' - -/** - * A wrapper class for IndexedDB to simplify common database operations. - */ -export interface ItemStoreOptions { - indexedDB?: IDBFactory - keyRange?: { - bound: Function - lowerBound: Function - upperBound: Function - } -} -export class ItemStore { - #options: ItemStoreOptions - - #db: IDBDatabase | undefined = undefined - readonly #dbName: string - readonly #storeName: string - - constructor(options: ItemStoreOptions, dbName: string, storeName: string) { - this.#options = options - this.#dbName = dbName - this.#storeName = storeName - } - - /** - * Initializes the database connection and creates the object store if needed. - * This must be called before any other database operations. - */ - async init(): Promise { - if (this.#db) return - await this.#safariFix() - return this.#openDatabase() - } - - /** - * Opens the database, and if there's an error, deletes the database and tries again. - */ - async #openDatabase(): Promise { - const idb = this.#options.indexedDB ?? indexedDB - - return new Promise((resolve, reject) => { - const request = idb.open(this.#dbName, 1) - - request.onerror = (): any => { - console.warn( - `Failed to open database: ${this.#dbName}, deleting and trying again` - ) - // Delete the database and try again - const deleteRequest = idb.deleteDatabase(this.#dbName) - deleteRequest.onsuccess = (): any => { - // Try opening again after deletion - void this.#openDatabase().then(resolve).catch(reject) - } - deleteRequest.onerror = (): any => { - reject(`Failed to delete and reopen database: ${this.#dbName}`) - } - } - - request.onupgradeneeded = (event): any => { - const db = (event.target as IDBOpenDBRequest).result - if (db.objectStoreNames.contains(this.#storeName)) { - db.deleteObjectStore(this.#storeName) - } - db.createObjectStore(this.#storeName, { keyPath: 'baseId' }) - } - - request.onsuccess = (event): any => { - this.#db = (event.target as IDBOpenDBRequest).result - resolve() - } - }) - } - - #getDB(): IDBDatabase { - if (!this.#db) { - throw new Error('Database not initialized. Call init() first.') - } - return this.#db - } - - /** - * Fixes a Safari bug where IndexedDB requests get lost and never resolve - invoke before you use IndexedDB - * @link Credits and more info: https://github.com/jakearchibald/safari-14-idb-fix - */ - async #safariFix(): Promise { - // No point putting other browsers or older versions of Safari through this mess. - if (!isSafari() || !this.#options.indexedDB?.databases) return Promise.resolve() - - let intervalId: ReturnType - - return new Promise((resolve: () => void) => { - const tryIdb = (): Promise | undefined => - this.#options.indexedDB?.databases().finally(resolve) - intervalId = setInterval(() => { - void tryIdb() - }, 100) - void tryIdb() - }).finally(() => clearInterval(intervalId)) - } - - /** - * Inserts or updates an array of items in a single transaction. - * @param data The array of items to insert. - */ - bulkInsert(data: Item[]): Promise { - return new Promise((resolve, reject) => { - try { - const transaction = this.#getDB().transaction(this.#storeName, 'readwrite', { - durability: 'relaxed' - }) - const store = transaction.objectStore(this.#storeName) - - transaction.onerror = (): any => { - reject(`Transaction error: ${transaction.error}`) - } - transaction.oncomplete = (): any => { - resolve() - } - - data.forEach((item) => store.put(item)) - } catch (error) { - reject(error) - } - }) - } - - /** - * Retrieves an array of items from the object store based on their IDs. - * @param ids The array of IDs to retrieve. - */ - bulkGet(ids: string[]): Promise<(Item | undefined)[]> { - return new Promise((resolve, reject) => { - if (ids.length === 0) { - return resolve([]) - } - try { - const transaction = this.#getDB().transaction(this.#storeName, 'readonly', { - durability: 'relaxed' - }) - const store = transaction.objectStore(this.#storeName) - const promises: Promise[] = [] - - for (const id of ids) { - promises.push( - new Promise((resolveGet, rejectGet) => { - const request = store.get(id) - request.onerror = (): void => - rejectGet(`Request error for id ${id}: ${request.error}`) - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - request.onsuccess = (): void => resolveGet(request.result) - }) - ) - } - - Promise.all(promises) - .then((results) => { - resolve(results) - }) - .catch(reject) - } catch (error) { - reject(error) - } - }) - } - - /** - * Retrieves all items from the object store. - */ - getAll(): Promise { - return new Promise((resolve, reject) => { - try { - const transaction = this.#getDB().transaction(this.#storeName, 'readonly') - const store = transaction.objectStore(this.#storeName) - const request = store.getAll() - - request.onerror = (): any => reject(`Request error: ${request.error}`) - request.onsuccess = (): any => resolve(request.result) - } catch (error) { - reject(error) - } - }) - } - - close(): void { - if (!this.#db) return - this.#db.close() - this.#db = undefined - } -} diff --git a/packages/objectloader2/src/core/stages/cacheWriter.spec.ts b/packages/objectloader2/src/core/stages/cacheWriter.spec.ts index cc82f330a..ea79a5782 100644 --- a/packages/objectloader2/src/core/stages/cacheWriter.spec.ts +++ b/packages/objectloader2/src/core/stages/cacheWriter.spec.ts @@ -15,13 +15,13 @@ class MockDatabase implements Database { return Promise.resolve([]) } - saveBatch({ batch }: { batch: Item[] }): Promise { + putAll(batch: Item[]): Promise { this.savedItems.push(...batch) return Promise.resolve() } - disposeAsync(): Promise { - return Promise.resolve() + dispose(): void { + this.savedItems = [] } } @@ -150,7 +150,7 @@ describe('CacheWriter', () => { }) it('should process items in batches according to maxCacheWriteSize', async () => { - const spy = vi.spyOn(database, 'saveBatch') + const spy = vi.spyOn(database, 'putAll') const smallBatchOptions: CacheOptions = { ...options, maxCacheWriteSize: 2, // Set small batch size diff --git a/packages/objectloader2/src/core/stages/cacheWriter.ts b/packages/objectloader2/src/core/stages/cacheWriter.ts index 8909d5697..5502f63f1 100644 --- a/packages/objectloader2/src/core/stages/cacheWriter.ts +++ b/packages/objectloader2/src/core/stages/cacheWriter.ts @@ -45,7 +45,7 @@ export class CacheWriter implements Queue { async writeAll(items: Item[]): Promise { const start = performance.now() - await this.#database.saveBatch({ batch: items }) + await this.#database.putAll(items) this.#logger( `writeBatch: wrote ${items.length}, time ${ performance.now() - start diff --git a/packages/objectloader2/src/core/stages/indexedDatabase.spec.ts b/packages/objectloader2/src/core/stages/indexedDatabase.spec.ts index 49acf7a4d..9069b0f05 100644 --- a/packages/objectloader2/src/core/stages/indexedDatabase.spec.ts +++ b/packages/objectloader2/src/core/stages/indexedDatabase.spec.ts @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest' import { IDBFactory, IDBKeyRange } from 'fake-indexeddb' import { Base, Item } from '../../types/types.js' -import IndexedDatabase, { IndexedDatabaseOptions } from './indexedDatabase.js' +import { IndexedDatabase, IndexedDatabaseOptions } from './indexedDatabase.js' // Mock Item const defaultItem = (id: string): Item => ({ @@ -19,19 +19,19 @@ describe('IndexedDatabase', () => { db = new IndexedDatabase(options) }) - afterEach(async () => { - await db.disposeAsync() + afterEach(() => { + db.dispose() }) it('should add and get multiple items', async () => { const items = [defaultItem('id1'), defaultItem('id2')] - await db.saveBatch({ batch: items }) + await db.putAll(items) const result = await db.getAll(['id1', 'id2']) expect(result).toMatchSnapshot() expect(result).toEqual(items) }) - it('should dispose without error', async () => { - await expect(db.disposeAsync()).resolves.not.toThrow() + it('should dispose without error', () => { + expect(() => db.dispose()).not.toThrow() }) }) diff --git a/packages/objectloader2/src/core/stages/indexedDatabase.ts b/packages/objectloader2/src/core/stages/indexedDatabase.ts index 7dfb39d37..e56958571 100644 --- a/packages/objectloader2/src/core/stages/indexedDatabase.ts +++ b/packages/objectloader2/src/core/stages/indexedDatabase.ts @@ -1,9 +1,14 @@ +/* eslint-disable @typescript-eslint/no-base-to-string */ +/* eslint-disable @typescript-eslint/prefer-promise-reject-errors */ +/* eslint-disable @typescript-eslint/no-explicit-any */ /* eslint-disable @typescript-eslint/no-unsafe-function-type */ +import { isSafari } from '@speckle/shared' import { Item } from '../../types/types.js' import { Database } from '../interfaces.js' -import BatchingQueue from '../../queues/batchingQueue.js' -import { ItemStore } from './ItemStore.js' +/** + * A wrapper class for IndexedDB to simplify common database operations. + */ export interface IndexedDatabaseOptions { indexedDB?: IDBFactory keyRange?: { @@ -12,38 +17,162 @@ export interface IndexedDatabaseOptions { upperBound: Function } } - -export default class IndexedDatabase implements Database { +export class IndexedDatabase implements Database { #options: IndexedDatabaseOptions - #cacheDB: ItemStore - #writeQueue: BatchingQueue | undefined + + #db: IDBDatabase | undefined = undefined + readonly #dbName: string = 'speckle-cache' + readonly #storeName: string = 'cache' constructor(options: IndexedDatabaseOptions) { this.#options = options - this.#cacheDB = new ItemStore( - { - indexedDB: this.#options.indexedDB, - keyRange: this.#options.keyRange - }, - 'speckle-cache', - 'cache' - ) } - async getAll(keys: string[]): Promise<(Item | undefined)[]> { - await this.#cacheDB.init() - return await this.#cacheDB.bulkGet(keys) + /** + * Initializes the database connection and creates the object store if needed. + * This must be called before any other database operations. + */ + async init(): Promise { + if (this.#db) return + await this.#safariFix() + return this.#openDatabase() } - async saveBatch(params: { batch: Item[] }): Promise { - await this.#cacheDB.init() - const { batch } = params - await this.#cacheDB.bulkInsert(batch) + /** + * Opens the database, and if there's an error, deletes the database and tries again. + */ + async #openDatabase(): Promise { + const idb = this.#options.indexedDB ?? indexedDB + + return new Promise((resolve, reject) => { + const request = idb.open(this.#dbName, 1) + + request.onerror = (): any => { + console.warn( + `Failed to open database: ${this.#dbName}, deleting and trying again` + ) + // Delete the database and try again + const deleteRequest = idb.deleteDatabase(this.#dbName) + deleteRequest.onsuccess = (): any => { + // Try opening again after deletion + void this.#openDatabase().then(resolve).catch(reject) + } + deleteRequest.onerror = (): any => { + reject(`Failed to delete and reopen database: ${this.#dbName}`) + } + } + + request.onupgradeneeded = (event): any => { + const db = (event.target as IDBOpenDBRequest).result + if (db.objectStoreNames.contains(this.#storeName)) { + db.deleteObjectStore(this.#storeName) + } + db.createObjectStore(this.#storeName, { keyPath: 'baseId' }) + } + + request.onsuccess = (event): any => { + this.#db = (event.target as IDBOpenDBRequest).result + resolve() + } + }) } - async disposeAsync(): Promise { - await this.#writeQueue?.disposeAsync() - this.#writeQueue = undefined - this.#cacheDB.close() + #getDB(): IDBDatabase { + if (!this.#db) { + throw new Error('Database not initialized. Call init() first.') + } + return this.#db + } + + /** + * Fixes a Safari bug where IndexedDB requests get lost and never resolve - invoke before you use IndexedDB + * @link Credits and more info: https://github.com/jakearchibald/safari-14-idb-fix + */ + async #safariFix(): Promise { + // No point putting other browsers or older versions of Safari through this mess. + if (!isSafari() || !this.#options.indexedDB?.databases) return Promise.resolve() + + let intervalId: ReturnType + + return new Promise((resolve: () => void) => { + const tryIdb = (): Promise | undefined => + this.#options.indexedDB?.databases().finally(resolve) + intervalId = setInterval(() => { + void tryIdb() + }, 100) + void tryIdb() + }).finally(() => clearInterval(intervalId)) + } + + /** + * Inserts or updates an array of items in a single transaction. + * @param data The array of items to insert. + */ + async putAll(data: Item[]): Promise { + await this.init() // Ensure the database is initialized + return new Promise((resolve, reject) => { + try { + const transaction = this.#getDB().transaction(this.#storeName, 'readwrite', { + durability: 'relaxed' + }) + const store = transaction.objectStore(this.#storeName) + + transaction.onerror = (): any => { + reject(`Transaction error: ${transaction.error}`) + } + transaction.oncomplete = (): any => { + resolve() + } + + data.forEach((item) => store.put(item)) + } catch (error) { + reject(error) + } + }) + } + + /** + * Retrieves an array of items from the object store based on their IDs. + * @param ids The array of IDs to retrieve. + */ + async getAll(ids: string[]): Promise<(Item | undefined)[]> { + await this.init() // Ensure the database is initialized + return new Promise((resolve, reject) => { + if (ids.length === 0) { + return resolve([]) + } + try { + const transaction = this.#getDB().transaction(this.#storeName, 'readonly', { + durability: 'relaxed' + }) + const store = transaction.objectStore(this.#storeName) + const promises: Promise[] = [] + + for (const id of ids) { + promises.push( + new Promise((resolveGet, rejectGet) => { + const request = store.get(id) + request.onerror = (): void => + rejectGet(`Request error for id ${id}: ${request.error}`) + // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + request.onsuccess = (): void => resolveGet(request.result) + }) + ) + } + + Promise.all(promises) + .then((results) => { + resolve(results) + }) + .catch(reject) + } catch (error) { + reject(error) + } + }) + } + dispose(): void { + if (!this.#db) return + this.#db.close() + this.#db = undefined } } diff --git a/packages/objectloader2/src/core/stages/memory/memoryDatabase.spec.ts b/packages/objectloader2/src/core/stages/memory/memoryDatabase.spec.ts index a365a06ad..f186dbf84 100644 --- a/packages/objectloader2/src/core/stages/memory/memoryDatabase.spec.ts +++ b/packages/objectloader2/src/core/stages/memory/memoryDatabase.spec.ts @@ -21,14 +21,14 @@ describe('MemoryDatabase', () => { it('should add and retrieve a single item', async () => { const item = makeItem('id1') - await db.saveBatch({ batch: [item] }) + await db.putAll([item]) const result = await db.getAll(['id1']) expect(result).toEqual([item]) }) it('should add and retrieve multiple items', async () => { const items = [makeItem('id1'), makeItem('id2', 'baz')] - await db.saveBatch({ batch: items }) + await db.putAll(items) const result = await db.getAll(['id1', 'id2']) expect(result).toEqual(items) }) @@ -36,13 +36,13 @@ describe('MemoryDatabase', () => { it('should overwrite items with the same key', async () => { const item1 = makeItem('id1', 'foo') const item2 = makeItem('id1', 'bar') - await db.saveBatch({ batch: [item1] }) - await db.saveBatch({ batch: [item2] }) + await db.putAll([item1]) + await db.putAll([item2]) const result = await db.getAll(['id1']) expect(result).toEqual([item2]) }) - it('disposeAsync should resolve', async () => { - await expect(db.disposeAsync()).resolves.not.toThrow() + it('dispose should not throw', () => { + db.dispose() }) }) diff --git a/packages/objectloader2/src/core/stages/memory/memoryDatabase.ts b/packages/objectloader2/src/core/stages/memory/memoryDatabase.ts index cb2cbc19e..41b46251f 100644 --- a/packages/objectloader2/src/core/stages/memory/memoryDatabase.ts +++ b/packages/objectloader2/src/core/stages/memory/memoryDatabase.ts @@ -22,7 +22,7 @@ export class MemoryDatabase implements Database { return Promise.resolve(found) } - saveBatch({ batch }: { batch: Item[] }): Promise { + putAll(batch: Item[]): Promise { for (const item of batch) { if (!item.baseId || !item.base) { throw new Error('Item must have a baseId and base') @@ -32,7 +32,7 @@ export class MemoryDatabase implements Database { return Promise.resolve() } - disposeAsync(): Promise { - return Promise.resolve() + dispose(): void { + this.items.clear() } }