diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index 23f1800..73d30a9 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/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> { + async computeAncestorsFor (_id: mongoose.Types.ObjectId, session: ClientSession): Promise> { 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 { + 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 { + async syncEmbeddedRelations ( + area: { + _id: mongoose.Types.ObjectId + area_name: string + metadata: { area_id: MUUID } + }, + session: ClientSession, + ancestorPath?: DenormalizedAreaSummary[] + ): Promise { + // 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 { + } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index ffcb805..c55730a 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/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 { + 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() }) } diff --git a/src/model/__tests__/AreaRelationsEmbeddings.test.ts b/src/model/__tests__/AreaRelationsEmbeddings.test.ts index 61a006a..b9dd1e0 100644 --- a/src/model/__tests__/AreaRelationsEmbeddings.test.ts +++ b/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 => { diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 67d60d6..34a703a 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/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') + })) }) }) \ No newline at end of file