Area parent migration implemented and tested

src/model/AreaRelationsEmbeddings.ts

@@ -71,7 +71,7 @@ export class AreaRelationsEmbeddings {
    * remember: This function does not terminate at area passed in, rather it stops at that areas <.parent>.
    * This is out of step with how ancestors are computed elsewhere.
    */
-  async computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise<Array<{ ancestor: AreaType }>> {
+  async computeAncestorsFor (_id: mongoose.Types.ObjectId, session: ClientSession): Promise<Array<{ ancestor: AreaType }>> {
     return await this.areaModel.aggregate([
       { $match: { _id } },
       {
@@ -96,6 +96,21 @@ export class AreaRelationsEmbeddings {
       },
       { $sort: { 'ancestor.level': -1 } }
     ])
+      .session(session)
+  }
+
+  /**
+   * For a given area, compute all areas that trace the path back to root.
+   * remember: This function does not terminate at area passed in, rather it stops at that areas <.parent>.
+   * This is out of step with how ancestors are computed elsewhere.
+   */
+  async hasDescendent (area: mongoose.Types.ObjectId, _id: mongoose.Types.ObjectId, session: ClientSession): Promise<boolean> {
+    return (await this.areaModel.findOne({
+      _id,
+      'embeddedRelations.ancestors': {
+        $elemMatch: { _id: area }
+      }
+    }).session(session)) !== null
   }
 
   /**
@@ -107,24 +122,33 @@ export class AreaRelationsEmbeddings {
    *
    * will be updated with the relevant values.
    */
-  async syncEmbeddedRelations (area: AreaSinkReference, session: ClientSession, ancestorPath?: DenormalizedAreaSummary[]): Promise<void> {
+  async syncEmbeddedRelations (
+    area: {
+      _id: mongoose.Types.ObjectId
+      area_name: string
+      metadata: { area_id: MUUID }
+    },
+    session: ClientSession,
+    ancestorPath?: DenormalizedAreaSummary[]
+  ): Promise<void> {
+    // If an ancestor path has not yet been computed then we can run that query here to initialize the path to this point,
+    // subsequently we can build the ancestors by just adding the current area to the path.
     if (ancestorPath === undefined) {
-      ancestorPath = (await this.computeAncestorsFor(area._id)).map(({ ancestor }) => ({
+      ancestorPath = (await this.computeAncestorsFor(area._id, session)).map(({ ancestor }) => ({
         name: ancestor.area_name,
         _id: ancestor._id,
         uuid: ancestor.metadata.area_id
       }))
     }
 
-    ancestorPath = [
-      ...ancestorPath,
-      {
-        name: area.area_name,
-        _id: area._id,
-        uuid: area.metadata.area_id
-      }]
+    ancestorPath.push({
+      name: area.area_name,
+      _id: area._id,
+      uuid: area.metadata.area_id
+    })
 
     const children = await this.areaModel.find(
+      { parent: area._id },
       { _id: 1, area_name: 1, 'metadata.area_id': 1 }
     )
 
@@ -133,7 +157,8 @@ export class AreaRelationsEmbeddings {
         { _id: area._id },
         {
           'embeddedRelations.ancestors': ancestorPath,
-          // We've gone through the trouble of fetching this data, so we will update.
+          // We've gone through the trouble of fetching this data, so we will update it
+          // since it costs us very little to do that here - but is technically a side-effect of the function.
           'embeddedRelations.children': children.map(area => ({
             name: area.area_name,
             _id: area._id,
@@ -146,8 +171,6 @@ export class AreaRelationsEmbeddings {
   }
 }
 
-interface AreaSinkReference {
-  _id: mongoose.Types.ObjectId
-  area_name: string
-  metadata: { area_id: MUUID }
+export class AreaStructureError extends Error {
+
 }

src/model/MutableAreaDataSource.ts

@@ -26,7 +26,7 @@ import { sanitizeStrict } from '../utils/sanitize.js'
 import AreaDataSource from './AreaDataSource.js'
 import { changelogDataSource } from './ChangeLogDataSource.js'
 import { muuidToString, resolveTransaction, withTransaction } from '../utils/helpers.js'
-import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings.js'
+import { AreaRelationsEmbeddings, AreaStructureError } from './AreaRelationsEmbeddings.js'
 import { getCountriesDefaultGradeContext, GradeContexts } from '../GradeUtils.js'
 
 isoCountries.registerLocale(enJson)
@@ -410,6 +410,10 @@ export default class MutableAreaDataSource extends AreaDataSource {
    *  it as its own sequential operation.
   */
   async setAreaParent (user: MUUID, areaUuid: MUUID, newParent: MUUID, sessionCtx?: ClientSession): Promise<AreaType> {
+    if (muuidToString(areaUuid) === muuidToString(newParent)) {
+      throw new AreaStructureError('You cannot set self as a parent')
+    }
+
     return await resolveTransaction(this.areaModel, sessionCtx, async (session) => {
       const area = await this.areaModel.findOne({ 'metadata.area_id': areaUuid })
         .orFail()
@@ -422,7 +426,7 @@ export default class MutableAreaDataSource extends AreaDataSource {
       if (area.parent === undefined) {
         // This is a root node (country, likely) and so this is an operation with
         // high level privliges that are currently not enumerated.
-        throw new Error('You cannot migrate, what appears to be, a country.')
+        throw new AreaStructureError('You cannot migrate, what appears to be, a country.')
       }
 
       // Retrieve the current parent for this area.
@@ -441,10 +445,18 @@ export default class MutableAreaDataSource extends AreaDataSource {
 
       const nextParent = await this.areaModel.findOne({ 'metadata.area_id': newParent }).orFail().session(session)
 
+      // We need to validate that a circular reference has not been invoked.
+      // Essentially, we cannot specify a parent reference if we have that parent somewhere in our decendants.
+      if (
+        area.embeddedRelations.children.includes(nextParent._id) ||
+        await this.relations.hasDescendent(area._id, nextParent._id, session)
+      ) {
+        throw new AreaStructureError('CIRCULAR STRUCTURE: The requested parent is already a descendant, and so cannot also be a parent.')
+      }
+
       // By this point we are satisfied that there are no obvious reasons to reject this request, so we can begin saving
       // and producing effects in the context of this transaction.
       area.parent = nextParent._id
-      await this.relations.computeEmbeddedAncestors(area, session)
 
       const change = await changelogDataSource.create(session, user, OperationType.changeAreaParent)
 
@@ -459,8 +471,9 @@ export default class MutableAreaDataSource extends AreaDataSource {
         updatedBy: user
       })
 
-      const cursor = await area.save({ session })
-      return await cursor.toObject()
+      await area.save({ session })
+      await this.relations.computeEmbeddedAncestors(area, session)
+      return await this.areaModel.findById(area._id).orFail()
     })
   }
 

src/model/__tests__/AreaRelationsEmbeddings.test.ts

@@ -4,7 +4,7 @@ import MutableAreaDataSource from "../MutableAreaDataSource"
 import muid from 'uuid-mongodb'
 import { getAreaModel, createIndexes } from "../../db"
 import inMemoryDB from "../../utils/inMemoryDB"
-import { muuidToString } from "../../utils/helpers"
+import { muuidToString, resolveTransaction, useOrCreateTransaction } from "../../utils/helpers"
 
 export function embeddedRelationsReducer(path: AreaType[]) {
     let trace: DenormalizedAreaSummary[] = []
@@ -97,7 +97,8 @@ describe("updating of areas should propogate embeddedRelations", () => {
     }
 
     test('computing ancestors from reified node', async () => growTree().then(async (tree) => {
-        let computedAncestors = await areas.relations.computeAncestorsFor(tree.at(-1)!._id)
+        await useOrCreateTransaction(areas.areaModel, undefined, async (session) => {
+        let computedAncestors = await areas.relations.computeAncestorsFor(tree.at(-1)!._id, session)
 
         // We expect the mongo computation to pull down the same data as our locally constructed tree
         // caveat: the ancestor computation does not include the leaf.
@@ -114,6 +115,8 @@ describe("updating of areas should propogate embeddedRelations", () => {
             expect(current.ancestor._id.equals(tree[idx]._id))
             return current
         })
+    })
+
     }))
 
     test('ancestors should be computed on area add.', async () => growTree(5).then(async (tree) => {
@@ -131,20 +134,22 @@ describe("updating of areas should propogate embeddedRelations", () => {
     }))
 
     test("re-naming an area should update its pathTokens", async () => growTree(5).then(async tree => {
-        let treeLength = tree.length
-        let target = Math.floor(treeLength / 2)
+        await useOrCreateTransaction(areas.areaModel, undefined, async (session) => {
+            let treeLength = tree.length
+            let target = Math.floor(treeLength / 2)
 
-        await areas.updateArea(
-            testUser,
-            tree[target].metadata.area_id, {
-                areaName: 'updated name'
-            },
-        )
+            await areas.updateArea(
+                testUser,
+                tree[target].metadata.area_id, {
+                    areaName: 'updated name'
+                },
+            )
 
-        tree = (await areas.relations.computeAncestorsFor(tree.at(-1)!._id)).map( i => i.ancestor)
+            tree = (await areas.relations.computeAncestorsFor(tree.at(-1)!._id, session)).map( i => i.ancestor)
 
-        expect(tree[target].area_name).toEqual('updated name')
-        expect(tree[target].embeddedRelations.ancestors.map(i => i.name)[target]).toEqual('updated name')
+            expect(tree[target].area_name).toEqual('updated name')
+            expect(tree[target].embeddedRelations.ancestors.map(i => i.name)[target]).toEqual('updated name')
+        })
     }))
 
     test("re-naming a parent should update all descendant pathTokens", async () => growTree(5, 2).then(async tree => {

src/model/__tests__/MutableAreaDataSource.test.ts

@@ -7,6 +7,7 @@ import { ChangeRecordMetadataType } from "../../db/ChangeLogType"
 import { UserInputError } from "apollo-server-core"
 import { muuidToString, resolveTransaction, useOrCreateTransaction } from "../../utils/helpers"
 import { embeddedRelationsReducer } from "./AreaRelationsEmbeddings.test"
+import { AreaStructureError } from "../AreaRelationsEmbeddings"
 
 describe("Test area mutations", () => {
     let areas: MutableAreaDataSource
@@ -269,8 +270,32 @@ describe("Test area mutations", () => {
                     expect(original.embeddedRelations.children.some(child => child.equals(area._id))).not.toBeTruthy()
                 }))
 
-            test.todo('Updating an areas parent reference should produce an appropriate changelog item')
-            test.todo('Updating an areas parent reference should update an areas embeddedRelations')
+            test('Updating an areas parent reference should produce an appropriate changelog item', async () => {
+                let area = await addArea()
+                let parent = await addArea()
+
+                await areas.setAreaParent(testUser, area.metadata.area_id, parent.metadata.area_id)
+
+                area = await areas.findOneAreaByUUID(area.metadata.area_id)
+                expect(area._change).toBeDefined()
+                expect(area._change!.operation).toEqual(OperationType.changeAreaParent)
+            })
+            test('Updating an areas parent reference should update an areas embeddedRelations', async () => {
+                let railLength = 7
+                let rail: AreaType[] = [rootCountry]
+                let newParent = await addArea()
+
+                for (const idx in Array.from({ length: railLength }).map((_, idx) => idx)) {
+                    rail.push(await addArea(undefined, { parent: rail[idx] }))
+                }
+
+                expect(rail).toHaveLength(railLength + 1)
+
+                await areas.setAreaParent(testUser, rail[1].metadata.area_id, newParent.metadata.area_id)
+                let area = await areas.findOneAreaByUUID(rail[1].metadata.area_id)
+                expect(area.embeddedRelations.ancestors[2]._id.equals(newParent._id))
+            })
+
             test('Modifying an areas parent should update its child embeddedRelations', async () => {
                 let railLength = 7
                 let rail: AreaType[] = [rootCountry]
@@ -289,7 +314,7 @@ describe("Test area mutations", () => {
                     // get the most up-to-date copy of this area
                     const area = await areas.areaModel.findById(oldAreaData._id).orFail()
                     // This expects a valid chain of IDs for each ancestor - the second-last ancestor is our parent
-                    expect(area.embeddedRelations.ancestors.at(-2)!._id.equals(area.parent!))
+                    expect(area.embeddedRelations.ancestors.at(-2)!._id.equals(area.parent!)).toEqual(true)
 
                     const pathElement = area.embeddedRelations.ancestors[offset]
                     // we expect the element at [offset] to have changed such that the new objectID is not equal to its previous value
@@ -301,14 +326,29 @@ describe("Test area mutations", () => {
                     // name and UUID were not? This is a strange case indeed.
                     expect(muuidToString(pathElement.uuid)).toEqual(muuidToString(newParent.metadata.area_id))
 
-
                     expect(pathElement.name).not.toEqual(oldAreaData.embeddedRelations.ancestors[offset].name)
                     expect(pathElement.name).toEqual(newParent.area_name)
                 }
             })
 
-            test.todo('Attempting to update a countries parent should throw')
-            test.todo('Circular references should always be prohibitted')
-            test.todo('Self-referece should always be prohobitted')
+            test('Attempting to update a countries parent should throw', async () => {
+                let other = await areas.addCountry('CA')
+                expect(() => areas.setAreaParent(testUser, rootCountry.metadata.area_id, other.metadata.area_id)).rejects.toThrowError(AreaStructureError)
+            })
+
+            test('Circular references should always be prohibitted', async () => {
+                let parent = await addArea()
+                let child = await addArea(undefined, { parent })
+                let child2 = await addArea(undefined, { parent: child })
+
+                expect(() => areas.setAreaParent(testUser, parent.metadata.area_id, child.metadata.area_id )).rejects.toThrowError(AreaStructureError)
+                expect(() => areas.setAreaParent(testUser, parent.metadata.area_id, child2.metadata.area_id )).rejects.toThrowError(AreaStructureError)
+                expect(() => areas.setAreaParent(testUser, child.metadata.area_id, child2.metadata.area_id )).rejects.toThrowError(AreaStructureError)
+            })
+
+            test('Self-referece should always be prohobitted', async () => addArea().then(area => {
+                expect(() => areas.setAreaParent(testUser, area.metadata.area_id, area.metadata.area_id)).rejects.toThrowError(AreaStructureError)
+                expect(() => areas.setAreaParent(testUser, area.metadata.area_id, area.metadata.area_id)).rejects.toThrowError('You cannot set self as a parent')
+            }))
     })
 })