Skip to content

Commit f78f8ac

Browse files
authored
Merge pull request #410 from OpenBeta/coco/#409
fix for #409 / #408
2 parents df91b92 + be379f6 commit f78f8ac

4 files changed

Lines changed: 227 additions & 25 deletions

File tree

src/model/MutableAreaDataSource.ts

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,27 @@ export interface UpdateAreaOptions {
5353
export default class MutableAreaDataSource extends AreaDataSource {
5454
experimentalUserDataSource = createExperimentalUserDataSource()
5555

56+
private areaNameCompare (name: string): string {
57+
return name.trim().toLocaleLowerCase().split(' ').filter(i => i !== '').join(' ')
58+
}
59+
60+
private async validateUniqueAreaName (areaName: string, parent: AreaType | null): Promise<void> {
61+
// area names must be unique in a document area structure context, so if the name has changed we need to check
62+
// that the name is unique for this context
63+
let neighbours: string[]
64+
65+
if (parent !== null) {
66+
neighbours = (await this.areaModel.find({ _id: parent.children })).map(i => i.area_name)
67+
} else {
68+
neighbours = (await this.areaModel.find({ pathTokens: { $size: 1 } })).map(i => i.area_name)
69+
}
70+
71+
neighbours = neighbours.map(i => this.areaNameCompare(i))
72+
if (neighbours.includes(this.areaNameCompare(areaName))) {
73+
throw new UserInputError(`[${areaName}]: This name already exists for some other area in this parent`)
74+
}
75+
}
76+
5677
async setDestinationFlag (user: MUUID, uuid: MUUID, flag: boolean): Promise<AreaType | null> {
5778
const session = await this.areaModel.startSession()
5879
let ret: AreaType | null = null
@@ -119,6 +140,8 @@ export default class MutableAreaDataSource extends AreaDataSource {
119140
logger.warn(`Missing lnglat for ${countryName}`)
120141
}
121142

143+
await this.validateUniqueAreaName(countryName, null)
144+
122145
const rs = await this.areaModel.insertMany(doc)
123146
if (rs.length === 1) {
124147
return await rs[0].toObject()
@@ -194,6 +217,8 @@ export default class MutableAreaDataSource extends AreaDataSource {
194217
parent.metadata.isBoulder = false
195218
}
196219

220+
await this.validateUniqueAreaName(areaName, parent)
221+
197222
// See https://github.com/OpenBeta/openbeta-graphql/issues/244
198223
let experimentaAuthorId: MUUID | null = null
199224
if (experimentalAuthor != null) {
@@ -377,6 +402,12 @@ export default class MutableAreaDataSource extends AreaDataSource {
377402
experimentalAuthorId = await this.experimentalUserDataSource.updateUser(session, experimentalAuthor.displayName, experimentalAuthor.url)
378403
}
379404

405+
// area names must be unique in a document area structure context, so if the name has changed we need to check
406+
// that the name is unique for this context
407+
if (areaName !== undefined && this.areaNameCompare(areaName) !== this.areaNameCompare(area.area_name)) {
408+
await this.validateUniqueAreaName(areaName, await this.areaModel.findOne({ children: area._id }).session(session))
409+
}
410+
380411
const opType = OperationType.updateArea
381412
const change = await changelogDataSource.create(session, user, opType)
382413

@@ -611,7 +642,7 @@ export default class MutableAreaDataSource extends AreaDataSource {
611642

612643
export const newAreaHelper = (areaName: string, parentAncestors: string, parentPathTokens: string[], parentGradeContext: GradeContexts): AreaType => {
613644
const _id = new mongoose.Types.ObjectId()
614-
const uuid = genMUIDFromPaths(parentPathTokens, areaName)
645+
const uuid = muuid.v4()
615646

616647
const pathTokens = produce(parentPathTokens, draft => {
617648
draft.push(areaName)
@@ -663,15 +694,3 @@ export const countryCode2Uuid = (code: string): MUUID => {
663694
const alpha3 = code.length === 2 ? isoCountries.toAlpha3(code) : code
664695
return muuid.from(uuidv5(alpha3.toUpperCase(), NIL))
665696
}
666-
667-
/**
668-
* Generate a stable UUID from a list of paths. Example: `Canada|Squamish => 8f623793-c2b2-59e0-9e64-d167097e3a3d`
669-
* @param parentPathTokens Ancestor paths
670-
* @param thisPath Current area
671-
* @returns MUUID
672-
*/
673-
export const genMUIDFromPaths = (parentPathTokens: string[], thisPath: string): MUUID => {
674-
const keys = parentPathTokens.slice() // clone array
675-
keys.push(thisPath)
676-
return muuid.from(uuidv5(keys.join('|').toUpperCase(), NIL))
677-
}

src/model/__tests__/AreaUtils.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,4 @@
1-
// import muid from 'uuid-mongodb'
2-
import { genMUIDFromPaths } from '../MutableAreaDataSource.js'
3-
41
describe('Test area utilities', () => {
5-
it('generates UUID from area tokens', () => {
6-
const paths = ['USA', 'Red Rocks']
7-
const uid = genMUIDFromPaths(paths, 'Calico Basin')
8-
9-
expect(uid.toUUID().toString()).toEqual('9fbc30ab-82d7-5d74-85b1-9bec5ee00388')
10-
expect(paths).toHaveLength(2) // make sure we're not changing the input!
11-
})
2+
test.todo('The name comparison code unit')
3+
test.todo('The name-uniqueness system with other side-effects stripped out')
124
})
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
import { getAreaModel, createIndexes } from "../../db"
2+
import inMemoryDB from "../../utils/inMemoryDB"
3+
import MutableAreaDataSource from "../MutableAreaDataSource"
4+
import muid, { MUUID } from 'uuid-mongodb'
5+
import { AreaType, OperationType } from "../../db/AreaTypes"
6+
import { ChangeRecordMetadataType } from "../../db/ChangeLogType"
7+
import { UserInputError } from "apollo-server-core"
8+
import mongoose from "mongoose"
9+
import exp from "constants"
10+
11+
12+
describe("Test area mutations", () => {
13+
let areas: MutableAreaDataSource
14+
let rootCountry: AreaType
15+
let areaCounter = 0
16+
const testUser = muid.v4()
17+
18+
async function addArea(name?: string, extra?: Partial<{ leaf: boolean, boulder: boolean, parent: MUUID | AreaType}>) {
19+
function isArea(x: any): x is AreaType {
20+
return typeof x.metadata?.area_id !== 'undefined'
21+
}
22+
23+
areaCounter += 1
24+
if (name === undefined || name === 'test') {
25+
name = process.uptime().toString() + '-' + areaCounter.toString()
26+
}
27+
28+
let parent: MUUID | undefined = undefined
29+
if (extra?.parent) {
30+
if (isArea(extra.parent)) {
31+
parent = extra.parent.metadata?.area_id
32+
} else {
33+
parent = extra.parent
34+
}
35+
}
36+
37+
return areas.addArea(
38+
testUser,
39+
name,
40+
parent ?? rootCountry.metadata.area_id,
41+
undefined,
42+
undefined,
43+
extra?.leaf,
44+
extra?.boulder
45+
)
46+
}
47+
48+
beforeAll(async () => {
49+
await inMemoryDB.connect()
50+
await getAreaModel().collection.drop()
51+
await createIndexes()
52+
53+
areas = MutableAreaDataSource.getInstance()
54+
// We need a root country, and it is beyond the scope of these tests
55+
rootCountry = await areas.addCountry("USA")
56+
})
57+
58+
afterAll(inMemoryDB.close)
59+
60+
describe("Add area param cases", () => {
61+
test("Add a simple area with no specifications using a parent UUID", () => areas
62+
.addArea(testUser, 'Texas2', rootCountry.metadata.area_id)
63+
.then(area => {
64+
expect(area?._change).toMatchObject({
65+
user: testUser,
66+
operation: OperationType.addArea,
67+
} satisfies Partial<ChangeRecordMetadataType>)
68+
}))
69+
70+
test("Add an area with an unknown UUID parent should fail",
71+
async () => await expect(() => areas.addArea(testUser, 'Texas', muid.v4())).rejects.toThrow())
72+
73+
test("Add a simple area with no specifications using a country code", () => areas.addArea(testUser, 'Texas part 2', null, 'USA')
74+
.then(texas => areas.addArea(testUser, 'Texas Child', texas.metadata.area_id)))
75+
76+
test("Add a simple area, then specify a new child one level deep", () => addArea('California')
77+
.then(async parent => {
78+
let child = await addArea('Child', { parent })
79+
expect(child).toMatchObject({ area_name: 'Child' })
80+
let parentCheck = await areas.findOneAreaByUUID(parent.metadata.area_id)
81+
expect(parentCheck?.children ?? []).toContainEqual(child._id)
82+
}))
83+
84+
test("Add a leaf area", () => addArea('Somewhere').then(parent => addArea('Child', { leaf: true, parent }))
85+
.then(async leaf => {
86+
expect(leaf).toMatchObject({ metadata: { leaf: true }})
87+
let area = await areas.areaModel.findById(leaf._id)
88+
expect(area).toMatchObject({ metadata: { leaf: true }})
89+
}))
90+
91+
test("Add a leaf area that is a boulder", () => addArea('Maine')
92+
.then(parent => addArea('Child', {leaf: true, boulder: true, parent} ))
93+
.then(area => {
94+
expect(area).toMatchObject({
95+
metadata: {
96+
leaf: true,
97+
isBoulder: true,
98+
},
99+
} satisfies Partial<Omit<AreaType, 'metadata'> & { metadata: Partial<AreaType['metadata']>}>)
100+
}))
101+
102+
test("Add a NON-leaf area that is a boulder", () => addArea('Wisconcin')
103+
.then(texas => addArea('Child', { leaf: false, boulder: true }))
104+
.then(area => {
105+
expect(area).toMatchObject({
106+
metadata: {
107+
// Even though we specified false to leaf on the input, we expect it to be true
108+
// after write because a boulder cannot contain sub-areas
109+
leaf: true,
110+
isBoulder: true,
111+
},
112+
} satisfies Partial<Omit<AreaType, 'metadata'> & { metadata: Partial<AreaType['metadata']>}>)
113+
}))
114+
115+
test("Adding a child to a leaf area should cause it to become a normal area", () => addArea()
116+
.then(parent => Promise.all(new Array(5).map(() => addArea('test', { leaf: true, parent } ))))
117+
.then(([leaf]) => leaf)
118+
.then(leaf => addArea('test', { parent: leaf }))
119+
.then(leaf => expect(leaf).toMatchObject({ metadata: { leaf: false }})))
120+
121+
test("area names should be unique in their parent context", () => addArea('test').then(async parent => {
122+
await addArea('Big ol boulder', { parent })
123+
await expect(() => addArea('Big ol boulder', { parent })).rejects.toThrowError(UserInputError)
124+
}))
125+
})
126+
127+
test("Delete Area", () => addArea("test").then(area => areas.deleteArea(testUser, area.metadata.area_id)).then(async deleted => {
128+
expect(deleted).toBeDefined()
129+
// TODO: this test fails based on the data returned, which appears to omit the _deleting field.
130+
let d = await areas.areaModel.findById(deleted?._id)
131+
132+
expect(d).toBeDefined()
133+
expect(d).not.toBeNull()
134+
expect(d?._deleting).toBeDefined()
135+
}))
136+
137+
test("Delete Area that is already deleted should throw", () => addArea("test")
138+
.then(area => areas.deleteArea(testUser, area.metadata.area_id))
139+
.then(async area => {
140+
expect(area).not.toBeNull()
141+
await expect(() => areas.deleteArea(testUser, area!.metadata.area_id)).rejects.toThrow()
142+
}))
143+
144+
145+
146+
describe("Area update cases", () => {
147+
test("Updating an area should superficially pass", () => addArea('test').then(area => areas.updateArea(testUser, area.metadata.area_id, { areaName: `New Name! ${process.uptime()}`})))
148+
test("Updating an area should produce a change entry in the changelog", () => addArea('test')
149+
.then(area => areas.updateArea(testUser, area.metadata.area_id, { areaName: process.uptime().toString() }))
150+
.then(area => {
151+
expect(area?._change).toMatchObject({
152+
user: testUser,
153+
operation: OperationType.updateArea,
154+
} satisfies Partial<ChangeRecordMetadataType>)
155+
}))
156+
157+
test("Area name uniqueness in its current parent context", () => addArea('test').then(async parent => {
158+
let [area, newArea, divorcedArea] = await Promise.all([
159+
addArea('original', { parent }),
160+
addArea('wannabe', { parent }),
161+
addArea(undefined, { parent: rootCountry }),
162+
])
163+
164+
await Promise.all([
165+
// Case where an area gets changed to what it already is, which should not throw an error
166+
areas.updateArea(testUser, area.metadata.area_id, { areaName: area.area_name }),
167+
// name-uniqueness should not be global, so this shouldn't throw
168+
areas.updateArea(testUser, divorcedArea.metadata.area_id, { areaName: area.area_name }),
169+
// if we update one of the areas to have a name for which another area already exists, we should expect this to throw.
170+
expect(() => areas.updateArea(testUser, newArea.metadata.area_id, { areaName: area.area_name })).rejects.toThrowError(UserInputError),
171+
])
172+
}))
173+
})
174+
175+
test("Area name uniqueness should not create a UUID shadow via deletion", () => addArea('test').then(async parent => {
176+
let name = 'Big ol boulder'
177+
let big = await addArea(name, { boulder: true, parent })
178+
await areas.deleteArea(testUser, big.metadata.area_id)
179+
await addArea(name, { boulder: true, parent })
180+
}))
181+
182+
test("Area name uniqueness should not create a UUID shadow via edit of name", () => addArea('test').then(async parent => {
183+
let nameShadow = 'Big ol boulder 2'
184+
let big = await addArea(nameShadow, { boulder: true, parent })
185+
186+
// We change the name of the original owner of the nameshadow, and then try to add a
187+
// name claming the original name in this area structure context
188+
await areas.updateArea(testUser, big.metadata.area_id, { areaName: "Still big ol bolder"})
189+
await addArea(nameShadow, { boulder: true, parent })
190+
}))
191+
})

src/model/__tests__/updateAreas.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,14 +224,14 @@ describe('Areas', () => {
224224
// eslint-disable-next-line
225225
await new Promise(res => setTimeout(res, 2000))
226226

227-
await expect(areas.addCountry('ita')).rejects.toThrowError('E11000 duplicate key error')
227+
await expect(areas.addCountry('ita')).rejects.toThrowError('This name already exists for some other area in this parent')
228228
})
229229

230230
it('should not create duplicate sub-areas', async () => {
231231
const fr = await areas.addCountry('fra')
232232
await areas.addArea(testUser, 'Verdon Gorge', fr.metadata.area_id)
233233
await expect(areas.addArea(testUser, 'Verdon Gorge', fr.metadata.area_id))
234-
.rejects.toThrowError('E11000 duplicate key error')
234+
.rejects.toThrowError('This name already exists for some other area in this parent')
235235
})
236236

237237
it('should fail when adding without a parent country', async () => {

0 commit comments

Comments
 (0)