From 029b811f7627b8f0387e126805f5d11c27498402 Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Mon, 12 Jun 2023 14:36:44 -0600 Subject: [PATCH 1/8] working fix, not performant, may need to add more protection for db write. --- src/model/MutableAreaDataSource.ts | 45 ++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 765f9e7d..b71cf116 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -1,7 +1,7 @@ import { geometry, Point } from '@turf/helpers' import muuid, { MUUID } from 'uuid-mongodb' import { v5 as uuidv5, NIL } from 'uuid' -import mongoose, { ClientSession } from 'mongoose' +import mongoose, { ClientSession, Types } from 'mongoose' import { produce } from 'immer' import { UserInputError } from 'apollo-server' import isoCountries from 'i18n-iso-countries' @@ -301,7 +301,20 @@ export default class MutableAreaDataSource extends AreaDataSource { throw new Error('Area update error. Reason: Updating leaf or boulder status of an area with subareas is not allowed.') } - if (areaName != null) area.set({ area_name: sanitizeStrict(areaName) }) + if (areaName != null) { + const sanitizedName = sanitizeStrict(areaName) + area.set({ area_name: sanitizedName }) + + const newPath = [...area.pathTokens] + newPath.pop() + newPath.push(sanitizedName) + area.set({ pathTokens: newPath }) + + for (const childId of area.children) { + await this.updatePathTokens(childId, sanitizedName) + } + } + if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) if (isDestination != null) area.set({ 'metadata.isDestination': isDestination }) if (isLeaf != null) area.set({ 'metadata.leaf': isLeaf }) @@ -358,6 +371,34 @@ export default class MutableAreaDataSource extends AreaDataSource { return ret } + async updatePathTokens (childId: Types.ObjectId, newAreaName: string, index: number = 2): Promise { + // const session = await this.areaModel.startSession() + const filter = { _id: childId } + // const area = await this.areaModel.findOne(filter).session(session) + const area = await this.areaModel.findOne(filter) + + if (area == null) { + throw new Error('pathTokens update error. Reason: child area not found.') + } + + if (area.pathTokens.length > 0) { + const newPath = [...area.pathTokens] + newPath[newPath.length - index] = newAreaName + area.set({ pathTokens: newPath }) + area.save(function (err, result) { + if (err != null) { + throw new Error('pathTokens update error. Reason: save operation failed.') + } else { + console.log(result) + } + }) + + for (const childId of area.children) { + await this.updatePathTokens(childId, newAreaName, index + 1) + } + } + } + /** * * @param user user id From a3d54d2744aec967df7e83cb4405325c6e012b42 Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Mon, 12 Jun 2023 14:55:39 -0600 Subject: [PATCH 2/8] added some comments, cleaned up (#213) --- src/model/MutableAreaDataSource.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index b71cf116..deddc11d 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -305,16 +305,18 @@ export default class MutableAreaDataSource extends AreaDataSource { const sanitizedName = sanitizeStrict(areaName) area.set({ area_name: sanitizedName }) + // change our pathTokens const newPath = [...area.pathTokens] newPath.pop() newPath.push(sanitizedName) area.set({ pathTokens: newPath }) + // iterate over the DB id's in the child list and update pathTokens for (const childId of area.children) { await this.updatePathTokens(childId, sanitizedName) } } - + if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) if (isDestination != null) area.set({ 'metadata.isDestination': isDestination }) if (isLeaf != null) area.set({ 'metadata.leaf': isLeaf }) @@ -382,17 +384,19 @@ export default class MutableAreaDataSource extends AreaDataSource { } if (area.pathTokens.length > 0) { + // copy old tokens const newPath = [...area.pathTokens] + // update the correct index in path tokens -> (length - depth) newPath[newPath.length - index] = newAreaName + // set tokens in the object and save the object to the DB area.set({ pathTokens: newPath }) area.save(function (err, result) { if (err != null) { throw new Error('pathTokens update error. Reason: save operation failed.') - } else { - console.log(result) } }) + // iterate over the DB id's in the child list for (const childId of area.children) { await this.updatePathTokens(childId, newAreaName, index + 1) } From 62223a2df943af0895624bada4c4e7f4cffa8c4a Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Tue, 13 Jun 2023 13:39:14 -0600 Subject: [PATCH 3/8] passed session down to recursive call to initiate rollback if exception is thrown (#213) --- src/model/MutableAreaDataSource.ts | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index deddc11d..377764ca 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -307,13 +307,12 @@ export default class MutableAreaDataSource extends AreaDataSource { // change our pathTokens const newPath = [...area.pathTokens] - newPath.pop() - newPath.push(sanitizedName) + newPath[newPath.length - 1] = sanitizedName area.set({ pathTokens: newPath }) // iterate over the DB id's in the child list and update pathTokens for (const childId of area.children) { - await this.updatePathTokens(childId, sanitizedName) + await this.updatePathTokens(session, childId, sanitizedName) } } @@ -373,32 +372,18 @@ export default class MutableAreaDataSource extends AreaDataSource { return ret } - async updatePathTokens (childId: Types.ObjectId, newAreaName: string, index: number = 2): Promise { - // const session = await this.areaModel.startSession() + async updatePathTokens (session: ClientSession, childId: Types.ObjectId, newAreaName: string, index: number = 2): Promise { const filter = { _id: childId } - // const area = await this.areaModel.findOne(filter).session(session) - const area = await this.areaModel.findOne(filter) - - if (area == null) { - throw new Error('pathTokens update error. Reason: child area not found.') - } + const area = await this.areaModel.findOne(filter).session(session).orFail(new Error('pathTokens update error. Reason: child area not found.')) if (area.pathTokens.length > 0) { - // copy old tokens const newPath = [...area.pathTokens] - // update the correct index in path tokens -> (length - depth) newPath[newPath.length - index] = newAreaName - // set tokens in the object and save the object to the DB area.set({ pathTokens: newPath }) - area.save(function (err, result) { - if (err != null) { - throw new Error('pathTokens update error. Reason: save operation failed.') - } - }) + await area.save() - // iterate over the DB id's in the child list for (const childId of area.children) { - await this.updatePathTokens(childId, newAreaName, index + 1) + await this.updatePathTokens(session, childId, newAreaName, index + 1) } } } From ba28366e12790bc5df880e335777adac6d67abab Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Tue, 13 Jun 2023 13:50:39 -0600 Subject: [PATCH 4/8] added error throw back to save (#213) --- src/model/MutableAreaDataSource.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 377764ca..568a2fd3 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -380,7 +380,11 @@ export default class MutableAreaDataSource extends AreaDataSource { const newPath = [...area.pathTokens] newPath[newPath.length - index] = newAreaName area.set({ pathTokens: newPath }) - await area.save() + area.save((err) => { + if (err != null) { + throw new Error('pathTokens update error. Reason: save operation failed.') + } + }) for (const childId of area.children) { await this.updatePathTokens(session, childId, newAreaName, index + 1) From 45e382e67bb34eeb16d3e4845b31c50a41145175 Mon Sep 17 00:00:00 2001 From: viet nguyen Date: Wed, 14 Jun 2023 09:04:28 -0700 Subject: [PATCH 5/8] refactor: use mongoose 'populate()' to hydrate children array --- src/model/MutableAreaDataSource.ts | 88 ++++++++++++++++-------------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 568a2fd3..4790d322 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -1,7 +1,7 @@ import { geometry, Point } from '@turf/helpers' import muuid, { MUUID } from 'uuid-mongodb' import { v5 as uuidv5, NIL } from 'uuid' -import mongoose, { ClientSession, Types } from 'mongoose' +import mongoose, { ClientSession } from 'mongoose' import { produce } from 'immer' import { UserInputError } from 'apollo-server' import isoCountries from 'i18n-iso-countries' @@ -22,6 +22,8 @@ import { createInstance as createExperimentalUserDataSource } from '../model/Exp isoCountries.registerLocale(enJson) +type AreaDocumnent = mongoose.Document & AreaType + export default class MutableAreaDataSource extends AreaDataSource { experimentalUserDataSource = createExperimentalUserDataSource() @@ -293,6 +295,25 @@ export default class MutableAreaDataSource extends AreaDataSource { const { areaName, description, shortCode, isDestination, isLeaf, isBoulder, lat, lng, experimentalAuthor } = document + // See https://github.com/OpenBeta/openbeta-graphql/issues/244 + let experimentaAuthorId: MUUID | null = null + if (experimentalAuthor != null) { + experimentaAuthorId = await this.experimentalUserDataSource.updateUser(session, experimentalAuthor.displayName, experimentalAuthor.url) + } + + const opType = OperationType.updateArea + const change = await changelogDataSource.create(session, user, opType) + + const _change: ChangeRecordMetadataType = { + user: experimentaAuthorId ?? user, + historyId: change._id, + prevHistoryId: area._change?.historyId._id, + operation: opType, + seq: 0 + } + area.set({ _change }) + area.updatedBy = experimentaAuthorId ?? user + if (area.pathTokens.length === 1) { if (areaName != null || shortCode != null) throw new Error('Area update error. Reason: Updating country name or short code is not allowed.') } @@ -306,14 +327,7 @@ export default class MutableAreaDataSource extends AreaDataSource { area.set({ area_name: sanitizedName }) // change our pathTokens - const newPath = [...area.pathTokens] - newPath[newPath.length - 1] = sanitizedName - area.set({ pathTokens: newPath }) - - // iterate over the DB id's in the child list and update pathTokens - for (const childId of area.children) { - await this.updatePathTokens(session, childId, sanitizedName) - } + await this.updatePathTokens(session, _change, area, sanitizedName) } if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) @@ -337,24 +351,6 @@ export default class MutableAreaDataSource extends AreaDataSource { }) } - // See https://github.com/OpenBeta/openbeta-graphql/issues/244 - let experimentaAuthorId: MUUID | null = null - if (experimentalAuthor != null) { - experimentaAuthorId = await this.experimentalUserDataSource.updateUser(session, experimentalAuthor.displayName, experimentalAuthor.url) - } - - const opType = OperationType.updateArea - const change = await changelogDataSource.create(session, user, opType) - - const _change: ChangeRecordMetadataType = { - user: experimentaAuthorId ?? user, - historyId: change._id, - prevHistoryId: area._change?.historyId._id, - operation: opType, - seq: 0 - } - area.set({ _change }) - area.updatedBy = experimentaAuthorId ?? user const cursor = await area.save() return cursor.toObject() } @@ -372,23 +368,31 @@ export default class MutableAreaDataSource extends AreaDataSource { return ret } - async updatePathTokens (session: ClientSession, childId: Types.ObjectId, newAreaName: string, index: number = 2): Promise { - const filter = { _id: childId } - const area = await this.areaModel.findOne(filter).session(session).orFail(new Error('pathTokens update error. Reason: child area not found.')) - - if (area.pathTokens.length > 0) { + /** + * Update path tokens + * @param session Mongoose session + * @param changeRecord Changeset metadata + * @param area area to update + * @param newAreaName new area name + * @param depth tree depth + */ + async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, depth: number = 2): Promise { + if (area.pathTokens.length > 1) { const newPath = [...area.pathTokens] - newPath[newPath.length - index] = newAreaName + newPath[newPath.length - depth] = newAreaName area.set({ pathTokens: newPath }) - area.save((err) => { - if (err != null) { - throw new Error('pathTokens update error. Reason: save operation failed.') - } - }) - - for (const childId of area.children) { - await this.updatePathTokens(session, childId, newAreaName, index + 1) - } + area.set({ _change: changeRecord }) + await area.save({ session }) + + // hydrate children_ids array with actual area documents + await area.populate('children') + + await Promise.all(area.children.map(async childArea => { + // TS complains about ObjectId type + // Fix this when we upgrade Mongoose library + // @ts-expect-error + await this.updatePathTokens(session, changeRecord, childArea, newAreaName, depth + 1) + })) } } From 0e40fea1e1a8f3ffc78c3c37ce4db9430993013b Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Wed, 14 Jun 2023 15:57:48 -0600 Subject: [PATCH 6/8] added basic unit test for (#213) - verify pathToken change through tree --- src/model/__tests__/updateAreas.ts | 104 +++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/src/model/__tests__/updateAreas.ts b/src/model/__tests__/updateAreas.ts index 5e2496a9..b320da77 100644 --- a/src/model/__tests__/updateAreas.ts +++ b/src/model/__tests__/updateAreas.ts @@ -299,4 +299,108 @@ describe('Areas', () => { await areas.updateSortingOrder(testUser, [{ ...change3, leftRightIndex: -1 }, { ...change4, leftRightIndex: -1 }]) }) + + it('should update self and childrens pathTokens', async () => { + await areas.addCountry('JP') + const a1 = await areas.addArea(testUser, 'Parent', null, 'JP') + const b1 = await areas.addArea(testUser, 'B1', a1.metadata.area_id) + const b2 = await areas.addArea(testUser, 'B2', a1.metadata.area_id) + const c1 = await areas.addArea(testUser, 'C1', b1.metadata.area_id) + const c2 = await areas.addArea(testUser, 'C2', b1.metadata.area_id) + const c3 = await areas.addArea(testUser, 'C3', b2.metadata.area_id) + const e1 = await areas.addArea(testUser, 'E1', c3.metadata.area_id) + + let a1Actual = await areas.findOneAreaByUUID(a1.metadata.area_id) + expect(a1Actual).toEqual( + expect.objectContaining({ + area_name: 'Parent', + pathTokens: ['Japan', 'Parent'] + })) + + let b1Actual = await areas.findOneAreaByUUID(b1.metadata.area_id) + expect(b1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B1'] + })) + + let b2Actual = await areas.findOneAreaByUUID(b2.metadata.area_id) + expect(b2Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B2'] + })) + + let c1Actual = await areas.findOneAreaByUUID(c1.metadata.area_id) + expect(c1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B1', 'C1'] + })) + + let c2Actual = await areas.findOneAreaByUUID(c2.metadata.area_id) + expect(c2Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B1', 'C2'] + })) + + let c3Actual = await areas.findOneAreaByUUID(c3.metadata.area_id) + expect(c3Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B2', 'C3'] + })) + + let e1Actual = await areas.findOneAreaByUUID(e1.metadata.area_id) + expect(e1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Parent', 'B2', 'C3', 'E1'] + })) + + // Update + const doc1: AreaEditableFieldsType = { + areaName: 'Test Name' + } + await areas.updateArea(testUser, a1?.metadata.area_id, doc1) + + // Verify + a1Actual = await areas.findOneAreaByUUID(a1.metadata.area_id) + expect(a1Actual).toEqual( + expect.objectContaining({ + area_name: 'Test Name', + pathTokens: ['Japan', 'Test Name'] + })) + + b1Actual = await areas.findOneAreaByUUID(b1.metadata.area_id) + expect(b1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B1'] + })) + + b2Actual = await areas.findOneAreaByUUID(b2.metadata.area_id) + expect(b2Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B2'] + })) + + c1Actual = await areas.findOneAreaByUUID(c1.metadata.area_id) + expect(c1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B1', 'C1'] + })) + + c2Actual = await areas.findOneAreaByUUID(c2.metadata.area_id) + expect(c2Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B1', 'C2'] + })) + + c3Actual = await areas.findOneAreaByUUID(c3.metadata.area_id) + expect(c3Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B2', 'C3'] + })) + + e1Actual = await areas.findOneAreaByUUID(e1.metadata.area_id) + expect(e1Actual).toEqual( + expect.objectContaining({ + pathTokens: ['Japan', 'Test Name', 'B2', 'C3', 'E1'] + })) + }) }) From d1ddfea1151febfc2a99538036b5d2b4ccfb0375 Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Thu, 15 Jun 2023 14:22:22 -0600 Subject: [PATCH 7/8] default depth value was 2 after refactor, needs to start at 1 --- src/model/MutableAreaDataSource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 4790d322..c280fad6 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -376,7 +376,7 @@ export default class MutableAreaDataSource extends AreaDataSource { * @param newAreaName new area name * @param depth tree depth */ - async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, depth: number = 2): Promise { + async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, depth: number = 1): Promise { if (area.pathTokens.length > 1) { const newPath = [...area.pathTokens] newPath[newPath.length - depth] = newAreaName From cded51feeca5b4253df36356488c20c000c0eca3 Mon Sep 17 00:00:00 2001 From: andrew-jp <74275670+andrew-jp@users.noreply.github.com> Date: Thu, 15 Jun 2023 16:12:15 -0600 Subject: [PATCH 8/8] simplified array indexing since the value is constant, passes tests --- src/model/MutableAreaDataSource.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index c280fad6..80440979 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -376,10 +376,12 @@ export default class MutableAreaDataSource extends AreaDataSource { * @param newAreaName new area name * @param depth tree depth */ - async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, depth: number = 1): Promise { + async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, changeIndex: number = -1): Promise { if (area.pathTokens.length > 1) { + if (changeIndex === -1) { changeIndex = area.pathTokens.length - 1 } + const newPath = [...area.pathTokens] - newPath[newPath.length - depth] = newAreaName + newPath[changeIndex] = newAreaName area.set({ pathTokens: newPath }) area.set({ _change: changeRecord }) await area.save({ session }) @@ -391,7 +393,7 @@ export default class MutableAreaDataSource extends AreaDataSource { // TS complains about ObjectId type // Fix this when we upgrade Mongoose library // @ts-expect-error - await this.updatePathTokens(session, changeRecord, childArea, newAreaName, depth + 1) + await this.updatePathTokens(session, changeRecord, childArea, newAreaName, changeIndex) })) } }