Add validations before nodes are saved (#562)

b310-digital · Nov 15, 2024 · b79d3fc · b79d3fc
1 parent b87f9bb
commit b79d3fc
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 38 deletions.
diff --git a/teammapper-backend/package-lock.json b/teammapper-backend/package-lock.json
diff --git a/teammapper-backend/package.json b/teammapper-backend/package.json
@@ -44,7 +44,9 @@
     "@nestjs/serve-static": "^4.0.2",
     "@nestjs/typeorm": "^10.0.2",
     "@nestjs/websockets": "^10.4.1",
+    "@types/uuid": "^10.0.0",
     "cache-manager": "^5.7.4",
+    "class-validator": "^0.14.1",
     "dotenv": "16.4.5",
     "http-proxy-middleware": "3.0.3",
     "npm-check-updates": "^17.1.10",
@@ -55,8 +57,7 @@
     "rxjs": "^7.8.1",
     "socket.io": "4.8.0",
     "typeorm": "^0.3.20",
-    "uuid": "10.0.0",
-    "@types/uuid": "^10.0.0"
+    "uuid": "10.0.0"
   },
   "devDependencies": {
     "@golevelup/ts-jest": "^0.5.6",
@@ -115,4 +116,4 @@
     "coverageDirectory": "../coverage",
     "testEnvironment": "node"
   }
-}
+}
diff --git a/teammapper-backend/src/map/entities/mmpNode.entity.ts b/teammapper-backend/src/map/entities/mmpNode.entity.ts
@@ -8,8 +8,11 @@ import {
   JoinColumn,
   Generated,
   OneToMany,
+  BeforeInsert, 
+  BeforeUpdate,
 } from 'typeorm'
 import { MmpMap } from './mmpMap.entity'
