fix for #409

src/model/MutableAreaDataSource.ts

@@ -53,6 +53,27 @@ export interface UpdateAreaOptions {
 export default class MutableAreaDataSource extends AreaDataSource {
   experimentalUserDataSource = createExperimentalUserDataSource()
 
+  private areaNameCompare (name: string): string {
+    return name.trim().toLocaleLowerCase().split(' ').filter(i => i !== '').join(' ')
+  }
+
+  private async validateUniqueAreaName (areaName: string, parent: AreaType | null): Promise<void> {
+    // area names must be unique in a document area structure context, so if the name has changed we need to check
+    // that the name is unique for this context
+    let neighbours: string[]
+
+    if (parent !== null) {
+      neighbours = (await this.areaModel.find({ _id: parent.children })).map(i => i.area_name)
+    } else {
+      neighbours = (await this.areaModel.find({ pathTokens: { $size: 1 } })).map(i => i.area_name)
+    }
+
+    neighbours = neighbours.map(i => this.areaNameCompare(i))
+    if (neighbours.includes(this.areaNameCompare(areaName))) {
+      throw new UserInputError(`[${areaName}]: This name already exists for some other area in this parent`)
+    }
+  }
+
   async setDestinationFlag (user: MUUID, uuid: MUUID, flag: boolean): Promise<AreaType | null> {
     const session = await this.areaModel.startSession()
     let ret: AreaType | null = null
@@ -119,6 +140,8 @@ export default class MutableAreaDataSource extends AreaDataSource {
       logger.warn(`Missing lnglat for ${countryName}`)
     }
 
+    await this.validateUniqueAreaName(countryName, null)
+
     const rs = await this.areaModel.insertMany(doc)
     if (rs.length === 1) {
       return await rs[0].toObject()
@@ -194,6 +217,8 @@ export default class MutableAreaDataSource extends AreaDataSource {
       parent.metadata.isBoulder = false
     }
 
+    await this.validateUniqueAreaName(areaName, parent)
+
     // See https://github.com/OpenBeta/openbeta-graphql/issues/244
     let experimentaAuthorId: MUUID | null = null
     if (experimentalAuthor != null) {
@@ -377,6 +402,12 @@ export default class MutableAreaDataSource extends AreaDataSource {
         experimentalAuthorId = await this.experimentalUserDataSource.updateUser(session, experimentalAuthor.displayName, experimentalAuthor.url)
       }
 
+      // area names must be unique in a document area structure context, so if the name has changed we need to check
+      // that the name is unique for this context
+      if (areaName !== undefined && this.areaNameCompare(areaName) !== this.areaNameCompare(area.area_name)) {
+        await this.validateUniqueAreaName(areaName, await this.areaModel.findOne({ children: area._id }).session(session))
+      }
+
       const opType = OperationType.updateArea
       const change = await changelogDataSource.create(session, user, opType)
 
@@ -611,7 +642,7 @@ export default class MutableAreaDataSource extends AreaDataSource {
 
 export const newAreaHelper = (areaName: string, parentAncestors: string, parentPathTokens: string[], parentGradeContext: GradeContexts): AreaType => {
   const _id = new mongoose.Types.ObjectId()
-  const uuid = genMUIDFromPaths(parentPathTokens, areaName)
+  const uuid = muuid.v4()
 
   const pathTokens = produce(parentPathTokens, draft => {
     draft.push(areaName)
@@ -663,15 +694,3 @@ export const countryCode2Uuid = (code: string): MUUID => {
   const alpha3 = code.length === 2 ? isoCountries.toAlpha3(code) : code
   return muuid.from(uuidv5(alpha3.toUpperCase(), NIL))
 }
-
-/**
- * Generate a stable UUID from a list of paths. Example: `Canada|Squamish => 8f623793-c2b2-59e0-9e64-d167097e3a3d`
- * @param parentPathTokens Ancestor paths
- * @param thisPath Current area
- * @returns MUUID
- */
-export const genMUIDFromPaths = (parentPathTokens: string[], thisPath: string): MUUID => {
-  const keys = parentPathTokens.slice() // clone array
-  keys.push(thisPath)
-  return muuid.from(uuidv5(keys.join('|').toUpperCase(), NIL))
-}

src/model/__tests__/AreaUtils.ts

@@ -1,12 +1,4 @@
-// import muid from 'uuid-mongodb'
-import { genMUIDFromPaths } from '../MutableAreaDataSource.js'
-
 describe('Test area utilities', () => {
-  it('generates UUID from area tokens', () => {
-    const paths = ['USA', 'Red Rocks']
-    const uid = genMUIDFromPaths(paths, 'Calico Basin')
-
-    expect(uid.toUUID().toString()).toEqual('9fbc30ab-82d7-5d74-85b1-9bec5ee00388')
-    expect(paths).toHaveLength(2) // make sure we're not changing the input!
-  })
+  test.todo('The name comparison code unit')
+  test.todo('The name-uniqueness system with other side-effects stripped out')
 })

src/model/__tests__/MutableAreaDataSource.test.ts

@@ -0,0 +1,191 @@
+import { getAreaModel, createIndexes } from "../../db"
+import inMemoryDB from "../../utils/inMemoryDB"
+import MutableAreaDataSource from "../MutableAreaDataSource"
+import muid, { MUUID } from 'uuid-mongodb'
+import { AreaType, OperationType } from "../../db/AreaTypes"
+import { ChangeRecordMetadataType } from "../../db/ChangeLogType"
+import { UserInputError } from "apollo-server-core"
+import mongoose from "mongoose"
+import exp from "constants"
+
+
+describe("Test area mutations", () => {
+    let areas: MutableAreaDataSource
+    let rootCountry: AreaType
+    let areaCounter = 0
+    const testUser = muid.v4()
+
+    async function addArea(name?: string, extra?: Partial<{ leaf: boolean, boulder: boolean, parent: MUUID | AreaType}>) {
+        function isArea(x: any): x is AreaType {
+            return typeof x.metadata?.area_id !== 'undefined'
+        }
+
+        areaCounter += 1
+        if (name === undefined || name === 'test') {
+            name = process.uptime().toString() + '-' + areaCounter.toString()
+        }
+
+        let parent: MUUID | undefined = undefined
+        if (extra?.parent) {
+            if (isArea(extra.parent)) {
+                parent = extra.parent.metadata?.area_id
+            } else {
+                parent = extra.parent
+            }
+        }
+
+        return areas.addArea(
+            testUser,
+            name,
+            parent ?? rootCountry.metadata.area_id,
+            undefined,
+            undefined,
+            extra?.leaf,
+            extra?.boulder
+        )
+    }
+
+    beforeAll(async () => {
+        await inMemoryDB.connect()
+        await getAreaModel().collection.drop()
+        await createIndexes()
+
+        areas = MutableAreaDataSource.getInstance()
+        // We need a root country, and it is beyond the scope of these tests
+        rootCountry = await areas.addCountry("USA")
+      })
+
+      afterAll(inMemoryDB.close)
+
+      describe("Add area param cases", () => {
+        test("Add a simple area with no specifications using a parent UUID", () => areas
+            .addArea(testUser, 'Texas2', rootCountry.metadata.area_id)
+            .then(area => {
+                expect(area?._change).toMatchObject({
+                    user: testUser,
+                    operation: OperationType.addArea,
+                } satisfies Partial<ChangeRecordMetadataType>)
+            }))
+
+        test("Add an area with an unknown UUID parent should fail",
+                async () => await expect(() => areas.addArea(testUser, 'Texas', muid.v4())).rejects.toThrow())
+
+        test("Add a simple area with no specifications using a country code", () => areas.addArea(testUser, 'Texas part 2', null, 'USA')
+            .then(texas => areas.addArea(testUser, 'Texas Child', texas.metadata.area_id)))
+
+        test("Add a simple area, then specify a new child one level deep", () => addArea('California')
+            .then(async parent => {
+                let child = await addArea('Child', { parent })
+                expect(child).toMatchObject({ area_name: 'Child' })
+                let parentCheck = await areas.findOneAreaByUUID(parent.metadata.area_id)
+                expect(parentCheck?.children ?? []).toContainEqual(child._id)
+            }))
+
+        test("Add a leaf area", () => addArea('Somewhere').then(parent => addArea('Child', { leaf: true, parent }))
+            .then(async leaf => {
+                expect(leaf).toMatchObject({ metadata: { leaf: true }})
+                let area = await areas.areaModel.findById(leaf._id)
+                expect(area).toMatchObject({ metadata: { leaf: true }})
+            }))
+
+        test("Add a leaf area that is a boulder", () => addArea('Maine')
+            .then(parent => addArea('Child', {leaf: true, boulder: true, parent} ))
+            .then(area => {
+                expect(area).toMatchObject({
+                    metadata: {
+                        leaf: true,
+                        isBoulder: true,
+                    },
+                } satisfies Partial<Omit<AreaType, 'metadata'> & { metadata: Partial<AreaType['metadata']>}>)
+            }))
+
+        test("Add a NON-leaf area that is a boulder", () => addArea('Wisconcin')
+            .then(texas => addArea('Child', { leaf: false, boulder: true }))
+            .then(area => {
+                expect(area).toMatchObject({
+                    metadata: {
+                        // Even though we specified false to leaf on the input, we expect it to be true
+                        // after write because a boulder cannot contain sub-areas
+                        leaf: true,
+                        isBoulder: true,
+                    },
+                } satisfies Partial<Omit<AreaType, 'metadata'> & { metadata: Partial<AreaType['metadata']>}>)
+            }))
+
+        test("Adding a child to a leaf area should cause it to become a normal area", () => addArea()
+            .then(parent => Promise.all(new Array(5).map(() => addArea('test', { leaf: true, parent } ))))
+            .then(([leaf]) => leaf)
+            .then(leaf => addArea('test', { parent: leaf }))
+            .then(leaf => expect(leaf).toMatchObject({ metadata: { leaf: false }})))
+
+        test("area names should be unique in their parent context", () => addArea('test').then(async parent => {
+            await addArea('Big ol boulder', { parent })
+            await expect(() => addArea('Big ol boulder', { parent })).rejects.toThrowError(UserInputError)
+        }))
+      })
+
+      test("Delete Area", () => addArea("test").then(area => areas.deleteArea(testUser, area.metadata.area_id)).then(async deleted => {
+        expect(deleted).toBeDefined()
+        // TODO: this test fails based on the data returned, which appears to omit the _deleting field.
+        let d = await areas.areaModel.findById(deleted?._id)
+
+        expect(d).toBeDefined()
+        expect(d).not.toBeNull()
+        expect(d?._deleting).toBeDefined()
+      }))
+
+      test("Delete Area that is already deleted should throw", () => addArea("test")
+        .then(area => areas.deleteArea(testUser, area.metadata.area_id))
+        .then(async area => {
+            expect(area).not.toBeNull()
+            await expect(() => areas.deleteArea(testUser, area!.metadata.area_id)).rejects.toThrow()
+        }))
+
+
+
+      describe("Area update cases", () => {
+        test("Updating an area should superficially pass", () => addArea('test').then(area => areas.updateArea(testUser, area.metadata.area_id, { areaName: `New Name! ${process.uptime()}`})))
+        test("Updating an area should produce a change entry in the changelog", () => addArea('test')
+            .then(area => areas.updateArea(testUser, area.metadata.area_id, { areaName: process.uptime().toString() }))
+            .then(area => {
+                expect(area?._change).toMatchObject({
+                    user: testUser,
+                    operation: OperationType.updateArea,
+                } satisfies Partial<ChangeRecordMetadataType>)
+            }))
+
+        test("Area name uniqueness in its current parent context", () => addArea('test').then(async parent => {
+            let [area, newArea, divorcedArea] = await Promise.all([
+                addArea('original', { parent }),
+                addArea('wannabe', { parent }),
+                addArea(undefined, { parent: rootCountry }),
+            ])
+
+            await Promise.all([
+                // Case where an area gets changed to what it already is, which should not throw an error
+                areas.updateArea(testUser, area.metadata.area_id, { areaName: area.area_name }),
+                // name-uniqueness should not be global, so this shouldn't throw
+                areas.updateArea(testUser, divorcedArea.metadata.area_id, { areaName: area.area_name }),
+                // if we update one of the areas to have a name for which another area already exists, we should expect this to throw.
+                expect(() => areas.updateArea(testUser, newArea.metadata.area_id, { areaName: area.area_name })).rejects.toThrowError(UserInputError),
+            ])
+        }))
+      })
+
+      test("Area name uniqueness should not create a UUID shadow via deletion", () => addArea('test').then(async parent => {
+        let name = 'Big ol boulder'
+        let big = await addArea(name, { boulder: true, parent })
+        await areas.deleteArea(testUser, big.metadata.area_id)
+        await addArea(name, { boulder: true, parent })
+    }))
+
+    test("Area name uniqueness should not create a UUID shadow via edit of name", () => addArea('test').then(async parent => {
+        let nameShadow = 'Big ol boulder 2'
+        let big = await addArea(nameShadow, { boulder: true, parent })
+
+        // We change the name of the original owner of the nameshadow, and then try to add a
+        // name claming the original name in this area structure context
+        await areas.updateArea(testUser, big.metadata.area_id, { areaName: "Still big ol bolder"})
+        await addArea(nameShadow, { boulder: true, parent })
+    }))
+})

src/model/__tests__/updateAreas.ts

@@ -224,14 +224,14 @@ describe('Areas', () => {
     // eslint-disable-next-line
     await new Promise(res => setTimeout(res, 2000))
 
-    await expect(areas.addCountry('ita')).rejects.toThrowError('E11000 duplicate key error')
+    await expect(areas.addCountry('ita')).rejects.toThrowError('This name already exists for some other area in this parent')
   })
 
   it('should not create duplicate sub-areas', async () => {
     const fr = await areas.addCountry('fra')
     await areas.addArea(testUser, 'Verdon Gorge', fr.metadata.area_id)
     await expect(areas.addArea(testUser, 'Verdon Gorge', fr.metadata.area_id))
-      .rejects.toThrowError('E11000 duplicate key error')
+      .rejects.toThrowError('This name already exists for some other area in this parent')
   })
 
   it('should fail when adding without a parent country', async () => {