From 848659665811abe610da2ad32970a9645665965a Mon Sep 17 00:00:00 2001 From: Schottkyc137 <45085299+Schottkyc137@users.noreply.github.com> Date: Fri, 8 Apr 2022 22:36:34 +0200 Subject: [PATCH] refactor: Implement batch save for demo data closes #1168 Co-authored-by: Simon <33730997+TheSlimvReal@users.noreply.github.com> Co-authored-by: Simon --- src/app/core/database/database.ts | 8 ++ src/app/core/database/pouch-database.spec.ts | 97 ++++++++++++--- src/app/core/database/pouch-database.ts | 41 ++++++- src/app/core/demo-data/demo-data.service.ts | 10 +- .../core/entity/entity-mapper.service.spec.ts | 115 +++++++++++++----- src/app/core/entity/entity-mapper.service.ts | 24 +++- 6 files changed, 228 insertions(+), 67 deletions(-) diff --git a/src/app/core/database/database.ts b/src/app/core/database/database.ts index ed897660ee..bc22da9fc7 100644 --- a/src/app/core/database/database.ts +++ b/src/app/core/database/database.ts @@ -48,6 +48,14 @@ export abstract class Database { */ abstract put(object: any, forceUpdate?: boolean): Promise; + /** + * Save a bunch of documents at once to the database + * @param objects The documents to be saved + * @param forceUpdate (Optional) Whether conflicts should be ignored and existing conflicting documents forcefully overwritten. + * @returns array holding success responses or errors depending on the success of the operation + */ + abstract putAll(objects: any[], forceUpdate?: boolean): Promise; + /** * Delete a document from the database * @param object The document to be deleted (usually this object must at least contain the _id and _rev) diff --git a/src/app/core/database/pouch-database.spec.ts b/src/app/core/database/pouch-database.spec.ts index 2cb24e1e0f..92a8e28357 100644 --- a/src/app/core/database/pouch-database.spec.ts +++ b/src/app/core/database/pouch-database.spec.ts @@ -157,22 +157,19 @@ describe("PouchDatabase tests", () => { it("saveDatabaseIndex updates existing index", async () => { const testIndex = { _id: "_design/test_index", views: { a: {}, b: {} } }; - const existingIndex = { - _id: "_design/test_index", - _rev: "01", + await database.put({ + _id: testIndex._id, views: { a: {} }, - }; - // @ts-ignore - const pouchDB = database._pouchDB; - spyOn(pouchDB, "get").and.resolveTo(existingIndex); + }); + const existingIndex = await database.get(testIndex._id); spyOn(database, "put").and.resolveTo(); const spyOnQuery = spyOn(database, "query").and.resolveTo(); await database.saveDatabaseIndex(testIndex); expect(database.put).toHaveBeenCalledWith({ _id: testIndex._id, - views: testIndex.views, _rev: existingIndex._rev, + views: testIndex.views, }); // expect all indices to be queried @@ -181,33 +178,24 @@ describe("PouchDatabase tests", () => { ["test_index/a", { key: "1" }], ["test_index/b", { key: "1" }], ]); - - // reset pouchDB function - pouchDB.get.and.callThrough(); }); it("saveDatabaseIndex does not update unchanged index", async () => { const testIndex = { _id: "_design/test_index", views: { a: {}, b: {} } }; const existingIndex = { _id: "_design/test_index", - _rev: "01", views: testIndex.views, }; - // @ts-ignore - const pouchDB = database._pouchDB; - spyOn(pouchDB, "get").and.resolveTo(existingIndex); + await database.put(existingIndex); spyOn(database, "put").and.resolveTo(); await database.saveDatabaseIndex(testIndex); expect(database.put).not.toHaveBeenCalled(); - - // reset pouchDB function - pouchDB.get.and.callThrough(); }); - it("query simply calls through to query", async () => { + it("query simply calls through to pouchDB query", async () => { const testQuery = "testquery"; - const testQueryResults = { rows: [] }; + const testQueryResults = { rows: [] } as any; // @ts-ignore const pouchDB = database._pouchDB; spyOn(pouchDB, "query").and.resolveTo(testQueryResults); @@ -215,4 +203,73 @@ describe("PouchDatabase tests", () => { expect(result).toEqual(testQueryResults); expect(pouchDB.query).toHaveBeenCalledWith(testQuery, {}); }); + + it("writes all the docs to the database with putAll", async () => { + await database.putAll([ + { + _id: "5", + name: "The Grinch", + }, + { + _id: "8", + name: "Santa Claus", + }, + ]); + + await expectAsync(database.get("5")).toBeResolvedTo( + jasmine.objectContaining({ + _id: "5", + name: "The Grinch", + }) + ); + await expectAsync(database.get("8")).toBeResolvedTo( + jasmine.objectContaining({ + _id: "8", + name: "Santa Claus", + }) + ); + }); + + it("Throws errors for each conflict individually", async () => { + const resolveConflictSpy = spyOn(database, "resolveConflict"); + const conflictError = new Error(); + resolveConflictSpy.and.rejectWith(conflictError); + await database.put({ + _id: "3", + name: "Rudolph, the Red-Nosed Reindeer", + }); + const dataWithConflicts = [ + { + _id: "3", + name: "Rudolph, the Pink-Nosed Reindeer", + _rev: "1-invalid-rev", + }, + { + _id: "4", + name: "Dasher", + }, + { + _id: "5", + name: "Dancer", + }, + ]; + + const results = await database.putAll(dataWithConflicts); + expect(resolveConflictSpy.calls.allArgs()).toEqual([ + [ + { + _id: "3", + name: "Rudolph, the Pink-Nosed Reindeer", + _rev: "1-invalid-rev", + }, + false, + jasmine.objectContaining({ status: 409 }), + ], + ]); + expect(results).toEqual([ + conflictError, + jasmine.objectContaining({ id: "4", ok: true }), + jasmine.objectContaining({ id: "5", ok: true }), + ]); + }); }); diff --git a/src/app/core/database/pouch-database.ts b/src/app/core/database/pouch-database.ts index 93db3a4194..652569d3a8 100644 --- a/src/app/core/database/pouch-database.ts +++ b/src/app/core/database/pouch-database.ts @@ -66,7 +66,10 @@ export class PouchDatabase extends Database { * @param _pouchDB An (initialized) PouchDB database instance from the PouchDB library. * @param loggingService The LoggingService instance of the app to log and report problems. */ - constructor(private _pouchDB: any, private loggingService: LoggingService) { + constructor( + private _pouchDB: PouchDB.Database, + private loggingService: LoggingService + ) { super(); } @@ -125,13 +128,12 @@ export class PouchDatabase extends Database { * @param object The document to be saved * @param forceOverwrite (Optional) Whether conflicts should be ignored and an existing conflicting document forcefully overwritten. */ - put(object: any, forceOverwrite?: boolean): Promise { - const options: any = {}; + put(object: any, forceOverwrite = false): Promise { if (forceOverwrite) { object._rev = undefined; } - return this._pouchDB.put(object, options).catch((err) => { + return this._pouchDB.put(object).catch((err) => { if (err.status === 409) { return this.resolveConflict(object, forceOverwrite, err); } else { @@ -140,6 +142,33 @@ export class PouchDatabase extends Database { }); } + /** + * Save an array of documents to the database + * @param objects the documents to be saved + * @param forceOverwrite whether conflicting versions should be overwritten + * @returns array holding `{ ok: true, ... }` or `{ error: true, ... }` depending on whether the document could be saved + */ + async putAll(objects: any[], forceOverwrite = false): Promise { + if (forceOverwrite) { + objects.forEach((obj) => (obj._rev = undefined)); + } + + const results = await this._pouchDB.bulkDocs(objects); + + for (let i = 0; i < results.length; i++) { + // Check if document update conflicts happened in the request + const result = results[i] as PouchDB.Core.Error; + if (result.status === 409) { + results[i] = await this.resolveConflict( + objects.find((obj) => obj._id === result.id), + false, + result + ).catch((e) => e); + } + } + return results; + } + /** * Delete a document from the database * (see {@link Database}) @@ -241,8 +270,8 @@ export class PouchDatabase extends Database { */ private async resolveConflict( newObject: any, - overwriteChanges: boolean, - existingError: any + overwriteChanges = false, + existingError: any = {} ): Promise { const existingObject = await this.get(newObject._id); const resolvedObject = this.mergeObjects(existingObject, newObject); diff --git a/src/app/core/demo-data/demo-data.service.ts b/src/app/core/demo-data/demo-data.service.ts index dd99135be1..60ddf09bc9 100644 --- a/src/app/core/demo-data/demo-data.service.ts +++ b/src/app/core/demo-data/demo-data.service.ts @@ -24,7 +24,6 @@ import { } from "@angular/core"; import { DemoDataGenerator } from "./demo-data-generator"; import { EntityMapperService } from "../entity/entity-mapper.service"; -import { LoggingService } from "../logging/logging.service"; import { User } from "../user/user"; /** @@ -60,7 +59,6 @@ export class DemoDataService { constructor( private entityMapper: EntityMapperService, - private loggingService: LoggingService, private injector: Injector, private config: DemoDataServiceConfig ) { @@ -92,13 +90,7 @@ export class DemoDataService { // save the generated data for (const generator of this.dataGenerators) { - for (const entity of generator.entities) { - try { - await this.entityMapper.save(entity); - } catch (e) { - this.loggingService.warn(e); - } - } + await this.entityMapper.saveAll(generator.entities); } } diff --git a/src/app/core/entity/entity-mapper.service.spec.ts b/src/app/core/entity/entity-mapper.service.spec.ts index c565f56a0c..7335ac9399 100644 --- a/src/app/core/entity/entity-mapper.service.spec.ts +++ b/src/app/core/entity/entity-mapper.service.spec.ts @@ -20,7 +20,7 @@ import { Entity } from "./model/entity"; import { EntitySchemaService } from "./schema/entity-schema.service"; import { waitForAsync } from "@angular/core/testing"; import { PouchDatabase } from "../database/pouch-database"; -import { entityRegistry } from "./database-entity.decorator"; +import { DatabaseEntity, entityRegistry } from "./database-entity.decorator"; describe("EntityMapperService", () => { let entityMapper: EntityMapperService; @@ -178,55 +178,108 @@ describe("EntityMapperService", () => { expect(loadedByFullId._rev).toBe(loadedByEntityId._rev); }); - it("publishes updates to any listeners", (done) => { + it("publishes updates to any listeners", () => { const testId = "t1"; - receiveUpdatesAndTestTypeAndId(done, undefined, testId); - const testEntity = new Entity(testId); entityMapper .save(testEntity, true) .then(() => entityMapper.remove(testEntity)); - }); - it("publishes when an existing entity is updated", (done) => { - receiveUpdatesAndTestTypeAndId(done, "update", existingEntity.entityId); + return receiveUpdatesAndTestTypeAndId(undefined, testId); + }); + it("publishes when an existing entity is updated", () => { entityMapper - .load(Entity, existingEntity.entityId) - .then((loadedEntity) => entityMapper.save(loadedEntity)); - }); + .load(Entity, existingEntity.entityId) + .then((loadedEntity) => entityMapper.save(loadedEntity)); - it("publishes when an existing entity is deleted", (done) => { - receiveUpdatesAndTestTypeAndId(done, "remove", existingEntity.entityId); + return receiveUpdatesAndTestTypeAndId("update", existingEntity.entityId); + }); + it("publishes when an existing entity is deleted", () => { entityMapper - .load(Entity, existingEntity.entityId) - .then((loadedEntity) => entityMapper.remove(loadedEntity)); + .load(Entity, existingEntity.entityId) + .then((loadedEntity) => entityMapper.remove(loadedEntity)); + + return receiveUpdatesAndTestTypeAndId("remove", existingEntity.entityId); }); - it("publishes when a new entity is being saved", (done) => { + it("publishes when a new entity is being saved", () => { const testId = "t1"; - receiveUpdatesAndTestTypeAndId(done, "new", testId); - const testEntity = new Entity(testId); entityMapper.save(testEntity, true); + + return receiveUpdatesAndTestTypeAndId("new", testId); }); - function receiveUpdatesAndTestTypeAndId( - done: any, - type?: string, - entityId?: string - ) { - entityMapper.receiveUpdates(Entity).subscribe((e) => { - if (e) { - if (type) { - expect(e.type).toBe(type); - } - if (entityId) { - expect(e.entity.entityId).toBe(entityId); + it("correctly behaves when en empty array is given to the saveAll function", async () => { + const result = await entityMapper.saveAll([]); + expect(result).toHaveSize(0); + }); + + it("correctly saves an array of heterogeneous entities", async () => { + const result = await entityMapper.saveAll([ + new MockEntityA("1"), + new MockEntityA("10"), + new MockEntityA("42"), + ]); + expect(result).toEqual([ + jasmine.objectContaining({ + ok: true, + id: "EntityA:1", + }), + jasmine.objectContaining({ + ok: true, + id: "EntityA:10", + }), + jasmine.objectContaining({ + ok: true, + id: "EntityA:42", + }), + ]); + }); + + it("correctly saves an array of homogeneous entities", async () => { + const result = await entityMapper.saveAll([ + new MockEntityA("1"), + new MockEntityB("10"), + new MockEntityA("42"), + ]); + expect(result).toEqual([ + jasmine.objectContaining({ + ok: true, + id: "EntityA:1", + }), + jasmine.objectContaining({ + ok: true, + id: "EntityB:10", + }), + jasmine.objectContaining({ + ok: true, + id: "EntityA:42", + }), + ]); + }); + + function receiveUpdatesAndTestTypeAndId(type?: string, entityId?: string) { + return new Promise((resolve) => { + entityMapper.receiveUpdates(Entity).subscribe((e) => { + if (e) { + if (type) { + expect(e.type).toBe(type); + } + if (entityId) { + expect(e.entity.entityId).toBe(entityId); + } + resolve(); } - done(); - } + }); }); } + + @DatabaseEntity("EntityA") + class MockEntityA extends Entity {} + + @DatabaseEntity("EntityB") + class MockEntityB extends Entity {} }); diff --git a/src/app/core/entity/entity-mapper.service.ts b/src/app/core/entity/entity-mapper.service.ts index b1bd872195..53a83b5e56 100644 --- a/src/app/core/entity/entity-mapper.service.ts +++ b/src/app/core/entity/entity-mapper.service.ts @@ -133,14 +133,36 @@ export class EntityMapperService { const rawData = this.entitySchemaService.transformEntityToDatabaseFormat( entity ); - this.sendUpdate(entity, entity._rev === undefined ? "new" : "update"); const result = await this._db.put(rawData, forceUpdate); if (result?.ok) { + this.sendUpdate(entity, entity._rev === undefined ? "new" : "update"); entity._rev = result.rev; } return result; } + /** + * Saves an array of entities that are possibly heterogeneous, i.e. + * the entity-type of all the entities does not have to be the same. + * This method should be chosen whenever a bigger number of entities needs to be + * saved + * @param entities The entities to save + */ + public async saveAll(entities: Entity[]): Promise { + const rawData = entities.map((e) => + this.entitySchemaService.transformEntityToDatabaseFormat(e) + ); + const results = await this._db.putAll(rawData); + results.forEach((res, idx) => { + if (res.ok) { + const entity = entities[idx]; + this.sendUpdate(entity, entity._rev === undefined ? "new" : "update"); + entity._rev = res.rev; + } + }); + return results; + } + /** * Delete an entity from the database. * @param entity The entity to be deleted