From e85a758ec801965850c50756c775ccad93300134 Mon Sep 17 00:00:00 2001 From: sorenjohanson Date: Mon, 2 Dec 2024 11:14:17 +0000 Subject: [PATCH 1/4] update: updateNode and addNode to use upsert --- .../src/map/services/maps.service.spec.ts | 9 +- .../src/map/services/maps.service.ts | 98 +++++++++---------- .../services/mmp/__mocks__/mmp.service.ts | 1 + 3 files changed, 50 insertions(+), 58 deletions(-) diff --git a/teammapper-backend/src/map/services/maps.service.spec.ts b/teammapper-backend/src/map/services/maps.service.spec.ts index 85ae3b36..0c9b9171 100644 --- a/teammapper-backend/src/map/services/maps.service.spec.ts +++ b/teammapper-backend/src/map/services/maps.service.spec.ts @@ -149,16 +149,11 @@ describe('MapsController', () => { const clientNode = mapMmpNodeToClient(node) clientNode.name = 'new' - // we save the time before the update to be able to compare the lastModified date and make sure it's newer than this: - const timeBeforeUpdate = new Date() - await mapsService.updateNode(map.id, clientNode) - const updatedNode = await nodesRepo.findOne({ - where: { id: node.id }, - }) + const updatedNode = await mapsService.updateNode(map.id, clientNode) expect(updatedNode?.lastModified).not.toEqual(oldDate) expect(updatedNode?.lastModified!.getTime()).toBeGreaterThan( - timeBeforeUpdate.getTime() + oldDate.getTime() ) }) }) diff --git a/teammapper-backend/src/map/services/maps.service.ts b/teammapper-backend/src/map/services/maps.service.ts index 37f440f3..137fc760 100644 --- a/teammapper-backend/src/map/services/maps.service.ts +++ b/teammapper-backend/src/map/services/maps.service.ts @@ -24,7 +24,7 @@ import MalformedUUIDError from './uuid.error' @Injectable() export class MapsService { private readonly logger = new Logger(MapsService.name) - + constructor( @InjectRepository(MmpNode) private nodesRepository: Repository, @@ -62,27 +62,24 @@ export class MapsService { ) } - async addNode(mapId: string, node: MmpNode): Promise { + async addNode(mapId: string, node: MmpNode): Promise { // detached nodes are not allowed to have a parent if (node.detached && node.nodeParentId) return Promise.reject() // root nodes are not allowed to have a parent if (node.root && node.nodeParentId) return Promise.reject() if (!mapId || !node) return Promise.reject() - const existingNode = await this.nodesRepository.findOne({ - where: { id: node.id, nodeMapId: mapId }, - }) - if (existingNode) return existingNode - - const newNode = this.nodesRepository.create({ - ...node, - nodeMapId: mapId, - }) - try { - return this.nodesRepository.save(newNode) - } catch(error) { - this.logger.error(`${error.constructor.name} - Failed to add node ${newNode.id}: ${error}`) + await this.nodesRepository.upsert({ + ...node, + nodeMapId: mapId, + }, ["id", "nodeMapId"]); + + return this.nodesRepository.findOne({ + where: { id: node.id, nodeMapId: mapId }, + }) + } catch (error) { + this.logger.error(`${error.constructor.name} - Failed to add node ${node.id}: ${error}`) return Promise.reject(error) } } @@ -101,21 +98,21 @@ export class MapsService { ): Promise { if (!mapId || nodes.length === 0) return Promise.reject() - const reducer = async (previousPromise: Promise, node: MmpNode): Promise => { - const accCreatedNodes = await previousPromise; - if (await this.validatesNodeParentForNode(mapId, node)) { - try { - const newNode = await this.addNode(mapId, node); - return accCreatedNodes.concat([newNode]); - } catch (error) { - this.logger.warn(`Failed to add node ${node.id} to map ${mapId}: ${error}`); - return accCreatedNodes; - } + const reducer = async (previousPromise: Promise, node: MmpNode): Promise => { + const accCreatedNodes = await previousPromise; + if (await this.validatesNodeParentForNode(mapId, node)) { + try { + const newNode = await this.addNode(mapId, node); + return accCreatedNodes.concat([newNode!]); + } catch (error) { + this.logger.warn(`Failed to add node ${node.id} to map ${mapId}: ${error}`); + return accCreatedNodes; } - - this.logger.warn(`Parent with id ${node.nodeParentId} does not exist for node ${node.id} and map ${mapId}`); - return accCreatedNodes; - }; + } + + this.logger.warn(`Parent with id ${node.nodeParentId} does not exist for node ${node.id} and map ${mapId}`); + return accCreatedNodes; + }; return nodes.reduce(reducer, Promise.resolve(new Array())) @@ -140,23 +137,22 @@ export class MapsService { async updateNode( mapId: string, clientNode: IMmpClientNode - ): Promise { - const existingNode = await this.nodesRepository.findOne({ - where: { nodeMapId: mapId, id: clientNode.id }, - }) - - if (!existingNode) return Promise.reject() + ): Promise { + const mmpNode = mapClientNodeToMmpNode(clientNode, mapId); - try { - return this.nodesRepository.save({ - ...existingNode, - ...mapClientNodeToMmpNode(clientNode, mapId), - lastModified: new Date(), - }) - } catch(error) { - this.logger.error(`${error.constructor.name} - Failed to update node ${existingNode.id}: ${error}`) - return Promise.reject(error) - } + try { + await this.nodesRepository.upsert({ + ...mmpNode, + lastModified: new Date(), + }, ["id", "nodeMapId"]) + + return await this.nodesRepository.findOne({ + where: { nodeMapId: mapId, id: clientNode.id }, + }) + } catch (error) { + this.logger.error(`${error.constructor.name} - Failed to update node ${clientNode.id}: ${error}`) + return Promise.reject(error) + } } async removeNode( @@ -185,7 +181,7 @@ export class MapsService { ) try { await this.nodesRepository.save(newRootNode) - } catch(error) { + } catch (error) { this.logger.error(`${error.constructor.name} - Failed to create root node ${newRootNode.id}: ${error}`) return Promise.reject(error) } @@ -219,13 +215,13 @@ export class MapsService { if (clientNode) { const serverNode = await this.nodesRepository.findOne({ where: { nodeMapId: mapId, id: key } }); - + if (serverNode) { const mergedNode = mergeClientNodeIntoMmpNode(clientNode, serverNode); Object.assign(serverNode, mergedNode); try { await this.nodesRepository.save(serverNode); - } catch(error) { + } catch (error) { this.logger.error(`${error.constructor.name} - Failed to update node ${serverNode.id} during diff update: ${error}`) return Promise.reject(error) } @@ -240,11 +236,11 @@ export class MapsService { id: key, nodeMapId: mapId, }) - + if (!existingNode) { return } - + return this.nodesRepository.remove(existingNode) })); } @@ -256,7 +252,7 @@ export class MapsService { }; const diffKeys: DiffKey[] = ["added", "updated", "deleted"]; - + diffKeys.forEach(key => { const changes = diff[key]; if (changes && Object.keys(changes).length > 0) { diff --git a/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts b/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts index 1d75c63b..3ad20613 100644 --- a/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts +++ b/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts @@ -1,3 +1,4 @@ +// @ts-nocheck export const mockMmpService = { new: jest.fn(), addNodeImage: jest.fn(), From d2418588bd60483f83599e414d5d6811c38a657c Mon Sep 17 00:00:00 2001 From: sorenjohanson Date: Thu, 12 Dec 2024 12:14:24 +0000 Subject: [PATCH 2/4] remove ts-nocheck --- .../src/app/core/services/mmp/__mocks__/mmp.service.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts b/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts index 3ad20613..1d75c63b 100644 --- a/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts +++ b/teammapper-frontend/src/app/core/services/mmp/__mocks__/mmp.service.ts @@ -1,4 +1,3 @@ -// @ts-nocheck export const mockMmpService = { new: jest.fn(), addNodeImage: jest.fn(), From b3baaa74aff0eb94c0c761cbe0fb7e256de7a1aa Mon Sep 17 00:00:00 2001 From: sorenjohanson Date: Thu, 12 Dec 2024 12:48:42 +0000 Subject: [PATCH 3/4] adjust methods to return undefined --- teammapper-backend/src/map/services/maps.service.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/teammapper-backend/src/map/services/maps.service.ts b/teammapper-backend/src/map/services/maps.service.ts index 0166c075..c94b58cc 100644 --- a/teammapper-backend/src/map/services/maps.service.ts +++ b/teammapper-backend/src/map/services/maps.service.ts @@ -93,9 +93,12 @@ export class MapsService { nodeMapId: mapId, }, ["id", "nodeMapId"]); - return this.nodesRepository.findOne({ + const newNode = await this.nodesRepository.findOne({ where: { id: node.id, nodeMapId: mapId }, }) + + if (!newNode) return; + return newNode; } catch (error) { this.logger.error(`${error.constructor.name} - Failed to add node ${node.id}: ${error}`) return Promise.reject(error) @@ -169,9 +172,12 @@ export class MapsService { lastModified: new Date(), }, ["id", "nodeMapId"]) - return await this.nodesRepository.findOne({ + const updatedNode = await this.nodesRepository.findOne({ where: { nodeMapId: mapId, id: clientNode.id }, }) + + if (!updatedNode) return; + return updatedNode; } catch (error) { this.logger.error(`${error.constructor.name} - Failed to update node ${clientNode.id}: ${error}`) return Promise.reject(error) From 16a7a7ebfa7cdad7f5ffcd77fc55271c66562e87 Mon Sep 17 00:00:00 2001 From: sorenjohanson Date: Mon, 23 Dec 2024 11:24:41 +0000 Subject: [PATCH 4/4] lint --- .../src/map/services/maps.service.ts | 45 ++++++++++++------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/teammapper-backend/src/map/services/maps.service.ts b/teammapper-backend/src/map/services/maps.service.ts index 7e5cd87d..f5399794 100644 --- a/teammapper-backend/src/map/services/maps.service.ts +++ b/teammapper-backend/src/map/services/maps.service.ts @@ -248,24 +248,37 @@ export class MapsService { ) => { await Promise.all( Object.keys(diff).map(async (key) => { - const clientNode = diff[key] - - if (clientNode) { - const serverNode = await this.nodesRepository.findOne({ where: { nodeMapId: mapId, id: key } }); - - if (serverNode) { - const mergedNode = mergeClientNodeIntoMmpNode(clientNode, serverNode); - Object.assign(serverNode, mergedNode); - try { - await this.nodesRepository.save(serverNode); - } catch (error) { - this.logger.warn(`${error.constructor.name} diffUpdatedCallback(): Failed to update node ${serverNode.id}: ${error}`) - return Promise.reject(error) - } + const clientNode = diff[key]; + + if (!clientNode) { + return; + } + + const serverNode = await this.nodesRepository.findOne({ + where: { + nodeMapId: mapId, + id: key + } + }); + + if (!serverNode) { + return; + } + + const mergedNode = mergeClientNodeIntoMmpNode(clientNode, serverNode); + Object.assign(serverNode, mergedNode); + + try { + await this.nodesRepository.save(serverNode); + } catch (error) { + this.logger.warn( + `${error.constructor.name} diffUpdatedCallback(): Failed to update node ${serverNode.id}: ${error}` + ); + return Promise.reject(error); } }) - ) - } + ); + }; const diffDeletedCallback: DiffCallback = async ( diff: IMmpClientSnapshotChanges