+import { validateOrReject, IsDefined } from 'class-validator';
 
 @Entity()
 export class MmpNode {
@@ -45,12 +48,15 @@ export class MmpNode {
   /* eslint-enable @typescript-eslint/no-unused-vars */
 
   @Column({ default: false })
+  @IsDefined()
   root: boolean
 
   @Column({ type: 'float' })
+  @IsDefined()
   coordinatesX: number
 
   @Column({ type: 'float' })
+  @IsDefined()
   coordinatesY: number
 
   @Column({ nullable: true })
@@ -84,13 +90,15 @@ export class MmpNode {
   locked: boolean
 
   @Column({ default: false })
+  @IsDefined()
   detached: boolean
 
   @Column({ nullable: true, type: 'float' })
   k: number
 
   @PrimaryColumn('uuid')
   @Index()
+  @IsDefined()
   nodeMapId: string
 
   @Column({ nullable: true })
@@ -105,4 +113,10 @@ export class MmpNode {
 
   @Column({ type: 'timestamptz', default: () => 'now()', nullable: true })
   createdAt: Date
+
+  @BeforeInsert()
+  @BeforeUpdate()
+  async validate() {
+    await validateOrReject(this);
+  }
 }
diff --git a/teammapper-backend/src/map/services/maps.service.ts b/teammapper-backend/src/map/services/maps.service.ts
@@ -79,7 +79,12 @@ export class MapsService {
       nodeMapId: mapId,
     })
 
-    return this.nodesRepository.save(newNode)
+    try {
+      return this.nodesRepository.save(newNode)
+    } catch(error) {
+      this.logger.error(`${error.constructor.name} - Failed to add node ${newNode.id}: ${error}`)
+      return Promise.reject(error)
+    }
   }
 
   async addNodesFromClient(
@@ -142,11 +147,16 @@ export class MapsService {
 
     if (!existingNode) return Promise.reject()
 
-    return this.nodesRepository.save({
-      ...existingNode,
-      ...mapClientNodeToMmpNode(clientNode, mapId),
-      lastModified: new Date(),
-    })
+      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)
+      }
   }
 
   async removeNode(
@@ -173,7 +183,12 @@ export class MapsService {
       const newRootNode = this.nodesRepository.create(
         mapClientBasicNodeToMmpRootNode(rootNode, savedNewMap.id)
       )
-      await this.nodesRepository.save(newRootNode)
+      try {
+        await this.nodesRepository.save(newRootNode)
+      } catch(error) {
+        this.logger.error(`${error.constructor.name} - Failed to create root node ${newRootNode.id}: ${error}`)
+        return Promise.reject(error)
+      }
     }
 
     return newMap
@@ -194,44 +209,43 @@ export class MapsService {
     type DiffKey = keyof IMmpClientMapDiff;
 
     const diffAddedCallback: DiffCallback = async (diff: IMmpClientSnapshotChanges) => {
-      // Sort keys so parents come before children
-      const sortedKeys = Object.keys(diff).sort((a, b) => {
-        const nodeA = diff[a];
-        const nodeB = diff[b];
-        return nodeB!.parent === nodeA!.id ? -1 : 1;  // Parent nodes come first
-      });
-
-      // Then process in order
-      for (const key of sortedKeys) {
-        const clientNode = diff[key];
-        if (clientNode) {
-          const newNode = new MmpNode();
-          Object.assign(newNode, mergeClientNodeIntoMmpNode(clientNode, newNode));
-          newNode.nodeMapId = mapId;
-          await this.nodesRepository.save(newNode);
-        }
-      }
+      const nodes = Object.values(diff);
+      await this.addNodesFromClient(mapId, nodes as IMmpClientNode[])
     }
 
     const diffUpdatedCallback: DiffCallback = async (diff: IMmpClientSnapshotChanges) => {
       await Promise.all(Object.keys(diff).map(async (key) => {
         const clientNode = diff[key];
 
         if (clientNode) {
-          const serverNode = await this.nodesRepository.findOne({ where: { id: key } });
+          const serverNode = await this.nodesRepository.findOne({ where: { nodeMapId: mapId, id: key } });
 
           if (serverNode) {
             const mergedNode = mergeClientNodeIntoMmpNode(clientNode, serverNode);
             Object.assign(serverNode, mergedNode);
-            await this.nodesRepository.save(serverNode);
+            try {
+              await this.nodesRepository.save(serverNode);
+            } catch(error) {
+              this.logger.error(`${error.constructor.name} - Failed to update node ${serverNode.id} during diff update: ${error}`)
+              return Promise.reject(error)
+            }
           }
         }
       }));
     }
 
     const diffDeletedCallback: DiffCallback = async (diff: IMmpClientSnapshotChanges) => {
       await Promise.all(Object.keys(diff).map(async (key) => {
-        await this.nodesRepository.delete({ id: key, nodeMapId: mapId });
+        const existingNode = await this.nodesRepository.findOneBy({
+          id: key,
+          nodeMapId: mapId,
+        })
+
+        if (!existingNode) {
+          return
+        }
+
+        return this.nodesRepository.remove(existingNode)
       }));
     }
 

diff --git a/teammapper-backend/src/map/utils/clientServerMapping.ts b/teammapper-backend/src/map/utils/clientServerMapping.ts
@@ -75,7 +75,7 @@ const mergeClientNodeIntoMmpNode = (
   locked: clientNode?.locked ?? serverNode.locked,
   detached: clientNode?.detached ?? serverNode.detached,
   name: clientNode?.name !== undefined ? clientNode.name : serverNode.name,
-  nodeParentId: clientNode?.parent ?? serverNode.nodeParentId,
+  nodeParentId: (clientNode?.parent || serverNode.nodeParentId) || undefined,
   root: clientNode?.isRoot ?? serverNode.root,
   nodeMapId: serverNode.nodeMapId,
 })

diff --git a/teammapper-backend/test/app.e2e-spec.ts b/teammapper-backend/test/app.e2e-spec.ts
@@ -150,7 +150,8 @@ describe('AppController (e2e)', () => {
                 font: {},
                 colors: {},
                 link: {},
-                detached: true
+                isRoot: true,
+                detached: false
               },
             ],
           })
@@ -174,6 +175,8 @@ describe('AppController (e2e)', () => {
             coordinatesX: 1,
             coordinatesY: 2,
             nodeMapId: '51271bf2-81fa-477a-b0bd-10cecf8d6b65',
+            detached: false,
+            root: true
           }),
         ],
       }).then((map) => {
@@ -189,6 +192,8 @@ describe('AppController (e2e)', () => {
               font: {},
               colors: {},
               link: {},
+              detached: false,
+              root: true
             },
           })
         })

diff --git a/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts b/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts
@@ -195,12 +195,11 @@ export class MmpService implements OnDestroy {
     notifyWithEvent = true
   ) {
     const newProps: UserNodeProperties = properties || { name: '' };
-    const parent =
-      properties?.parent !== undefined
-        ? this.selectNode(properties.parent)
-        : !properties?.detached
-        ? this.selectNode()
-        : null;
+    const parent = properties?.parent
+      ? this.selectNode(properties.parent)
+      : !properties?.detached
+      ? this.selectNode()
+      : null;
 
     // detached nodes are not available as parent
     if (this.selectNode()?.detached) {