From 4e9b17799764fb8b1203fcd8b0a3c1a91a3d161d Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 2 Nov 2024 11:31:36 +0200 Subject: [PATCH 01/27] shebang should be first line (https://en.wikipedia.org/wiki/Shebang_(Unix)#Version_8_improved_shell_scripts if you are sufficiently nerdy to care) --- seed-db.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/seed-db.sh b/seed-db.sh index 67bd0c66..ecf198df 100755 --- a/seed-db.sh +++ b/seed-db.sh @@ -1,12 +1,12 @@ +#!/bin/bash # Rebuild your local database with a copy of OpenBeta staging database. # -# To keep running time short, the script only downloads the remote +# To keep running time short, the script only downloads the remote # database dump file once. Specify 'download' argument to force download. # # Syntax: # ./seed-db.sh [download] # -#!/bin/bash FILE_NAME="openbeta-stg-db.tar.gz" REMOTE_FILE="https://storage.googleapis.com/openbeta-dev-dbs/$FILE_NAME" From 8b66de366d8980d16386b8501394b1a3146ff322 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 2 Nov 2024 12:11:02 +0200 Subject: [PATCH 02/27] For those using bash beautify vscode extension --- .vscode/settings.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index e5e150b7..e6252d1c 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -17,5 +17,6 @@ "javascript.format.semicolons": "remove", "typescript.format.enable": false, "prettier.enable": false, - "editor.defaultFormatter": "standard.vscode-standard" + "editor.defaultFormatter": "standard.vscode-standard", + "bashBeautify.tabSize": 2 } \ No newline at end of file From a9c852701cc105506b1bc8c291434a3f3421c52c Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 2 Nov 2024 12:24:04 +0200 Subject: [PATCH 03/27] Make replica set specification optional in seed script I ran into a one-off issue where the replica set specification was throwing off the script in a development environment. This code will let you un-set the variable if you don't need it while in dev. --- seed-db.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/seed-db.sh b/seed-db.sh index ecf198df..5bb2bf65 100755 --- a/seed-db.sh +++ b/seed-db.sh @@ -22,7 +22,11 @@ tar xzf $FILE_NAME . .env -connStr="${MONGO_SCHEME}://${MONGO_INITDB_ROOT_USERNAME}:${MONGO_INITDB_ROOT_PASSWORD}@${MONGO_SERVICE}/${MONGO_DBNAME}?authSource=${MONGO_AUTHDB}&tls=${MONGO_TLS}&replicaSet=${MONGO_REPLICA_SET_NAME}" +connStr="${MONGO_SCHEME}://${MONGO_INITDB_ROOT_USERNAME}:${MONGO_INITDB_ROOT_PASSWORD}@${MONGO_SERVICE}/${MONGO_DBNAME}?authSource=${MONGO_AUTHDB}&tls=${MONGO_TLS}" + +if [ -z "${MONGO_REPLICA_SET_NAME}" ]; then + connStr+="&replicaSet=${MONGO_REPLICA_SET_NAME}" +fi mongorestore --uri "$connStr" -d=${MONGO_DBNAME} --gzip --drop ./db-dumps/staging/openbeta From 0b0d6307bf78f1e6410cd70e40be6a5d6dd6f515 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 2 Nov 2024 12:25:13 +0200 Subject: [PATCH 04/27] Add a shell.nix for people using nixOS This is by no means a complete environment, but if no one minds me leaving this here it would save me some trouble - but is not required. If you are unfamiliar with nixos, this file is intended to give my environment the necessary mongo utils / yarn / node and so forth. You can peg a node version and all sorts of various magic. I would recommend any nerds with sufficient free time investigate - but you will find it very hard to escape once it has ensnared you. --- shell.nix | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 shell.nix diff --git a/shell.nix b/shell.nix new file mode 100644 index 00000000..492016c4 --- /dev/null +++ b/shell.nix @@ -0,0 +1,20 @@ +{ + pkgs ? import { + config = { + allowUnfree = true; + }; + }, +}: +pkgs.mkShell { + buildInputs = with pkgs; [ + mongodb-tools + yarn + ]; + + shellHook = '' + set -a + source .env + + echo "🧗 Alle!" + ''; +} From c0eb1881f21900b21861135c4e622f6a540f3424 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 2 Nov 2024 13:34:10 +0200 Subject: [PATCH 05/27] Integrate memory server into nixos environment --- shell.nix | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/shell.nix b/shell.nix index 492016c4..7aa2cca0 100644 --- a/shell.nix +++ b/shell.nix @@ -9,12 +9,27 @@ pkgs.mkShell { buildInputs = with pkgs; [ mongodb-tools yarn + mongodb-ce ]; + # MONGOMS_DOWNLOAD_URL = "https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu2404-8.0.1.tgz"; + MONGOMS_DISTRO = "ubuntu-22.04"; + MONGOMS_RUNTIME_DOWNLOAD = false; + MONGOMS_SYSTEM_BINARY = "${pkgs.mongodb-ce}/bin/mongod"; + # you will need to keep this value in sync with the pre-built mongodb-ce + # (or you can use the mongodb package which will build from source and take a WHILE) + # https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/mo/mongodb-ce/package.nix#L113 + MONGOMS_VERSION = "7.0.14"; + shellHook = '' set -a source .env + # mongotop alias + alias mto="mongotop --uri=\"$MONGO_SCHEME://$MONGO_INITDB_ROOT_USERNAME:$MONGO_INITDB_ROOT_PASSWORD@$MONGO_SERVICE/$MONGO_DBNAME?authSource=$MONGO_AUTHDB&tls=$MONGO_TLS\"" + # mongostat alias + alias mst="mongostat --uri=\"$MONGO_SCHEME://$MONGO_INITDB_ROOT_USERNAME:$MONGO_INITDB_ROOT_PASSWORD@$MONGO_SERVICE/$MONGO_DBNAME?authSource=$MONGO_AUTHDB&tls=$MONGO_TLS\"" + echo "🧗 Alle!" ''; } From bf1f30776e82d323df77fa1efb0459dd34655adc Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sun, 3 Nov 2024 09:27:38 +0200 Subject: [PATCH 06/27] compass is a neat tool - though duplicates much of the functionality of mono-express --- shell.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/shell.nix b/shell.nix index 7aa2cca0..b7915e88 100644 --- a/shell.nix +++ b/shell.nix @@ -10,6 +10,7 @@ pkgs.mkShell { mongodb-tools yarn mongodb-ce + mongodb-compass ]; # MONGOMS_DOWNLOAD_URL = "https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu2404-8.0.1.tgz"; From 6ca34b2e9003420df17132cb8654981de86f4749 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sun, 3 Nov 2024 09:28:07 +0200 Subject: [PATCH 07/27] A naieve implementation of the functionality I was thinking of using. The query is too slow, it remains to be seen if this is because the scope is simply too liberal - but my intuition is that this is down to the regex search and full-document retrieval Current root query time is along the lines of 16 seconds (Unthinkably slow) so I'll try another approach, otherwise this architechture is simply no good. --- src/__tests__/areas.ts | 48 +++++++++++++++++++++++++++++++++ src/db/AreaTypes.ts | 2 ++ src/db/index.ts | 13 +++++++-- src/graphql/area/AreaQueries.ts | 43 +++++++++++++++++++++++++++-- src/graphql/schema/Area.gql | 36 +++++++++++++++++++++++++ src/model/AreaDataSource.ts | 29 +++++++++++++++++++- 6 files changed, 166 insertions(+), 5 deletions(-) diff --git a/src/__tests__/areas.ts b/src/__tests__/areas.ts index 8047815d..35b59573 100644 --- a/src/__tests__/areas.ts +++ b/src/__tests__/areas.ts @@ -107,4 +107,52 @@ describe('areas API', () => { expect(areaResult.organizations[0].orgId).toBe(muuidToString(alphaOrg.orgId)) }) }) + + describe('area structure API', () => { + const structureQuery = ` + query structure($parent: ID) { + structure(uuid: $parent) { + uuid + organizations { + orgId + } + } + } + ` + + it('retrieves traversal of all roots using auto-depth', async () => { + const response = await queryAPI({ + query: structureQuery, + operationName: 'structure', + userUuid, + app + }) + + expect(response.statusCode).toBe(200) + const areaResult = response.body.data.area + expect(areaResult.uuid).toBe(muuidToString(ca.metadata.area_id)) + // Even though alphaOrg associates with ca's parent, usa, it excludes + // ca and so should not be listed. + expect(areaResult.organizations).toHaveLength(0) + }) + + it('retrieves traversal of a high level root using auto-depth', async () => { + const response = await queryAPI({ + query: structureQuery, + operationName: 'structure', + // Pass the usa top-level countru area + variables: { input: usa.metadata.area_id }, + userUuid, + app + }) + expect(response.statusCode).toBe(200) + const areaResult = response.body.data.area + expect(areaResult.uuid).toBe(muuidToString(ca.metadata.area_id)) + // Even though alphaOrg associates with ca's parent, usa, it excludes + // ca and so should not be listed. + expect(areaResult.organizations).toHaveLength(0) + }) + + it('failure to properly constrain depth', async () => {}) + }) }) diff --git a/src/db/AreaTypes.ts b/src/db/AreaTypes.ts index 7dad6116..32e9da1a 100644 --- a/src/db/AreaTypes.ts +++ b/src/db/AreaTypes.ts @@ -28,6 +28,8 @@ export type AreaType = IAreaProps & { metadata: IAreaMetadata } +export type ShadowArea = Pick & { parent: IAreaProps['uuid'] | null } + /** * Properties that areas are expected to have. * Objects of this kind may not be reified in the database and, if they are, diff --git a/src/db/index.ts b/src/db/index.ts index d087159f..d8fa7838 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -75,9 +75,18 @@ export const createIndexes = async (): Promise => { await getUserModel().createIndexes() } -export const gracefulExit = async (exitCode: number = 0): Promise => { +const SIGCODE = { + SIGINT: 130, + SIGTERM: 143 +} + +export const gracefulExit = async (exitCode: number | keyof typeof SIGCODE = 0): Promise => { await mongoose.connection.close(function () { - logger.info('Gracefully exiting.') + logger.info('Gracefully exiting') + if (typeof exitCode === 'string') { + process.exit(SIGCODE[exitCode]) + } + process.exit(exitCode) }) } diff --git a/src/graphql/area/AreaQueries.ts b/src/graphql/area/AreaQueries.ts index 609d7da6..919f5ddf 100644 --- a/src/graphql/area/AreaQueries.ts +++ b/src/graphql/area/AreaQueries.ts @@ -1,6 +1,15 @@ -import { AreaType } from '../../db/AreaTypes' +import muuid, { MUUID } from 'uuid-mongodb' +import { AreaType, ShadowArea } from '../../db/AreaTypes' import { Context } from '../../types' +interface StructureQuery extends Partial<{ + parent: MUUID + filter: Partial<{ + depth: number + + }> +}> {} + const AreaQueries = { cragsWithin: async (_, { filter }, { dataSources }: Context): Promise => { const { areas } = dataSources @@ -11,8 +20,38 @@ const AreaQueries = { countries: async (_, params, { dataSources }: Context): Promise => { const { areas } = dataSources return await areas.listAllCountries() - } + }, + + structure: async (_, params: StructureQuery, { dataSources }: Context): Promise => { + const { areas } = dataSources + const data: ShadowArea[] = [] + const roots: MUUID[] = [] + console.time('structure query') + + if (params.parent !== undefined) { + // A parent has been specified so we can eval just that node + roots.push(params.parent) + } else { + roots.push(...(await areas.listAllCountries()).map(i => i.metadata.area_id)) + } + + // For each root that we're interested in, we want to scan accross + for (const root of roots) { + const area = await areas.findOneAreaByUUID(root) + const parent = area.ancestors.split(',').pop() + if (parent === undefined) continue + + data.push({ uuid: root, area_name: area.area_name, parent: muuid.from(parent) }) + // descendents takes care of its own look-ahead to make sure the query + // does not munch up stupid bandwidth + data.push(...(await areas.descendents(root))) + } + + console.timeEnd('structure query') + + return data + } } export default AreaQueries diff --git a/src/graphql/schema/Area.gql b/src/graphql/schema/Area.gql index 20fbc88e..f4504f67 100644 --- a/src/graphql/schema/Area.gql +++ b/src/graphql/schema/Area.gql @@ -11,6 +11,7 @@ type Query { ): [CragsNear] cragsWithin(filter: SearchWithinFilter): [Area] countries: [Area] + structure(parent: ID, filter: StructureQuery): [AreaShadow!]! } "A climbing area, wall or crag" @@ -183,6 +184,16 @@ input Filter { field_compare: [ComparisonFilter] } +""" +When performing a query about area structure some limits must be specified to prevent +output cutoff. Selecting the entire area structure in one go is unreasonable, and the +query will cut off if it looks like it's about to deliver some rediculous number of +records. +""" +input StructureQuery { + depth: Int +} + enum Field { density totalClimbs @@ -226,3 +237,28 @@ type CragsNear { count: Int! crags: [Area] } + + +""" +Some graph queries are simply too heavy to allow full area types to be selected +in the process, so we specify a new type that is expressly for the purpose of +querying large relationship patterns from the database. +""" +type AreaShadow { + uuid: ID! + area_name: String! + """ + If this are has a parent it can be referenced here. + """ + parent: ID + """ + The path hash is not computed at query time so this should be inexpensive to fetch. + and deliver. This hash can be used to invalidate + """ + pathHash: String + """ + If this area has climbs as its direct descendents then we can loop in ONLY a pointer + to these climbs, otherwise it will be null. + """ + climbs: [ID!] +} \ No newline at end of file diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 443c2aad..84d18d8f 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -4,7 +4,7 @@ import muuid from 'uuid-mongodb' import bboxPolygon from '@turf/bbox-polygon' import { getAreaModel, getMediaObjectModel } from '../db/index.js' -import { AreaType } from '../db/AreaTypes' +import { AreaType, ShadowArea } from '../db/AreaTypes' import { AreaFilterParams, BBoxType, @@ -18,6 +18,7 @@ import { import { getClimbModel } from '../db/ClimbSchema.js' import { ClimbGQLQueryType } from '../db/ClimbTypes.js' import { logger } from '../logger.js' +import { muuidToString } from '../utils/helpers.js' export default class AreaDataSource extends MongoDataSource { areaModel = getAreaModel() @@ -275,4 +276,30 @@ export default class AreaDataSource extends MongoDataSource { } return await this.areaModel.find(filter).lean() } + + async descendents (of: muuid.MUUID, previous?: ShadowArea[]): Promise { + previous = previous ?? [] + + // All descendents + const regex = new RegExp(`\\b${muuidToString(of)}\\b`) + const cursor = this.collection.find({ ancestors: regex }) + + previous.push() + + let counter = 0 + return await cursor.map((i) => { + // if (counter > 1000) { + // throw new Error('Data volume exceeded. Try filtering data') + // } + const parentString = i.ancestors.split(',').at(-2) + let parent: muuid.MUUID | null = null + + if (parentString !== undefined) { + parent = muuid.from(parentString) + } + + counter += 1 + return { area_name: i.area_name, uuid: i.metadata.area_id, parent } satisfies ShadowArea + }).toArray() as ShadowArea[] + } } From 027b0e67eebbe09ab9ad901b1f46ea32270dfda3 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sun, 3 Nov 2024 09:38:16 +0200 Subject: [PATCH 08/27] I think I may have been wrong about this. Path Hash is not a cached value? This may be down to me not manually running some cron operation in my development environment? I will work it out later. --- src/graphql/schema/Area.gql | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/graphql/schema/Area.gql b/src/graphql/schema/Area.gql index f4504f67..392b2656 100644 --- a/src/graphql/schema/Area.gql +++ b/src/graphql/schema/Area.gql @@ -252,11 +252,6 @@ type AreaShadow { """ parent: ID """ - The path hash is not computed at query time so this should be inexpensive to fetch. - and deliver. This hash can be used to invalidate - """ - pathHash: String - """ If this area has climbs as its direct descendents then we can loop in ONLY a pointer to these climbs, otherwise it will be null. """ From 4794107bce6f0692b30efc5cadd3a975f05e6eb0 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Mon, 4 Nov 2024 08:27:41 +0200 Subject: [PATCH 09/27] Performance improvements and less jank There is still some query tuning that I want to do - I might sqaush this commit later Cost Reference Texas ~50ms California ~900ms --- src/db/AreaTypes.ts | 2 +- src/graphql/area/AreaQueries.ts | 34 ++-------- src/graphql/schema/Area.gql | 2 +- src/model/AreaDataSource.ts | 109 +++++++++++++++++++++++++------- 4 files changed, 93 insertions(+), 54 deletions(-) diff --git a/src/db/AreaTypes.ts b/src/db/AreaTypes.ts index 32e9da1a..39be0ed3 100644 --- a/src/db/AreaTypes.ts +++ b/src/db/AreaTypes.ts @@ -28,7 +28,7 @@ export type AreaType = IAreaProps & { metadata: IAreaMetadata } -export type ShadowArea = Pick & { parent: IAreaProps['uuid'] | null } +export type ShadowArea = Pick & { parent: IAreaProps['uuid'] | null } /** * Properties that areas are expected to have. diff --git a/src/graphql/area/AreaQueries.ts b/src/graphql/area/AreaQueries.ts index 919f5ddf..10560462 100644 --- a/src/graphql/area/AreaQueries.ts +++ b/src/graphql/area/AreaQueries.ts @@ -1,14 +1,14 @@ import muuid, { MUUID } from 'uuid-mongodb' import { AreaType, ShadowArea } from '../../db/AreaTypes' import { Context } from '../../types' +import { validate } from 'uuid' -interface StructureQuery extends Partial<{ +interface StructureQuery { parent: MUUID filter: Partial<{ depth: number - }> -}> {} +} const AreaQueries = { cragsWithin: async (_, { filter }, { dataSources }: Context): Promise => { @@ -24,33 +24,11 @@ const AreaQueries = { structure: async (_, params: StructureQuery, { dataSources }: Context): Promise => { const { areas } = dataSources - const data: ShadowArea[] = [] - const roots: MUUID[] = [] - - console.time('structure query') - - if (params.parent !== undefined) { - // A parent has been specified so we can eval just that node - roots.push(params.parent) - } else { - roots.push(...(await areas.listAllCountries()).map(i => i.metadata.area_id)) + if (!(typeof params.parent === 'string' && validate(params.parent))) { + throw new Error('Malformed UUID string') } - // For each root that we're interested in, we want to scan accross - for (const root of roots) { - const area = await areas.findOneAreaByUUID(root) - const parent = area.ancestors.split(',').pop() - if (parent === undefined) continue - - data.push({ uuid: root, area_name: area.area_name, parent: muuid.from(parent) }) - // descendents takes care of its own look-ahead to make sure the query - // does not munch up stupid bandwidth - data.push(...(await areas.descendents(root))) - } - - console.timeEnd('structure query') - - return data + return await areas.descendents(muuid.from(params.parent)) } } diff --git a/src/graphql/schema/Area.gql b/src/graphql/schema/Area.gql index 392b2656..91c51e6b 100644 --- a/src/graphql/schema/Area.gql +++ b/src/graphql/schema/Area.gql @@ -11,7 +11,7 @@ type Query { ): [CragsNear] cragsWithin(filter: SearchWithinFilter): [Area] countries: [Area] - structure(parent: ID, filter: StructureQuery): [AreaShadow!]! + structure(parent: ID!, filter: StructureQuery): [AreaShadow!]! } "A climbing area, wall or crag" diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 84d18d8f..4935c249 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -1,5 +1,5 @@ import { MongoDataSource } from 'apollo-datasource-mongodb' -import { Filter } from 'mongodb' +import { Filter, Document } from 'mongodb' import muuid from 'uuid-mongodb' import bboxPolygon from '@turf/bbox-polygon' @@ -18,7 +18,15 @@ import { import { getClimbModel } from '../db/ClimbSchema.js' import { ClimbGQLQueryType } from '../db/ClimbTypes.js' import { logger } from '../logger.js' -import { muuidToString } from '../utils/helpers.js' + +function shadowArea (doc: Document): ShadowArea { + return { + area_name: doc.area_name, + uuid: doc.uuid, + parent: doc.parent, + climbs: doc.climbs + } +} export default class AreaDataSource extends MongoDataSource { areaModel = getAreaModel() @@ -187,6 +195,7 @@ export default class AreaDataSource extends MongoDataSource { return await data.toArray() } + uuid /** * Get whole db stats * @returns @@ -277,29 +286,81 @@ export default class AreaDataSource extends MongoDataSource { return await this.areaModel.find(filter).lean() } - async descendents (of: muuid.MUUID, previous?: ShadowArea[]): Promise { - previous = previous ?? [] - - // All descendents - const regex = new RegExp(`\\b${muuidToString(of)}\\b`) - const cursor = this.collection.find({ ancestors: regex }) - - previous.push() - - let counter = 0 - return await cursor.map((i) => { - // if (counter > 1000) { - // throw new Error('Data volume exceeded. Try filtering data') - // } - const parentString = i.ancestors.split(',').at(-2) - let parent: muuid.MUUID | null = null - - if (parentString !== undefined) { - parent = muuid.from(parentString) + /** + * Using the child relations we can do a graph lookup and flatten that result. + * I've put a leniant timeout of 500ms on the query to encourage proper loading + * patterns from api users. + * + * The timeout is a heuristic, sufficiently fast hardware may munch up a fair quantity + * of memory, but the docs say that this should be 100mb in the worst case? + * https://www.mongodb.com/docs/manual/reference/operator/aggregation/graphLookup/#memory + * someone more familair with mongo may want to double check that. + */ + async descendents (ofArea: muuid.MUUID): Promise { + const cursor = this.collection.aggregate([ + { $match: { 'metadata.area_id': ofArea, _deleting: { $exists: false } } }, + { + $project: + { + _id: 1, + 'metadata.area_id': 1, + area_name: 1, + children: 1 + } + }, + { + $graphLookup: { + from: this.collection.collectionName, + startWith: '$_id', + connectFromField: 'children', + connectToField: '_id', + as: 'descendants' + } + }, + { + $unwind: { + path: '$descendants' + } + }, + { + $replaceRoot: + { + newRoot: '$descendants' + } + }, + // Sadly we need to duplicate work previously done to now look up the immediate parent of + // the area + { + $lookup: + { + from: 'areas', + localField: '_id', + foreignField: 'children', + as: 'parent' + } + }, + { + $addFields: + { + uuid: '$metadata.area_id', + parent: { + $first: '$parent.metadata.area_id' + } + } + }, + { + $project: + { + _id: 0, + uuid: 1, + area_name: 1, + parent: 1, + climbs: 1 + } } + ]) + .maxTimeMS(900) - counter += 1 - return { area_name: i.area_name, uuid: i.metadata.area_id, parent } satisfies ShadowArea - }).toArray() as ShadowArea[] + return await cursor.map(shadowArea).toArray() } } From 9c58cfe1ba103397aaa81ab6e63d58b279d02c9f Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Mon, 4 Nov 2024 17:59:10 +0200 Subject: [PATCH 10/27] minor speed improvement to the query achieved by pruning the data being passed through the pipeline --- src/auth/local-dev/middleware.ts | 13 ++++ src/graphql/area/AreaQueries.ts | 16 +++-- src/graphql/gql-parse.ts | 20 ++++++ src/model/AreaDataSource.ts | 118 +++++++++++++++++++------------ src/server.ts | 11 ++- 5 files changed, 124 insertions(+), 54 deletions(-) create mode 100644 src/graphql/gql-parse.ts diff --git a/src/auth/local-dev/middleware.ts b/src/auth/local-dev/middleware.ts index 55898df6..4ab12d72 100644 --- a/src/auth/local-dev/middleware.ts +++ b/src/auth/local-dev/middleware.ts @@ -5,6 +5,7 @@ import muuid, { MUUID } from 'uuid-mongodb' import { AuthUserType } from '../../types.js' import { logger } from '../../logger.js' +import type { ApolloServerPlugin } from 'apollo-server-plugin-base' export const localDevBypassAuthContext = (() => { const testUUID: MUUID = muuid.v4() @@ -19,3 +20,15 @@ export const localDevBypassAuthContext = (() => { return { user } } })() + +export const loggerPlugin: ApolloServerPlugin = { + async requestDidStart (ctx) { + const startTime = process.uptime() + return { + async willSendResponse () { + ctx.logger.info(ctx.request.query) + ctx.logger.info(`query resolved in ${((process.uptime() - startTime) * 1000).toFixed(4)}ms`) + } + } + } +} diff --git a/src/graphql/area/AreaQueries.ts b/src/graphql/area/AreaQueries.ts index 10560462..5e4006ec 100644 --- a/src/graphql/area/AreaQueries.ts +++ b/src/graphql/area/AreaQueries.ts @@ -2,15 +2,16 @@ import muuid, { MUUID } from 'uuid-mongodb' import { AreaType, ShadowArea } from '../../db/AreaTypes' import { Context } from '../../types' import { validate } from 'uuid' +import { IResolverObject } from 'graphql-middleware/dist/types' +import { flatFieldSet } from '../gql-parse.js' +import { DescendantQuery } from '../../model/AreaDataSource' interface StructureQuery { parent: MUUID - filter: Partial<{ - depth: number - }> + filter: Partial } -const AreaQueries = { +const AreaQueries: IResolverObject = { cragsWithin: async (_, { filter }, { dataSources }: Context): Promise => { const { areas } = dataSources const { bbox, zoom } = filter @@ -22,13 +23,16 @@ const AreaQueries = { return await areas.listAllCountries() }, - structure: async (_, params: StructureQuery, { dataSources }: Context): Promise => { + structure: async (_, params: StructureQuery, { dataSources }: Context, info): Promise => { const { areas } = dataSources if (!(typeof params.parent === 'string' && validate(params.parent))) { throw new Error('Malformed UUID string') } - return await areas.descendents(muuid.from(params.parent)) + return await areas.descendants(muuid.from(params.parent), { + projection: flatFieldSet(info)[0], + filter: params.filter + }) } } diff --git a/src/graphql/gql-parse.ts b/src/graphql/gql-parse.ts new file mode 100644 index 00000000..8a722d61 --- /dev/null +++ b/src/graphql/gql-parse.ts @@ -0,0 +1,20 @@ +import { FieldNode, GraphQLResolveInfo, Kind, SelectionNode } from 'graphql' + +function selectNode (node: SelectionNode, type: T['kind']): node is T { + return node.kind === type +} + +function selector (type) { + return (node) => selectNode(node, type) +} + +function simpleFields (nodes: GraphQLResolveInfo['fieldNodes']): FieldNode[][] { + return nodes.map(set => set.selectionSet?.selections.filter(selector(Kind.FIELD)) as FieldNode[] ?? []) +} + +export function flatFieldSet (info: GraphQLResolveInfo): Array> { + return simpleFields(info.fieldNodes) + .map(set => set.reduce((acc, { name }) => + ({ ...acc, [name.value]: true }), {} + )) +} diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 4935c249..30147417 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -4,7 +4,7 @@ import muuid from 'uuid-mongodb' import bboxPolygon from '@turf/bbox-polygon' import { getAreaModel, getMediaObjectModel } from '../db/index.js' -import { AreaType, ShadowArea } from '../db/AreaTypes' +import { AreaType, IAreaProps, ShadowArea } from '../db/AreaTypes' import { AreaFilterParams, BBoxType, @@ -19,15 +19,6 @@ import { getClimbModel } from '../db/ClimbSchema.js' import { ClimbGQLQueryType } from '../db/ClimbTypes.js' import { logger } from '../logger.js' -function shadowArea (doc: Document): ShadowArea { - return { - area_name: doc.area_name, - uuid: doc.uuid, - parent: doc.parent, - climbs: doc.climbs - } -} - export default class AreaDataSource extends MongoDataSource { areaModel = getAreaModel() climbModel = getClimbModel() @@ -296,17 +287,32 @@ export default class AreaDataSource extends MongoDataSource { * https://www.mongodb.com/docs/manual/reference/operator/aggregation/graphLookup/#memory * someone more familair with mongo may want to double check that. */ - async descendents (ofArea: muuid.MUUID): Promise { - const cursor = this.collection.aggregate([ + async descendants (ofArea: muuid.MUUID, filter?: { + projection?: Record, boolean> + filter?: Partial + }): Promise { + function shadowArea (doc: Document): ShadowArea { + return { + area_name: doc.area_name, + uuid: doc.uuid, + parent: doc.parent, + climbs: doc.climbs + } + } + + const pipeline: Document[] = [ { $match: { 'metadata.area_id': ofArea, _deleting: { $exists: false } } }, { $project: - { - _id: 1, - 'metadata.area_id': 1, - area_name: 1, - children: 1 - } + { + // We need these two fields to make the structure query, + // all else are optional. + _id: 1, + children: 1, + + 'metadata.area_id': filter?.projection?.uuid, + ...filter?.projection + } }, { $graphLookup: { @@ -314,7 +320,13 @@ export default class AreaDataSource extends MongoDataSource { startWith: '$_id', connectFromField: 'children', connectToField: '_id', - as: 'descendants' + as: 'descendants', + // We can pass in a max depth if it is supplied to us. + ...(typeof filter?.filter?.maxDepth === 'number' + ? { + maxDepth: filter?.filter?.maxDepth + } + : {}) } }, { @@ -324,43 +336,57 @@ export default class AreaDataSource extends MongoDataSource { }, { $replaceRoot: - { - newRoot: '$descendants' - } + { + newRoot: '$descendants' + } }, - // Sadly we need to duplicate work previously done to now look up the immediate parent of - // the area { - $lookup: + $project: + { + // We need these two fields to make the structure query, + // all else are optional. + _id: 1, + 'metadata.area_id': filter?.projection?.uuid, + ...filter?.projection + } + } + ] + + if (filter?.projection?.parent ?? false) { + pipeline.push( + // Sadly we need to duplicate work previously done to now look up the immediate parent of + // the area + { + $lookup: { from: 'areas', localField: '_id', foreignField: 'children', as: 'parent' } - }, - { - $addFields: - { - uuid: '$metadata.area_id', - parent: { - $first: '$parent.metadata.area_id' - } - } - }, + } + ) + } + + pipeline.push({ + $addFields: { - $project: - { - _id: 0, - uuid: 1, - area_name: 1, - parent: 1, - climbs: 1 - } + uuid: '$metadata.area_id', + parent: { + $first: '$parent.metadata.area_id' + } } - ]) - .maxTimeMS(900) + }) - return await cursor.map(shadowArea).toArray() + return await this + .collection + .aggregate(pipeline) + .maxTimeMS(900) + .map(shadowArea) + .toArray() } } + +export interface DescendantQuery { + maxDepth: number +} diff --git a/src/server.ts b/src/server.ts index a3548d91..d21ccb0d 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,7 +10,7 @@ import MutableClimbDataSource from './model/MutableClimbDataSource.js' import TickDataSource from './model/TickDataSource.js' import { createContext } from './auth/middleware.js' import permissions from './auth/permissions.js' -import { localDevBypassAuthContext } from './auth/local-dev/middleware.js' +import { localDevBypassAuthContext, loggerPlugin as profilerPlugin } from './auth/local-dev/middleware.js' import localDevBypassAuthPermissions from './auth/local-dev/permissions.js' import XMediaDataSource from './model/XMediaDataSource.js' import PostDataSource from './model/PostDataSource.js' @@ -51,7 +51,14 @@ export async function createServer (): Promise<{ app: express.Application, serve schema, context: process.env.LOCAL_DEV_BYPASS_AUTH === 'true' ? localDevBypassAuthContext : createContext, dataSources, - cache: 'bounded' + cache: 'bounded', + // this could be extracted into a seperate settings flag, + // but should not make it into production. + plugins: process.env.LOCAL_DEV_BYPASS_AUTH === 'true' + ? [ + profilerPlugin + ] + : undefined }) // server must be started before applying middleware await server.start() From c72f95e6367ad33ac27407433fb871d018e04809 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Mon, 4 Nov 2024 17:59:10 +0200 Subject: [PATCH 11/27] minor speed improvement to the query achieved by pruning the data being passed through the pipeline --- src/__tests__/areas.ts | 2 +- src/auth/local-dev/middleware.ts | 13 ++++ src/graphql/area/AreaQueries.ts | 16 +++-- src/graphql/gql-parse.ts | 20 ++++++ src/graphql/schema/Area.gql | 2 +- src/model/AreaDataSource.ts | 118 +++++++++++++++++++------------ src/server.ts | 11 ++- 7 files changed, 126 insertions(+), 56 deletions(-) create mode 100644 src/graphql/gql-parse.ts diff --git a/src/__tests__/areas.ts b/src/__tests__/areas.ts index 35b59573..05b4ec0e 100644 --- a/src/__tests__/areas.ts +++ b/src/__tests__/areas.ts @@ -120,7 +120,7 @@ describe('areas API', () => { } ` - it('retrieves traversal of all roots using auto-depth', async () => { + it('retrieves ', async () => { const response = await queryAPI({ query: structureQuery, operationName: 'structure', diff --git a/src/auth/local-dev/middleware.ts b/src/auth/local-dev/middleware.ts index 55898df6..4ab12d72 100644 --- a/src/auth/local-dev/middleware.ts +++ b/src/auth/local-dev/middleware.ts @@ -5,6 +5,7 @@ import muuid, { MUUID } from 'uuid-mongodb' import { AuthUserType } from '../../types.js' import { logger } from '../../logger.js' +import type { ApolloServerPlugin } from 'apollo-server-plugin-base' export const localDevBypassAuthContext = (() => { const testUUID: MUUID = muuid.v4() @@ -19,3 +20,15 @@ export const localDevBypassAuthContext = (() => { return { user } } })() + +export const loggerPlugin: ApolloServerPlugin = { + async requestDidStart (ctx) { + const startTime = process.uptime() + return { + async willSendResponse () { + ctx.logger.info(ctx.request.query) + ctx.logger.info(`query resolved in ${((process.uptime() - startTime) * 1000).toFixed(4)}ms`) + } + } + } +} diff --git a/src/graphql/area/AreaQueries.ts b/src/graphql/area/AreaQueries.ts index 10560462..5e4006ec 100644 --- a/src/graphql/area/AreaQueries.ts +++ b/src/graphql/area/AreaQueries.ts @@ -2,15 +2,16 @@ import muuid, { MUUID } from 'uuid-mongodb' import { AreaType, ShadowArea } from '../../db/AreaTypes' import { Context } from '../../types' import { validate } from 'uuid' +import { IResolverObject } from 'graphql-middleware/dist/types' +import { flatFieldSet } from '../gql-parse.js' +import { DescendantQuery } from '../../model/AreaDataSource' interface StructureQuery { parent: MUUID - filter: Partial<{ - depth: number - }> + filter: Partial } -const AreaQueries = { +const AreaQueries: IResolverObject = { cragsWithin: async (_, { filter }, { dataSources }: Context): Promise => { const { areas } = dataSources const { bbox, zoom } = filter @@ -22,13 +23,16 @@ const AreaQueries = { return await areas.listAllCountries() }, - structure: async (_, params: StructureQuery, { dataSources }: Context): Promise => { + structure: async (_, params: StructureQuery, { dataSources }: Context, info): Promise => { const { areas } = dataSources if (!(typeof params.parent === 'string' && validate(params.parent))) { throw new Error('Malformed UUID string') } - return await areas.descendents(muuid.from(params.parent)) + return await areas.descendants(muuid.from(params.parent), { + projection: flatFieldSet(info)[0], + filter: params.filter + }) } } diff --git a/src/graphql/gql-parse.ts b/src/graphql/gql-parse.ts new file mode 100644 index 00000000..8a722d61 --- /dev/null +++ b/src/graphql/gql-parse.ts @@ -0,0 +1,20 @@ +import { FieldNode, GraphQLResolveInfo, Kind, SelectionNode } from 'graphql' + +function selectNode (node: SelectionNode, type: T['kind']): node is T { + return node.kind === type +} + +function selector (type) { + return (node) => selectNode(node, type) +} + +function simpleFields (nodes: GraphQLResolveInfo['fieldNodes']): FieldNode[][] { + return nodes.map(set => set.selectionSet?.selections.filter(selector(Kind.FIELD)) as FieldNode[] ?? []) +} + +export function flatFieldSet (info: GraphQLResolveInfo): Array> { + return simpleFields(info.fieldNodes) + .map(set => set.reduce((acc, { name }) => + ({ ...acc, [name.value]: true }), {} + )) +} diff --git a/src/graphql/schema/Area.gql b/src/graphql/schema/Area.gql index 91c51e6b..a74b06b3 100644 --- a/src/graphql/schema/Area.gql +++ b/src/graphql/schema/Area.gql @@ -191,7 +191,7 @@ query will cut off if it looks like it's about to deliver some rediculous number records. """ input StructureQuery { - depth: Int + maxDepth: Int } enum Field { diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 4935c249..30147417 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -4,7 +4,7 @@ import muuid from 'uuid-mongodb' import bboxPolygon from '@turf/bbox-polygon' import { getAreaModel, getMediaObjectModel } from '../db/index.js' -import { AreaType, ShadowArea } from '../db/AreaTypes' +import { AreaType, IAreaProps, ShadowArea } from '../db/AreaTypes' import { AreaFilterParams, BBoxType, @@ -19,15 +19,6 @@ import { getClimbModel } from '../db/ClimbSchema.js' import { ClimbGQLQueryType } from '../db/ClimbTypes.js' import { logger } from '../logger.js' -function shadowArea (doc: Document): ShadowArea { - return { - area_name: doc.area_name, - uuid: doc.uuid, - parent: doc.parent, - climbs: doc.climbs - } -} - export default class AreaDataSource extends MongoDataSource { areaModel = getAreaModel() climbModel = getClimbModel() @@ -296,17 +287,32 @@ export default class AreaDataSource extends MongoDataSource { * https://www.mongodb.com/docs/manual/reference/operator/aggregation/graphLookup/#memory * someone more familair with mongo may want to double check that. */ - async descendents (ofArea: muuid.MUUID): Promise { - const cursor = this.collection.aggregate([ + async descendants (ofArea: muuid.MUUID, filter?: { + projection?: Record, boolean> + filter?: Partial + }): Promise { + function shadowArea (doc: Document): ShadowArea { + return { + area_name: doc.area_name, + uuid: doc.uuid, + parent: doc.parent, + climbs: doc.climbs + } + } + + const pipeline: Document[] = [ { $match: { 'metadata.area_id': ofArea, _deleting: { $exists: false } } }, { $project: - { - _id: 1, - 'metadata.area_id': 1, - area_name: 1, - children: 1 - } + { + // We need these two fields to make the structure query, + // all else are optional. + _id: 1, + children: 1, + + 'metadata.area_id': filter?.projection?.uuid, + ...filter?.projection + } }, { $graphLookup: { @@ -314,7 +320,13 @@ export default class AreaDataSource extends MongoDataSource { startWith: '$_id', connectFromField: 'children', connectToField: '_id', - as: 'descendants' + as: 'descendants', + // We can pass in a max depth if it is supplied to us. + ...(typeof filter?.filter?.maxDepth === 'number' + ? { + maxDepth: filter?.filter?.maxDepth + } + : {}) } }, { @@ -324,43 +336,57 @@ export default class AreaDataSource extends MongoDataSource { }, { $replaceRoot: - { - newRoot: '$descendants' - } + { + newRoot: '$descendants' + } }, - // Sadly we need to duplicate work previously done to now look up the immediate parent of - // the area { - $lookup: + $project: + { + // We need these two fields to make the structure query, + // all else are optional. + _id: 1, + 'metadata.area_id': filter?.projection?.uuid, + ...filter?.projection + } + } + ] + + if (filter?.projection?.parent ?? false) { + pipeline.push( + // Sadly we need to duplicate work previously done to now look up the immediate parent of + // the area + { + $lookup: { from: 'areas', localField: '_id', foreignField: 'children', as: 'parent' } - }, - { - $addFields: - { - uuid: '$metadata.area_id', - parent: { - $first: '$parent.metadata.area_id' - } - } - }, + } + ) + } + + pipeline.push({ + $addFields: { - $project: - { - _id: 0, - uuid: 1, - area_name: 1, - parent: 1, - climbs: 1 - } + uuid: '$metadata.area_id', + parent: { + $first: '$parent.metadata.area_id' + } } - ]) - .maxTimeMS(900) + }) - return await cursor.map(shadowArea).toArray() + return await this + .collection + .aggregate(pipeline) + .maxTimeMS(900) + .map(shadowArea) + .toArray() } } + +export interface DescendantQuery { + maxDepth: number +} diff --git a/src/server.ts b/src/server.ts index a3548d91..d21ccb0d 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,7 +10,7 @@ import MutableClimbDataSource from './model/MutableClimbDataSource.js' import TickDataSource from './model/TickDataSource.js' import { createContext } from './auth/middleware.js' import permissions from './auth/permissions.js' -import { localDevBypassAuthContext } from './auth/local-dev/middleware.js' +import { localDevBypassAuthContext, loggerPlugin as profilerPlugin } from './auth/local-dev/middleware.js' import localDevBypassAuthPermissions from './auth/local-dev/permissions.js' import XMediaDataSource from './model/XMediaDataSource.js' import PostDataSource from './model/PostDataSource.js' @@ -51,7 +51,14 @@ export async function createServer (): Promise<{ app: express.Application, serve schema, context: process.env.LOCAL_DEV_BYPASS_AUTH === 'true' ? localDevBypassAuthContext : createContext, dataSources, - cache: 'bounded' + cache: 'bounded', + // this could be extracted into a seperate settings flag, + // but should not make it into production. + plugins: process.env.LOCAL_DEV_BYPASS_AUTH === 'true' + ? [ + profilerPlugin + ] + : undefined }) // server must be started before applying middleware await server.start() From 198d9068f5575a79bbfe6e18ee1c45198c127255 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Mon, 4 Nov 2024 21:52:59 +0200 Subject: [PATCH 12/27] Tests added and param options implemented The only way I can think of to speed this up is to eliminate the parent lookup stage in the pipeline, which seems to double the time the query takes to run until completion. The natural way to address this would be to embed a parent pointer - which I assume should be easy enough since there must be existing code to maintain the integrity of the pathTokens, ancestors So I'll take a look at doing that too --- src/__tests__/areas.ts | 62 ++++++++++++++++++++------------- src/graphql/area/AreaQueries.ts | 7 ++++ src/graphql/schema/Area.gql | 2 +- src/model/AreaDataSource.ts | 20 ++++++++--- 4 files changed, 62 insertions(+), 29 deletions(-) diff --git a/src/__tests__/areas.ts b/src/__tests__/areas.ts index 05b4ec0e..e57b11f7 100644 --- a/src/__tests__/areas.ts +++ b/src/__tests__/areas.ts @@ -1,5 +1,5 @@ import { ApolloServer } from 'apollo-server-express' -import muuid from 'uuid-mongodb' +import muuid, { MUUID } from 'uuid-mongodb' import { jest } from '@jest/globals' import MutableAreaDataSource from '../model/MutableAreaDataSource.js' import MutableOrganizationDataSource from '../model/MutableOrganizationDataSource.js' @@ -110,49 +110,63 @@ describe('areas API', () => { describe('area structure API', () => { const structureQuery = ` - query structure($parent: ID) { - structure(uuid: $parent) { - uuid - organizations { - orgId + query structure($parent: ID!) { + structure(parent: $parent) { + parent + uuid + area_name + climbs } } - } ` - it('retrieves ', async () => { + // Structure queries do not do write operations so we can build this once + beforeEach(async () => { + const maxDepth = 4 + const maxBreadth = 3 + + // So for the purposes of this test we will do a simple tree + async function grow (from: MUUID, depth: number = 0): Promise { + if (depth >= maxDepth) return + for (const idx of Array.from({ length: maxBreadth }, (_, i) => i + 1)) { + const newArea = await areas.addArea(user, `${depth}-${idx}`, from) + await grow(newArea.metadata.area_id, depth + 1) + } + } + + await grow(usa.metadata.area_id) + }) + + it('retrieves the structure of a given area', async () => { const response = await queryAPI({ query: structureQuery, operationName: 'structure', + variables: { parent: usa.metadata.area_id }, userUuid, app }) expect(response.statusCode).toBe(200) - const areaResult = response.body.data.area - expect(areaResult.uuid).toBe(muuidToString(ca.metadata.area_id)) - // Even though alphaOrg associates with ca's parent, usa, it excludes - // ca and so should not be listed. - expect(areaResult.organizations).toHaveLength(0) }) - it('retrieves traversal of a high level root using auto-depth', async () => { + it('should allow no parent to be supplied and get a shallow result', async () => { const response = await queryAPI({ - query: structureQuery, + query: ` + query structure { + structure { + parent + uuid + area_name + climbs + } + } + `, operationName: 'structure', - // Pass the usa top-level countru area - variables: { input: usa.metadata.area_id }, userUuid, app }) + expect(response.statusCode).toBe(200) - const areaResult = response.body.data.area - expect(areaResult.uuid).toBe(muuidToString(ca.metadata.area_id)) - // Even though alphaOrg associates with ca's parent, usa, it excludes - // ca and so should not be listed. - expect(areaResult.organizations).toHaveLength(0) }) - - it('failure to properly constrain depth', async () => {}) }) }) diff --git a/src/graphql/area/AreaQueries.ts b/src/graphql/area/AreaQueries.ts index 5e4006ec..4342e583 100644 --- a/src/graphql/area/AreaQueries.ts +++ b/src/graphql/area/AreaQueries.ts @@ -25,6 +25,13 @@ const AreaQueries: IResolverObject = { structure: async (_, params: StructureQuery, { dataSources }: Context, info): Promise => { const { areas } = dataSources + if (params.parent === undefined) { + return await areas.descendants(undefined, { + projection: flatFieldSet(info)[0], + filter: { ...params.filter, maxDepth: 2 } + }) + } + if (!(typeof params.parent === 'string' && validate(params.parent))) { throw new Error('Malformed UUID string') } diff --git a/src/graphql/schema/Area.gql b/src/graphql/schema/Area.gql index a74b06b3..0623fb8b 100644 --- a/src/graphql/schema/Area.gql +++ b/src/graphql/schema/Area.gql @@ -11,7 +11,7 @@ type Query { ): [CragsNear] cragsWithin(filter: SearchWithinFilter): [Area] countries: [Area] - structure(parent: ID!, filter: StructureQuery): [AreaShadow!]! + structure(parent: ID, filter: StructureQuery): [AreaShadow!]! } "A climbing area, wall or crag" diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 30147417..12157615 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -287,7 +287,7 @@ export default class AreaDataSource extends MongoDataSource { * https://www.mongodb.com/docs/manual/reference/operator/aggregation/graphLookup/#memory * someone more familair with mongo may want to double check that. */ - async descendants (ofArea: muuid.MUUID, filter?: { + async descendants (ofArea?: muuid.MUUID, filter?: { projection?: Record, boolean> filter?: Partial }): Promise { @@ -300,8 +300,20 @@ export default class AreaDataSource extends MongoDataSource { } } - const pipeline: Document[] = [ - { $match: { 'metadata.area_id': ofArea, _deleting: { $exists: false } } }, + const pipeline: Document[] = [] + + if (ofArea === undefined) { + // in this case we can filter on the max depth + } + + pipeline.push(...[ + { + $match: { + ...(ofArea !== undefined ? { 'metadata.area_id': ofArea } : {}), + ...(filter?.filter?.maxDepth !== undefined ? { $expr: { $lte: [{ $size: '$pathTokens' }, filter?.filter?.maxDepth] } } : {}), + _deleting: { $exists: false } + } + }, { $project: { @@ -350,7 +362,7 @@ export default class AreaDataSource extends MongoDataSource { ...filter?.projection } } - ] + ]) if (filter?.projection?.parent ?? false) { pipeline.push( From 9551722865a1afc3627df7c644a2f1390d3692b8 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Thu, 7 Nov 2024 20:55:54 +0200 Subject: [PATCH 13/27] Add utility for using or creating a client session Rather than duping logic from other code units over and over --- src/utils/helpers.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 9ed4f044..4adc57fc 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -1,6 +1,6 @@ import { MUUID } from 'uuid-mongodb' import { Point } from '@turf/helpers' -import { ClientSession } from 'mongoose' +import { ClientSession, ClientSessionOptions } from 'mongoose' export const muuidToString = (m: MUUID): string => m.toUUID().toString() @@ -36,3 +36,24 @@ export const withTransaction = async (session: ClientSession, closure: () => }) return result } + +interface SessionStartable { + startSession: (options?: ClientSessionOptions) => Promise +} + +export const useOrCreateTransaction = async(owner: SessionStartable, session: ClientSession | undefined, closure: (session: ClientSession) => Promise): Promise => { + const reifiedSession = session ?? await owner.startSession() + + try { + if (reifiedSession.inTransaction()) { + return await closure(reifiedSession) + } else { + return await withTransaction(reifiedSession, async () => await closure(reifiedSession)) + } + } finally { + // If the session was created in this context we can close it out. + if (session == null) { + await reifiedSession.endSession() + } + } +} From dfac7d1a9e1ba71a052c508c2776709a1a29cec8 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Fri, 8 Nov 2024 19:14:52 +0200 Subject: [PATCH 14/27] update script to allow 'mongosh' the name of mongo was changed to mongo a little while ago, and I can't be bothered to symlink the name, so now if mongo is not available the script will fallback onto mongosh. + Add mongosh to the nix shell --- migrate-db.sh | 19 ++++++++++++++++++- shell.nix | 1 + 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/migrate-db.sh b/migrate-db.sh index c11dc21c..c75c329b 100755 --- a/migrate-db.sh +++ b/migrate-db.sh @@ -6,8 +6,25 @@ then exit 1 fi +if [ ! -f $1 ] +then + echo "Specified migration file ($1) not found" + exit 1 +fi + . .env connStr="${MONGO_SCHEME}://${MONGO_INITDB_ROOT_USERNAME}:${MONGO_INITDB_ROOT_PASSWORD}@${MONGO_SERVICE}/${MONGO_DBNAME}?authSource=${MONGO_AUTHDB}&tls=${MONGO_TLS}&replicaSet=${MONGO_REPLICA_SET_NAME}" -mongo "$connStr" $1 +# Determine whether `mongo` or `mongosh` is available +if command -v mongosh &> /dev/null; then + mongoCmd="mongosh" + elif command -v mongo &> /dev/null; then + mongoCmd="mongo" +else + echo "Neither mongosh nor mongo command found. Please install one of them." + exit 1 +fi + +# Execute the chosen command with the connection string and migration file +"$mongoCmd" "$connStr" "$1" \ No newline at end of file diff --git a/shell.nix b/shell.nix index b7915e88..125512b4 100644 --- a/shell.nix +++ b/shell.nix @@ -11,6 +11,7 @@ pkgs.mkShell { yarn mongodb-ce mongodb-compass + mongosh ]; # MONGOMS_DOWNLOAD_URL = "https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu2404-8.0.1.tgz"; From 53e0b801dba3d9502a6ed980a5c94e08851be68c Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Fri, 8 Nov 2024 20:42:43 +0200 Subject: [PATCH 15/27] Migrate to new data model The tests are not passing at this commit, even though TSC is not reporting type issues. I will try and correct whatever is preventing the types from being reported. --- db-migrations/0006-area-structure.js | 95 +++++++++++++++++++ src/db/AreaSchema.ts | 47 +++++++-- src/db/AreaTypes.ts | 54 +++++++---- src/db/export/Typesense/Client.ts | 2 +- src/db/export/Typesense/transformers.ts | 2 +- src/db/export/json/area.resolver.test.ts | 2 +- src/db/export/json/area.resolver.ts | 4 +- src/db/import/usa/AreaTransformer.ts | 9 +- src/db/utils/jobs/MapTiles/exportCmd.ts | 4 +- .../utils/jobs/TreeUpdaters/updateAllAreas.ts | 6 +- src/graphql/resolvers.ts | 6 +- src/model/AreaDataSource.ts | 4 +- src/model/AreaRelationsEmbeddings.ts | 39 ++++++++ src/model/MutableAreaDataSource.ts | 38 ++++---- src/model/MutableClimbDataSource.ts | 2 +- src/model/MutableMediaDataSource.ts | 4 +- src/model/__tests__/AreaHistoryDataSource.ts | 4 +- src/model/__tests__/AreaUtils.ts | 1 + src/model/__tests__/BulkDataSource.test.ts | 12 +-- src/model/__tests__/MediaDataSource.ts | 2 +- .../__tests__/MutableAreaDataSource.test.ts | 4 +- src/model/__tests__/updateAreas.ts | 26 ++--- 22 files changed, 279 insertions(+), 88 deletions(-) create mode 100644 db-migrations/0006-area-structure.js create mode 100644 src/model/AreaRelationsEmbeddings.ts diff --git a/db-migrations/0006-area-structure.js b/db-migrations/0006-area-structure.js new file mode 100644 index 00000000..5f09958d --- /dev/null +++ b/db-migrations/0006-area-structure.js @@ -0,0 +1,95 @@ +// For each document that has children, we want to tell its children to back-link to us. +db.areas + .find({ children: { $exists: true, $type: "array" } }) + .forEach((parentDoc) => + db.areas.updateMany( + { _id: { $in: parentDoc.children } }, + { $set: { parent: parentDoc._id } }, + ), + ); + +// Pre-fetch children for all documents to avoid querying in the loop +const allChildren = db.areas.aggregate([ + { $match: { parent: { $exists: true } } }, + { $group: { _id: "$parent", children: { $push: "$_id" } } } +]).toArray(); + +// hold a reference to the children in memory +const childrenMap = allChildren.reduce((map, item) => { + map[item._id] = item.children; + return map; +}, {}); + +// This stage will take a WHILE +db.areas.find().forEach((doc) => { + // Perform a $graphLookup aggregation to get the full ancestor path for our target + const pathDocs = db.areas.aggregate([ + { + $match: { _id: doc._id }, + }, + { + $graphLookup: { + from: "areas", + startWith: "$parent", + connectFromField: "parent", + connectToField: "_id", + as: "ancestorPath", + depthField: "depth", + }, + }, + { + $unwind: "$ancestorPath", + }, + { + $sort: { "ancestorPath.depth": -1 }, + }, + { + $group: { + _id: "$_id", + pathTokens: { $push: "$ancestorPath.area_name" }, + ancestors: { $push: "$ancestorPath.metadata.area_id" }, + ancestorIndex: { $push: "$ancestorPath._id" }, + }, + }, + ]).toArray(); + + const pathTokens = [...(pathDocs[0]?.pathTokens ?? []), doc.area_name]; + const ancestors = [ + ...(pathDocs[0]?.ancestors ?? []), + doc.metadata.area_id, + ].join(","); + const ancestorIndex = pathDocs[0]?.ancestorIndex ?? []; + + const embeddedRelations = { + children: childrenMap[doc._id] || [], + pathTokens, + ancestors, + ancestorIndex, + }; + + if (pathTokens.join(",") !== doc.pathTokens.join(",")) { + throw `Path tokens did not match (${pathTokens} != ${doc.pathTokens})`; + } + + if (ancestors !== doc.ancestors) { + throw `Path tokens did not match (${ancestors} != ${doc.ancestors})`; + } + + if (ancestorIndex.length !== pathTokens.length - 1) { + print({ ancestorIndex, pathTokens }); + throw "ancestorIndex is the wrong shape"; + } + + // Use bulkWrite for efficient updates + db.areas.updateOne( + { _id: doc._id }, + { $set: { embeddedRelations } } + ); +}); + +print("Removing old fields."); + +// Remove the unneeded values since all ops have run without raising an assertion issue +db.areas.updateMany({}, { + $unset: { children: "", pathTokens: "", ancestors: "" }, +}); \ No newline at end of file diff --git a/src/db/AreaSchema.ts b/src/db/AreaSchema.ts index e6582248..4b2b236b 100644 --- a/src/db/AreaSchema.ts +++ b/src/db/AreaSchema.ts @@ -1,7 +1,7 @@ import mongoose from 'mongoose' import muuid from 'uuid-mongodb' -import { AreaType, IAreaContent, IAreaMetadata, AggregateType, CountByGroupType, CountByDisciplineType, CountByGradeBandType, DisciplineStatsType, OperationType } from './AreaTypes.js' +import { AreaType, IAreaContent, IAreaMetadata, AggregateType, CountByGroupType, CountByDisciplineType, CountByGradeBandType, DisciplineStatsType, OperationType, AreaEmbeddedRelations } from './AreaTypes.js' import { PointSchema } from './ClimbSchema.js' import { ChangeRecordMetadataType } from './ChangeLogType.js' import { GradeContexts } from '../GradeUtils.js' @@ -104,6 +104,37 @@ const AggregateSchema = new Schema({ byGradeBand: CountByGradeBandSchema }, { _id: false }) +/** + * + */ +const AreaEmbeddedRelationsSchema = new Schema({ + /** + * All child area documents that are contained within this area. + * This has a strong relation to the areas collection, and contains only direct + * child areas - rather than all descendents. + * + * computed from the remote documents parent field + */ + children: [{ type: Schema.Types.ObjectId, ref: 'areas', required: false, default: [], index: true }], + /** + * ancestors ids of this areas parents, traversing up the heirarchy to the root area. + * This is encoded as a string, but is really an array delimited by comma. + * @see https://www.mongodb.com/docs/manual/tutorial/model-tree-structures-with-materialized-paths/ + * + * computed from the remote documents parent field + */ + ancestors: { type: String, required: true, index: true }, + /** + * pathTokens names of this areas parents, traversing up the heirarchy to the root area + * with the current area being the last element. + */ + pathTokens: [{ type: String, required: true, index: true }], + /** + * Rather than doing a graph lookup, the ancestry can be traced from here. + */ + ancestorIndex: [{ type: Schema.Types.ObjectId, ref: 'areas', required: false, default: [], index: true }] +}, { _id: false }) + export const AreaSchema = new Schema({ area_name: { type: String, required: true, index: true }, shortCode: { type: String, required: false, index: true }, @@ -112,9 +143,13 @@ export const AreaSchema = new Schema({ ref: 'climbs', required: false }], - children: [{ type: Schema.Types.ObjectId, ref: 'areas', required: false }], - ancestors: { type: String, required: true, index: true }, - pathTokens: [{ type: String, required: true, index: true }], + parent: { + type: mongoose.Types.ObjectId, + ref: 'areas', + index: true, + validate: async function () {} + }, + embeddedRelations: AreaEmbeddedRelationsSchema, gradeContext: { type: String, enum: Object.values(GradeContexts), required: true }, aggregate: AggregateSchema, metadata: MetadataSchema, @@ -149,9 +184,5 @@ AreaSchema.index({ children: 1 }) -export const createAreaModel = (name: string = 'areas'): mongoose.Model => { - return connection.model(name, AreaSchema) -} - export const getAreaModel = (name: string = 'areas'): mongoose.Model => connection.model(name, AreaSchema) diff --git a/src/db/AreaTypes.ts b/src/db/AreaTypes.ts index 39be0ed3..3903624c 100644 --- a/src/db/AreaTypes.ts +++ b/src/db/AreaTypes.ts @@ -58,29 +58,21 @@ export interface IAreaProps extends AuthorMetadata { * unique and are subject to change. **/ area_name: string + + /** + * We currently only support a single parent for each area, this field is the source + * of truth that other embedded fields will be derived from. + */ + parent?: mongoose.Types.ObjectId + /** * The climbs that appear within this area (Only applies for leaf nodes). * Only areas that are permitted direct climb children will have these, and these * are conventionally not allowed to have area children. */ climbs: Array - /** - * All child area documents that are contained within this area. - * This has a strong relation to the areas collection, and contains only direct - * child areas - rather than all descendents. - */ - children: mongoose.Types.ObjectId[] - /** - * ancestors ids of this areas parents, traversing up the heirarchy to the root area. - * This is encoded as a string, but is really an array delimited by comma. - * @see https://www.mongodb.com/docs/manual/tutorial/model-tree-structures-with-materialized-paths/ - */ - ancestors: string - /** - * pathTokens names of this areas parents, traversing up the heirarchy to the root area - * with the current area being the last element. - */ - pathTokens: string[] + + embeddedRelations: AreaEmbeddedRelations gradeContext: GradeContexts /** @@ -116,6 +108,34 @@ export interface IAreaProps extends AuthorMetadata { _deleting?: Date } +export interface AreaEmbeddedRelations { + /** + * All child area documents that are contained within this area. + * This has a strong relation to the areas collection, and contains only direct + * child areas - rather than all descendents. + * + * computed from the remote documents parent field + */ + children: mongoose.Types.ObjectId[] + /** + * ancestors ids of this areas parents, traversing up the heirarchy to the root area. + * This is encoded as a string, but is really an array delimited by comma. + * @see https://www.mongodb.com/docs/manual/tutorial/model-tree-structures-with-materialized-paths/ + * + * computed from the remote documents parent field + */ + ancestors: string + /** + * Trace ancestors back to root, can be used as an index rather than computing + */ + ancestorIndex: mongoose.Types.ObjectId[] + /** + * pathTokens names of this areas parents, traversing up the heirarchy to the root area + * with the current area being the last element. + */ + pathTokens: string[] +} + export interface IAreaMetadata { isDestination: boolean /** diff --git a/src/db/export/Typesense/Client.ts b/src/db/export/Typesense/Client.ts index b2c357a4..2e0a9736 100644 --- a/src/db/export/Typesense/Client.ts +++ b/src/db/export/Typesense/Client.ts @@ -69,7 +69,7 @@ export const updateClimbIndex = async (climb: ClimbType | null, op: DBOperation) } // Look up additional attrs required by Climb index in Typesense. - const { pathTokens, ancestors } = await MutableAreaDataSource.getInstance().findOneAreaByUUID(climb.metadata.areaRef) + const { pathTokens, ancestors } = (await MutableAreaDataSource.getInstance().findOneAreaByUUID(climb.metadata.areaRef)).embeddedRelations const climbExt: ClimbExtType = { ...climb, diff --git a/src/db/export/Typesense/transformers.ts b/src/db/export/Typesense/transformers.ts index 7bc17511..79b4dbd3 100644 --- a/src/db/export/Typesense/transformers.ts +++ b/src/db/export/Typesense/transformers.ts @@ -13,7 +13,7 @@ export function mongoAreaToTypeSense (doc: AreaType): AreaTypeSenseItem { id: doc.metadata.area_id.toUUID().toString(), areaUUID: doc.metadata.area_id.toUUID().toString(), name: doc.area_name ?? '', - pathTokens: doc.pathTokens, + pathTokens: doc.embeddedRelations.pathTokens, areaLatLng: geoToLatLng(doc.metadata.lnglat), leaf: doc.metadata.leaf, isDestination: doc.metadata.isDestination, diff --git a/src/db/export/json/area.resolver.test.ts b/src/db/export/json/area.resolver.test.ts index c70fa532..0ce3bbb0 100644 --- a/src/db/export/json/area.resolver.test.ts +++ b/src/db/export/json/area.resolver.test.ts @@ -43,7 +43,7 @@ describe('area resolvers', () => { ] function assertSubPathResolver (path: string[], expected: string) { - expect(resolveAreaSubPath({ pathTokens: path })).toBe(expected) + expect(resolveAreaSubPath({ embeddedRelations: { pathTokens: path }})).toBe(expected) } testCases.forEach(testCase => { diff --git a/src/db/export/json/area.resolver.ts b/src/db/export/json/area.resolver.ts index 3bcfb7be..dd82d781 100644 --- a/src/db/export/json/area.resolver.ts +++ b/src/db/export/json/area.resolver.ts @@ -6,8 +6,8 @@ export function resolveAreaFileName (area: Partial): string { if (name === undefined || name === '') { return 'unknown' } else { return name } } -export function resolveAreaSubPath (area: Partial): string { - const paths: string[] = area.pathTokens?.map(normalizeName) +export function resolveAreaSubPath (area: Partial & { embeddedRelations: Partial }>): string { + const paths: string[] = area?.embeddedRelations?.pathTokens?.map(normalizeName) .map(token => token ?? '') .filter(token => token !== '') ?? [] return path.join(...paths) diff --git a/src/db/import/usa/AreaTransformer.ts b/src/db/import/usa/AreaTransformer.ts index 055b85ed..236bcb76 100644 --- a/src/db/import/usa/AreaTransformer.ts +++ b/src/db/import/usa/AreaTransformer.ts @@ -78,7 +78,6 @@ export const makeDBArea = (node: AreaNode): AreaType => { uuid, shortCode: '', area_name: areaName, - children: Array.from(children), metadata: { isDestination: false, leaf: isLeaf, @@ -88,9 +87,13 @@ export const makeDBArea = (node: AreaNode): AreaType => { leftRightIndex: -1, ext_id: isLeaf ? extractMpId(node.jsonLine.url) : '' }, - ancestors: uuidArrayToString(node.getAncestors()), climbs: [], - pathTokens: node.getPathTokens(), + embeddedRelations: { + children: Array.from(children), + pathTokens: node.getPathTokens(), + ancestors: uuidArrayToString(node.getAncestors()), + ancestorIndex: [] + }, gradeContext: node.getGradeContext(), aggregate: { byGrade: [], diff --git a/src/db/utils/jobs/MapTiles/exportCmd.ts b/src/db/utils/jobs/MapTiles/exportCmd.ts index cfa3288e..87b7929b 100644 --- a/src/db/utils/jobs/MapTiles/exportCmd.ts +++ b/src/db/utils/jobs/MapTiles/exportCmd.ts @@ -67,14 +67,14 @@ async function exportLeafCrags (): Promise { const { metadata, area_name: areaName, - pathTokens, - ancestors, content, gradeContext, climbs, totalClimbs } = doc + const { pathTokens, ancestors } = doc.embeddedRelations + const ancestorArray = ancestors.split(',') const pointFeature = point( doc.metadata.lnglat.coordinates, diff --git a/src/db/utils/jobs/TreeUpdaters/updateAllAreas.ts b/src/db/utils/jobs/TreeUpdaters/updateAllAreas.ts index e7ff4201..2eb9b2ed 100644 --- a/src/db/utils/jobs/TreeUpdaters/updateAllAreas.ts +++ b/src/db/utils/jobs/TreeUpdaters/updateAllAreas.ts @@ -49,7 +49,7 @@ export const updateAllAreas = async (): Promise => { for await (const countryNode of iterator) { const stateNodes = await countryNode.populate('children') const results = await Promise.all( - stateNodes.children.map(async entry => { + stateNodes.embeddedRelations.children.map(async entry => { const area: any = entry return leafReducer((area.toObject() as AreaType)) }) @@ -68,7 +68,7 @@ export interface StatsSummary { } async function postOrderVisit (node: AreaMongoType): Promise { - if (node.metadata.leaf || node.children.length === 0) { + if (node.metadata.leaf || node.embeddedRelations.children.length === 0) { return leafReducer((node.toObject() as AreaType)) } @@ -76,7 +76,7 @@ async function postOrderVisit (node: AreaMongoType): Promise { const nodeWithSubAreas = await node.populate('children') const results = await Promise.all( - nodeWithSubAreas.children.map(async entry => { + nodeWithSubAreas.embeddedRelations.children.map(async entry => { const area: any = entry /* eslint-disable-next-line */ return limiter(postOrderVisit, (area as AreaMongoType)) diff --git a/src/graphql/resolvers.ts b/src/graphql/resolvers.ts index 472e3b77..f83ed0da 100644 --- a/src/graphql/resolvers.ts +++ b/src/graphql/resolvers.ts @@ -212,8 +212,8 @@ const resolvers = { areaName: async (node: AreaType) => node.area_name, children: async (parent: AreaType, _: any, { dataSources: { areas } }: Context) => { - if (parent.children.length > 0) { - return await areas.findManyByIds(parent.children) + if (parent.embeddedRelations.children.length > 0) { + return await areas.findManyByIds(parent.embeddedRelations.children) } return [] }, @@ -270,7 +270,7 @@ const resolvers = { organizations: async (node: AreaType, args: any, { dataSources }: Context) => { const { organizations } = dataSources - const areaIdsToSearch = [node.metadata.area_id, ...node.ancestors.split(',').map(s => muid.from(s))] + const areaIdsToSearch = [node.metadata.area_id, ...node.embeddedRelations.ancestors.split(',').map(s => muid.from(s))] const associatedOrgsCursor = await organizations.findOrganizationsByFilter({ associatedAreaIds: { includes: areaIdsToSearch }, // Remove organizations that explicitly request not to be associated with this area. diff --git a/src/model/AreaDataSource.ts b/src/model/AreaDataSource.ts index 12157615..34d91f26 100644 --- a/src/model/AreaDataSource.ts +++ b/src/model/AreaDataSource.ts @@ -45,14 +45,14 @@ export default class AreaDataSource extends MongoDataSource { case 'path_tokens': { const pathFilter = filter as PathTokenParams if (pathFilter.exactMatch === true) { - acc.pathTokens = pathFilter.tokens + acc.embeddedRelations.pathTokens = pathFilter.tokens } else { const filter: Record = {} filter.$all = pathFilter.tokens if (pathFilter.size !== undefined) { filter.$size = pathFilter.size } - acc.pathTokens = filter + acc.embeddedRelations.pathTokens = filter } break } diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts new file mode 100644 index 00000000..22be6123 --- /dev/null +++ b/src/model/AreaRelationsEmbeddings.ts @@ -0,0 +1,39 @@ +import mongoose from 'mongoose' +import { getAreaModel } from '../db' + +export async function computeEmbeddedRelations (rootId: mongoose.Types.ObjectId): Promise { + const areaModel = getAreaModel() + const result = await areaModel.aggregate([ + { + $match: { _id: rootId } + }, + { + $graphLookup: { + from: 'areas', + startWith: '$parent', + connectFromField: 'parent', + connectToField: '_id', + as: 'computed_ancestors', + depthField: 'depth' + } + }, + { + $addFields: { + ancestorIndex: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor._id' } }, + pathTokens: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor.area_name' } }, + children: [], // Initialize empty children array + ancestors: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor.area_name' } } + } + }, + { + $project: { + ancestors: 1, + ancestorIndex: 1, + pathTokens: 1, + children: 1 + } + } + ]) + + throw new Error("not implemented yet") +} diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 8b12cb2b..a37b213d 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -63,7 +63,7 @@ export default class MutableAreaDataSource extends AreaDataSource { let neighbours: string[] if (parent !== null) { - neighbours = (await this.areaModel.find({ _id: parent.children })).map(i => i.area_name) + neighbours = (await this.areaModel.find({ _id: parent.embeddedRelations.children })).map(i => i.area_name) } else { neighbours = (await this.areaModel.find({ pathTokens: { $size: 1 } })).map(i => i.area_name) } @@ -209,7 +209,7 @@ export default class MutableAreaDataSource extends AreaDataSource { const parent = await this.areaModel.findOne(parentFilter).session(session).orFail(new UserInputError(`[${areaName}]: Expecting country or area parent, found none with id ${parentUuid.toString()}`)) if (parent.metadata.leaf || (parent.metadata?.isBoulder ?? false)) { - if (parent.children.length > 0 || parent.climbs.length > 0) { + if (parent.embeddedRelations.children.length > 0 || parent.climbs.length > 0) { throw new UserInputError(`[${areaName}]: Adding new areas to a leaf or boulder area is not allowed.`) } // No children. It's ok to continue turning an empty crag/boulder into an area. @@ -238,8 +238,8 @@ export default class MutableAreaDataSource extends AreaDataSource { draft.prevHistoryId = parent._change?.historyId }) - const parentAncestors = parent.ancestors - const parentPathTokens = parent.pathTokens + const parentAncestors = parent.embeddedRelations.ancestors + const parentPathTokens = parent.embeddedRelations.pathTokens const parentGradeContext = parent.gradeContext const newArea = newAreaHelper(areaName, parentAncestors, parentPathTokens, parentGradeContext) if (isLeaf != null) { @@ -262,7 +262,7 @@ export default class MutableAreaDataSource extends AreaDataSource { const rs1 = await this.areaModel.insertMany(newArea, { session }) // Make sure parent knows about this new area - parent.children.push(newArea._id) + parent.embeddedRelations.children.push(newArea._id) parent.updatedBy = experimentaAuthorId ?? user await parent.save({ timestamps: false }) return rs1[0].toObject() @@ -294,7 +294,7 @@ export default class MutableAreaDataSource extends AreaDataSource { throw new Error('Delete area error. Reason: area not found.') } - if (area?.children?.length > 0) { + if (area?.embeddedRelations.children?.length > 0) { throw new Error('Delete area error. Reason: subareas not empty.') } @@ -421,11 +421,11 @@ export default class MutableAreaDataSource extends AreaDataSource { area.set({ _change }) area.updatedBy = experimentalAuthorId ?? user - if (area.pathTokens.length === 1) { + if (area.embeddedRelations.pathTokens.length === 1) { if (areaName != null || shortCode != null) throw new Error(`[${area.area_name}]: Area update error. Reason: Updating country name or short code is not allowed.`) } - if (area.children.length > 0 && (isLeaf != null || isBoulder != null)) { + if (area.embeddedRelations.children.length > 0 && (isLeaf != null || isBoulder != null)) { throw new Error(`[${area.area_name}]: Area update error. Reason: Updating leaf or boulder status of an area with subareas is not allowed.`) } @@ -495,12 +495,12 @@ export default class MutableAreaDataSource extends AreaDataSource { * @param depth tree depth */ async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, changeIndex: number = -1): Promise { - if (area.pathTokens.length > 1) { + if (area.embeddedRelations.pathTokens.length > 1) { if (changeIndex === -1) { - changeIndex = area.pathTokens.length - 1 + changeIndex = area.embeddedRelations.pathTokens.length - 1 } - const newPath = [...area.pathTokens] + const newPath = [...area.embeddedRelations.pathTokens] newPath[changeIndex] = newAreaName area.set({ pathTokens: newPath }) area.set({ _change: changeRecord }) @@ -509,7 +509,7 @@ export default class MutableAreaDataSource extends AreaDataSource { // hydrate children_ids array with actual area documents await area.populate('children') - await Promise.all(area.children.map(async childArea => { + await Promise.all(area.embeddedRelations.children.map(async childArea => { // TS complains about ObjectId type // Fix this when we upgrade Mongoose library // @ts-expect-error @@ -585,12 +585,12 @@ export default class MutableAreaDataSource extends AreaDataSource { * Update function. For each node, recalculate stats and recursively update its acenstors until we reach the country node. */ const updateFn = async (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, childSummary?: StatsSummary): Promise => { - if (area.pathTokens.length <= 1) { + if (area.embeddedRelations.pathTokens.length <= 1) { // we're at the root country node return } - const ancestors = area.ancestors.split(',') + const ancestors = area.embeddedRelations.ancestors.split(',') const parentUuid = muuid.from(ancestors[ancestors.length - 2]) const parentArea = await this.areaModel.findOne({ 'metadata.area_id': parentUuid }) @@ -649,12 +649,12 @@ export const newAreaHelper = (areaName: string, parentAncestors: string, parentP }) const ancestors = parentAncestors + ',' + uuid.toUUID().toString() + return { _id, uuid, shortCode: '', area_name: areaName, - children: [], metadata: { isDestination: false, leaf: false, @@ -664,9 +664,13 @@ export const newAreaHelper = (areaName: string, parentAncestors: string, parentP bbox: undefined, polygon: undefined }, - ancestors, climbs: [], - pathTokens, + embeddedRelations: { + pathTokens, + ancestors, + children: [], + ancestorIndex: [] + }, gradeContext: parentGradeContext, aggregate: { byGrade: [], diff --git a/src/model/MutableClimbDataSource.ts b/src/model/MutableClimbDataSource.ts index 4147a53e..8ce63c25 100644 --- a/src/model/MutableClimbDataSource.ts +++ b/src/model/MutableClimbDataSource.ts @@ -78,7 +78,7 @@ export default class MutableClimbDataSource extends ClimbDataSource { parent.set({ _change }) // does the parent area have subareas? - if (parent.children.length > 0) { + if (parent.embeddedRelations.children.length > 0) { throw new UserInputError('You can only add climbs to a crag or a bouldering area (an area that doesn\'t contain other areas)') } diff --git a/src/model/MutableMediaDataSource.ts b/src/model/MutableMediaDataSource.ts index 011420fd..d9bddfc5 100644 --- a/src/model/MutableMediaDataSource.ts +++ b/src/model/MutableMediaDataSource.ts @@ -25,7 +25,7 @@ export default class MutableMediaDataSource extends MediaDataSource { _id: new mongoose.Types.ObjectId(), targetId: entityUuid, type: entityType, - ancestors: climb.parent.ancestors, + ancestors: climb.parent.embeddedRelations.ancestors, climbName: climb.name, areaName: climb.parent.area_name, lnglat: climb.metadata.lnglat @@ -47,7 +47,7 @@ export default class MutableMediaDataSource extends MediaDataSource { _id: new mongoose.Types.ObjectId(), targetId: entityUuid, type: entityType, - ancestors: area.ancestors, + ancestors: area.embeddedRelations.ancestors, areaName: area.area_name, lnglat: area.metadata.lnglat } diff --git a/src/model/__tests__/AreaHistoryDataSource.ts b/src/model/__tests__/AreaHistoryDataSource.ts index e4fbaf6f..62c75988 100644 --- a/src/model/__tests__/AreaHistoryDataSource.ts +++ b/src/model/__tests__/AreaHistoryDataSource.ts @@ -67,8 +67,8 @@ describe('Area history', () => { expect(nvAreaHistory[1].dbOp).toEqual('update') // add area to country.children[] expect(nvAreaHistory[1].fullDocument.area_name).toEqual(usa?.area_name) - expect(nvAreaHistory[1].fullDocument.children).toHaveLength(2) - expect(nvAreaHistory[1].fullDocument.children[1]).toEqual(nv?._id) // area added to parent.children[]? + expect(nvAreaHistory[1].fullDocument.embeddedRelations.children).toHaveLength(2) + expect(nvAreaHistory[1].fullDocument.embeddedRelations.children[1]).toEqual(nv?._id) // area added to parent.children[]? // verify change history linking // 2nd change record: parent (country) diff --git a/src/model/__tests__/AreaUtils.ts b/src/model/__tests__/AreaUtils.ts index 0f30849b..354943f5 100644 --- a/src/model/__tests__/AreaUtils.ts +++ b/src/model/__tests__/AreaUtils.ts @@ -1,4 +1,5 @@ describe('Test area utilities', () => { test.todo('The name comparison code unit') test.todo('The name-uniqueness system with other side-effects stripped out') + test.todo('normalizeName should also get a test, and possibly even merge.') }) diff --git a/src/model/__tests__/BulkDataSource.test.ts b/src/model/__tests__/BulkDataSource.test.ts index 9fd83866..0603f4ff 100644 --- a/src/model/__tests__/BulkDataSource.test.ts +++ b/src/model/__tests__/BulkDataSource.test.ts @@ -144,26 +144,26 @@ describe('bulk import e2e', () => { addedAreas: [ { area_name: 'Test Area', - pathTokens: ['United States of America', 'Test Area'], + embeddedRelations: { pathTokens: ['United States of America', 'Test Area'] }, }, { area_name: 'Test Area 2', - pathTokens: [ + embeddedRelations: { pathTokens: [ 'United States of America', 'Test Area', 'Test Area 2', - ], + ] }, }, { area_name: 'Test Area 3', - pathTokens: [ + embeddedRelations: { pathTokens: [ 'United States of America', 'Test Area', 'Test Area 2', 'Test Area 3', - ], + ] }, }, - ] as Partial[], + ] satisfies Array & { embeddedRelations: Partial }>>, }); }); diff --git a/src/model/__tests__/MediaDataSource.ts b/src/model/__tests__/MediaDataSource.ts index 0a30a164..17191d71 100644 --- a/src/model/__tests__/MediaDataSource.ts +++ b/src/model/__tests__/MediaDataSource.ts @@ -132,7 +132,7 @@ describe('MediaDataSource', () => { targetId: climbTag.entityUuid, type: climbTag.entityType, areaName: areaForTagging1.area_name, - ancestors: areaForTagging1.ancestors, + ancestors: areaForTagging1.embeddedRelations.ancestors, climbName: newSportClimb1.name, lnglat: areaForTagging1.metadata.lnglat }) diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 1250036c..76e8f97d 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -5,8 +5,6 @@ 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", () => { @@ -78,7 +76,7 @@ describe("Test area mutations", () => { 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) + expect(parentCheck?.embeddedRelations.children ?? []).toContainEqual(child._id) })) test("Add a leaf area", () => addArea('Somewhere').then(parent => addArea('Child', { leaf: true, parent })) diff --git a/src/model/__tests__/updateAreas.ts b/src/model/__tests__/updateAreas.ts index 4d5a894b..56a26b88 100644 --- a/src/model/__tests__/updateAreas.ts +++ b/src/model/__tests__/updateAreas.ts @@ -58,21 +58,21 @@ describe('Areas', () => { let canadaInDb = await areas.findOneAreaByUUID(canada.metadata.area_id) - expect(canadaInDb.children.length).toEqual(1) - expect(canadaInDb.children[0]).toEqual(bc?._id) + expect(canadaInDb.embeddedRelations.children.length).toEqual(1) + expect(canadaInDb.embeddedRelations.children[0]).toEqual(bc?._id) // Add another area to the country const theBug = await areas.addArea(testUser, 'The Bugaboos', canada.metadata.area_id) canadaInDb = await areas.findOneAreaByUUID(canada.metadata.area_id) - expect(canadaInDb.children.length).toEqual(2) - expect(canadaInDb.children[1]).toEqual(theBug?._id) + expect(canadaInDb.embeddedRelations.children.length).toEqual(2) + expect(canadaInDb.embeddedRelations.children[1]).toEqual(theBug?._id) // Verify paths and ancestors if (theBug != null) { // make TS happy - expect(theBug.ancestors) + expect(theBug.embeddedRelations.ancestors) .toEqual(`${canada.metadata.area_id.toUUID().toString()},${theBug?.metadata.area_id.toUUID().toString()}`) - expect(theBug.pathTokens) + expect(theBug.embeddedRelations.pathTokens) .toEqual([canada.area_name, theBug.area_name]) } }) @@ -94,7 +94,7 @@ describe('Areas', () => { // Reload the parent parent = await areas.findOneAreaByUUID(parent.metadata.area_id) expect(parent.climbs).toHaveLength(0) - expect(parent.children).toHaveLength(1) + expect(parent.embeddedRelations.children).toHaveLength(1) // make sure leaf and boulder flag are cleared expect(parent.metadata.leaf).toBeFalsy() expect(parent.metadata.isBoulder).toBeFalsy() @@ -105,8 +105,8 @@ describe('Areas', () => { const area = await areas.addArea(testUser, 'Table mountain', null, 'zaf') const countryInDb = await areas.findOneAreaByUUID(country.metadata.area_id) - expect(countryInDb.children.length).toEqual(1) - expect(countryInDb.children[0]).toEqual(area?._id) + expect(countryInDb.embeddedRelations.children.length).toEqual(1) + expect(countryInDb.embeddedRelations.children[0]).toEqual(area?._id) }) it('should set crag/boulder attribute when adding new areas', async () => { @@ -178,10 +178,10 @@ describe('Areas', () => { let usaInDB = await areas.findOneAreaByUUID(usa.metadata.area_id) // verify number of child areas in parent - expect(usaInDB.children as any[]).toHaveLength(3) + expect(usaInDB.embeddedRelations.children as any[]).toHaveLength(3) // verify child area IDs in parent - expect(usaInDB.children).toEqual([ + expect(usaInDB.embeddedRelations.children).toEqual([ ca._id, or._id, wa._id @@ -192,8 +192,8 @@ describe('Areas', () => { usaInDB = await areas.findOneAreaByUUID(usa.metadata.area_id) // verify child area IDs (one less than before) - expect(usaInDB.children as any[]).toHaveLength(2) - expect(usaInDB.children).toEqual([ + expect(usaInDB.embeddedRelations.children as any[]).toHaveLength(2) + expect(usaInDB.embeddedRelations.children).toEqual([ or._id, wa._id ]) From e0c154564ea0b06bd50daca463660adbdb154f88 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sat, 9 Nov 2024 14:31:04 +0200 Subject: [PATCH 16/27] Allow compass to open on the correct connection Previously you would need to re-enter the auth credentials. --- shell.nix | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/shell.nix b/shell.nix index 125512b4..616cb18f 100644 --- a/shell.nix +++ b/shell.nix @@ -12,6 +12,7 @@ pkgs.mkShell { mongodb-ce mongodb-compass mongosh + gsettings-desktop-schemas ]; # MONGOMS_DOWNLOAD_URL = "https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu2404-8.0.1.tgz"; @@ -24,13 +25,17 @@ pkgs.mkShell { MONGOMS_VERSION = "7.0.14"; shellHook = '' + set -a source .env + mongo_cnx="$MONGO_SCHEME://$MONGO_INITDB_ROOT_USERNAME:$MONGO_INITDB_ROOT_PASSWORD@$MONGO_SERVICE/$MONGO_DBNAME?authSource=$MONGO_AUTHDB&tls=$MONGO_TLS" # mongotop alias - alias mto="mongotop --uri=\"$MONGO_SCHEME://$MONGO_INITDB_ROOT_USERNAME:$MONGO_INITDB_ROOT_PASSWORD@$MONGO_SERVICE/$MONGO_DBNAME?authSource=$MONGO_AUTHDB&tls=$MONGO_TLS\"" + alias mto="mongotop --uri=$mongo_cnx" # mongostat alias - alias mst="mongostat --uri=\"$MONGO_SCHEME://$MONGO_INITDB_ROOT_USERNAME:$MONGO_INITDB_ROOT_PASSWORD@$MONGO_SERVICE/$MONGO_DBNAME?authSource=$MONGO_AUTHDB&tls=$MONGO_TLS\"" + alias mst="mongostat --uri=$mongo_cnx" + # Compass tooling + alias compass="mongodb-compass --theme=dark $mongo_cnx" echo "🧗 Alle!" ''; From 26d22c4c72dfb2ff13a4ed8f77461311228b82d6 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Sun, 10 Nov 2024 12:32:24 +0200 Subject: [PATCH 17/27] Begin integreating the parent-centric changes This commit should be squashed later... --- db-migrations/0007-area-parent-index.js | 10 ++ src/model/MutableAreaDataSource.ts | 135 +++++++++++------- .../__tests__/MutableAreaDataSource.test.ts | 52 ++++++- 3 files changed, 143 insertions(+), 54 deletions(-) create mode 100644 db-migrations/0007-area-parent-index.js diff --git a/db-migrations/0007-area-parent-index.js b/db-migrations/0007-area-parent-index.js new file mode 100644 index 00000000..e5363725 --- /dev/null +++ b/db-migrations/0007-area-parent-index.js @@ -0,0 +1,10 @@ + +printjson(db.areas.createIndex({ parent: 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.ancestorIndex': 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.children': 1 })) + +// https://www.mongodb.com/docs/v6.2/reference/method/db.collection.createIndex/#create-an-index-on-a-multiple-fields +// > The order of fields in a compound index is important for supporting sort() operations using the index. + +// It is not clear to me if there is a $lookup speed implication based on the direction of the join. +printjson(db.areas.createIndex({ parent: 1, _id: 1 })) diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index a37b213d..29c8d893 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -13,6 +13,7 @@ import CountriesLngLat from '../data/countries-with-lnglat.json' assert {type: ' import { AreaDocumnent, AreaEditableFieldsType, + AreaEmbeddedRelations, AreaType, OperationType, UpdateSortingOrderType @@ -29,6 +30,7 @@ import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' import { withTransaction } from '../utils/helpers.js' +import { arMA } from 'date-fns/locale' isoCountries.registerLocale(enJson) @@ -63,12 +65,13 @@ export default class MutableAreaDataSource extends AreaDataSource { let neighbours: string[] if (parent !== null) { - neighbours = (await this.areaModel.find({ _id: parent.embeddedRelations.children })).map(i => i.area_name) + neighbours = (await this.areaModel.find({ parent: parent._id })).map(i => i.area_name) } else { - neighbours = (await this.areaModel.find({ pathTokens: { $size: 1 } })).map(i => i.area_name) + // locate nodes with no direct parent (roots) + neighbours = (await this.areaModel.find({ parent: { $exists: false } })).map(i => i.area_name) } - neighbours = neighbours.map(i => this.areaNameCompare(i)) + neighbours = neighbours.map(neighbour => this.areaNameCompare(neighbour)) if (neighbours.includes(this.areaNameCompare(areaName))) { throw new UserInputError(`[${areaName}]: This name already exists for some other area in this parent`) } @@ -238,10 +241,7 @@ export default class MutableAreaDataSource extends AreaDataSource { draft.prevHistoryId = parent._change?.historyId }) - const parentAncestors = parent.embeddedRelations.ancestors - const parentPathTokens = parent.embeddedRelations.pathTokens - const parentGradeContext = parent.gradeContext - const newArea = newAreaHelper(areaName, parentAncestors, parentPathTokens, parentGradeContext) + const newArea = newAreaHelper(areaName, parent) if (isLeaf != null) { newArea.metadata.leaf = isLeaf } @@ -263,7 +263,6 @@ export default class MutableAreaDataSource extends AreaDataSource { // Make sure parent knows about this new area parent.embeddedRelations.children.push(newArea._id) - parent.updatedBy = experimentaAuthorId ?? user await parent.save({ timestamps: false }) return rs1[0].toObject() } @@ -311,13 +310,13 @@ export default class MutableAreaDataSource extends AreaDataSource { seq: 0 } // Remove this area id from the parent.children[] - await this.areaModel.updateOne( + await this.areaModel.updateMany( { - children: area._id + 'embeddedRelations.children': area._id }, [{ $set: { - children: { + 'embeddedRelations.children': { $filter: { input: '$children', as: 'child', @@ -434,7 +433,7 @@ export default class MutableAreaDataSource extends AreaDataSource { area.set({ area_name: sanitizedName }) // change our pathTokens - await this.updatePathTokens(session, _change, area, sanitizedName) + await this.computeEmbeddedAncestors(area) } if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) @@ -486,36 +485,73 @@ export default class MutableAreaDataSource extends AreaDataSource { } } + async passEmbeddedToChildren (_id: mongoose.Types.ObjectId, embeddedRelations?: AreaEmbeddedRelations, tree?: AreaType[]): Promise { + if (tree === undefined) { + tree = [] + } + + const area: Pick | null = await this.areaModel.findOne({ _id }, { embeddedRelations: 1, area_name: 1, 'metadata.area_id': 1 }) + if (area === null) { + logger.error('Programming error: A bad ID was passed from parent - a child is missing.') + return [] + } + + await Promise.all(area.embeddedRelations.children.map(async child => { + const ctx = { ...(embeddedRelations ?? area.embeddedRelations) } + ctx.ancestorIndex.push() + return await this.passEmbeddedToChildren(child, ctx) + })) + + return tree + } + + async computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise> { + return await this.areaModel.aggregate([ + { $match: { _id } }, + { + $graphLookup: { + from: this.areaModel.collection.name, + startWith: '$parent', + // connect parent -> _id to trace up the tree + connectFromField: 'parent', + connectToField: '_id', + as: 'ancestor', + depthField: 'level' + } + }, + { + $unwind: '$ancestor' + }, + { + $project: { + _id: 0, + ancestor: 1 + } + }, + { $sort: { 'ancestor.level': -1 } } + ]) + } + /** - * 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 + * When an area changes its parent it needs to have its children recieve the new derived data. + * There is a challenge here in that sharding the gql server may produce side-effects. */ - async updatePathTokens (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, newAreaName: string, changeIndex: number = -1): Promise { - if (area.embeddedRelations.pathTokens.length > 1) { - if (changeIndex === -1) { - changeIndex = area.embeddedRelations.pathTokens.length - 1 - } + async computeEmbeddedAncestors (area: AreaType): Promise { + await Promise.all([ + // ensure the parent has a reference to this area + this.areaModel.updateOne({ _id: area.parent }, { $addToSet: { 'embeddedRelations.children': area._id } }), + // ensure there are no divorced references to this area + this.areaModel.updateMany( + { _id: { $ne: area.parent }, 'embeddedRelations.children': area._id }, + { $pull: { 'embeddedRelations.children': area._id } } + ), + // We now compute the ancestors for this node + this.computeAncestorsFor(area._id).then(ancestors => { + }) + ]) - const newPath = [...area.embeddedRelations.pathTokens] - newPath[changeIndex] = newAreaName - area.set({ pathTokens: newPath }) - area.set({ _change: changeRecord }) - await area.save({ session }) - - // hydrate children_ids array with actual area documents - await area.populate('children') - - await Promise.all(area.embeddedRelations.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, changeIndex) - })) - } + // With all previous steps resolved we can pass the embedded data here to the children. + await Promise.all(area.embeddedRelations.children.map(async (child) => await this.passEmbeddedToChildren(child))) } /** @@ -640,19 +676,14 @@ export default class MutableAreaDataSource extends AreaDataSource { } } -export const newAreaHelper = (areaName: string, parentAncestors: string, parentPathTokens: string[], parentGradeContext: GradeContexts): AreaType => { +export const newAreaHelper = (areaName: string, parent: AreaType): AreaType => { const _id = new mongoose.Types.ObjectId() const uuid = muuid.v4() - const pathTokens = produce(parentPathTokens, draft => { - draft.push(areaName) - }) - - const ancestors = parentAncestors + ',' + uuid.toUUID().toString() - return { _id, uuid, + parent: parent._id, shortCode: '', area_name: areaName, metadata: { @@ -660,18 +691,16 @@ export const newAreaHelper = (areaName: string, parentAncestors: string, parentP leaf: false, area_id: uuid, leftRightIndex: -1, - ext_id: '', - bbox: undefined, - polygon: undefined + ext_id: '' }, climbs: [], embeddedRelations: { - pathTokens, - ancestors, children: [], - ancestorIndex: [] + ancestors: parent.embeddedRelations.ancestors + `,${uuid}`, + pathTokens: [...parent.embeddedRelations.pathTokens, areaName], + ancestorIndex: [...parent.embeddedRelations.ancestorIndex, _id] }, - gradeContext: parentGradeContext, + gradeContext: parent.gradeContext, aggregate: { byGrade: [], byDiscipline: {}, diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 76e8f97d..239a8d3f 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -59,6 +59,9 @@ describe("Test area mutations", () => { test("Add a simple area with no specifications using a parent UUID", () => areas .addArea(testUser, 'Texas2', rootCountry.metadata.area_id) .then(area => { + expect(area).toMatchObject({ + parent: rootCountry._id, + }) expect(area?._change).toMatchObject({ user: testUser, operation: OperationType.addArea, @@ -69,6 +72,12 @@ describe("Test area mutations", () => { 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 => { + expect(texas).toMatchObject({ + parent: rootCountry._id, + }) + return texas + }) .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') @@ -111,7 +120,7 @@ describe("Test area mutations", () => { })) 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(parent => Promise.all(Array.from({ length: 5 }).map(() => addArea('test', { leaf: true, parent } )))) .then(([leaf]) => leaf) .then(leaf => addArea('test', { parent: leaf })) .then(leaf => expect(leaf).toMatchObject({ metadata: { leaf: false }}))) @@ -186,4 +195,45 @@ describe("Test area mutations", () => { await areas.updateArea(testUser, big.metadata.area_id, { areaName: "Still big ol bolder"}) await addArea(nameShadow, { boulder: true, parent }) })) + + describe("updating of areas should propogate embeddedRelations", () => { + async function growTree(depth: number, bredth: number = 1): Promise { + const tree: AreaType[] = [rootCountry] + + async function grow(from: AreaType, level: number = 0) { + if (level >= depth) return + + await Promise.all(Array.from({ length: bredth }) + .map(() => addArea(undefined, { parent: from }) + .then(area => { + tree.push(area) + return grow(area, level + 1) + }))) + } + + await grow(await addArea(undefined, { parent: rootCountry})) + return tree + } + + test('computing ancestors from reified node', async () => growTree(5).then(async (tree) => { + // The 'tree' does not branch so it should be a straightforward line + expect(tree.length).toBe(6) + + // This will validate that the flat tree structure flows from root at ordinal 0 to leaf + // at ordinal tree[-1] + // tree.reduce((previous, current) => { + // expect(current.parent).toEqual(previous._id) + // return current + // }) + + let computedAncestors = await areas.computeAncestorsFor(tree.at(-1)!._id) + + expect(computedAncestors.length).toBe(tree.length) + // Similar to the previous validation step + computedAncestors.reduce((previous, current, idx) => { + expect(current.ancestor.parent).toEqual(previous.ancestor._id) + return current + }) + })) + }) }) \ No newline at end of file From ff27ec29745b8b91efb206a626a5ddf5345eb412 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Tue, 12 Nov 2024 11:21:09 +0200 Subject: [PATCH 18/27] Add debug flag to tests --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0b9729e4..ba27d442 100644 --- a/package.json +++ b/package.json @@ -73,7 +73,7 @@ "scripts": { "lint": "yarn ts-standard", "fix": "yarn ts-standard --fix", - "test": "cross-env NODE_OPTIONS=\"--experimental-vm-modules\" jest --runInBand", + "test": "cross-env NODE_OPTIONS=\"--experimental-vm-modules --inspect-brk\" jest --runInBand", "build": "tsc -p tsconfig.json", "build-release": "tsc -p tsconfig.release.json", "clean": "tsc -b --clean && rm -rf build/*", From 6e6f251d06377b27234ed1588518210d64e106c0 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Tue, 12 Nov 2024 11:21:49 +0200 Subject: [PATCH 19/27] Add an attach request to the launch options --- .vscode/launch.json | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 3d769e4e..e71b6ba3 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -90,7 +90,7 @@ "history" ], "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", }, { "type": "node", @@ -110,6 +110,12 @@ ], "console": "integratedTerminal", "internalConsoleOptions": "neverOpen" + }, + { + "type": "node", + "request": "attach", + "name": "Attach to Jest (Yarn)", + "port": 9229, } ] } \ No newline at end of file From 04e74301948c6ae62002be8310502b95f082a31d Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Tue, 12 Nov 2024 11:23:41 +0200 Subject: [PATCH 20/27] Prefer optional dbg over forced --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index ba27d442..421cf5e1 100644 --- a/package.json +++ b/package.json @@ -73,7 +73,8 @@ "scripts": { "lint": "yarn ts-standard", "fix": "yarn ts-standard --fix", - "test": "cross-env NODE_OPTIONS=\"--experimental-vm-modules --inspect-brk\" jest --runInBand", + "test": "cross-env NODE_OPTIONS=\"--experimental-vm-modules\" jest --runInBand", + "test:dbg": "cross-env NODE_OPTIONS=\"--experimental-vm-modules --inspect-brk\" jest --runInBand", "build": "tsc -p tsconfig.json", "build-release": "tsc -p tsconfig.release.json", "clean": "tsc -b --clean && rm -rf build/*", From af694b2ce12878f37e87612ac9e9b14464aead62 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Tue, 12 Nov 2024 16:54:14 +0200 Subject: [PATCH 21/27] I believe we have moved passed the need for import This code, as far as I can tell, hasn't been touched for a while and is just clutter? If I am wrong about this then I need to know why we are still importing data that we should presumably now be maintaining. --- src/db/AreaSchema.ts | 15 +- src/db/import/ClimbTransformer.ts | 53 ---- src/db/import/__tests__/climb-data.json | 74 ------ src/db/import/usa/AreaTransformer.ts | 131 ---------- src/db/import/usa/AreaTree.ts | 230 ------------------ src/db/import/usa/LinkClimbsWithCrags.ts | 29 --- src/db/import/usa/SeedState.ts | 87 ------- src/db/import/usa/USADay0Seed.ts | 77 ------ src/db/import/usa/__tests__/Tree.test.ts | 55 ----- src/db/import/usa/__tests__/Utils.test.ts | 8 - src/db/import/usa/us-states.ts | 204 ---------------- src/model/AreaRelationsEmbeddings.ts | 2 +- src/model/MutableAreaDataSource.ts | 67 ++--- .../__tests__/MutableAreaDataSource.test.ts | 45 ++-- src/model/__tests__/updateAreas.ts | 7 +- 15 files changed, 72 insertions(+), 1012 deletions(-) delete mode 100644 src/db/import/ClimbTransformer.ts delete mode 100644 src/db/import/__tests__/climb-data.json delete mode 100644 src/db/import/usa/AreaTransformer.ts delete mode 100644 src/db/import/usa/AreaTree.ts delete mode 100644 src/db/import/usa/LinkClimbsWithCrags.ts delete mode 100644 src/db/import/usa/SeedState.ts delete mode 100644 src/db/import/usa/USADay0Seed.ts delete mode 100644 src/db/import/usa/__tests__/Tree.test.ts delete mode 100644 src/db/import/usa/__tests__/Utils.test.ts delete mode 100644 src/db/import/usa/us-states.ts diff --git a/src/db/AreaSchema.ts b/src/db/AreaSchema.ts index 4b2b236b..ea2ecbfa 100644 --- a/src/db/AreaSchema.ts +++ b/src/db/AreaSchema.ts @@ -104,10 +104,7 @@ const AggregateSchema = new Schema({ byGradeBand: CountByGradeBandSchema }, { _id: false }) -/** - * - */ -const AreaEmbeddedRelationsSchema = new Schema({ +export const AreaEmbeddedRelationsSchema = new Schema({ /** * All child area documents that are contained within this area. * This has a strong relation to the areas collection, and contains only direct @@ -115,7 +112,13 @@ const AreaEmbeddedRelationsSchema = new Schema({ * * computed from the remote documents parent field */ - children: [{ type: Schema.Types.ObjectId, ref: 'areas', required: false, default: [], index: true }], + children: [{ + type: Schema.Types.ObjectId, + ref: 'areas', + required: false, + default: [], + index: true + }], /** * ancestors ids of this areas parents, traversing up the heirarchy to the root area. * This is encoded as a string, but is really an array delimited by comma. @@ -184,5 +187,7 @@ AreaSchema.index({ children: 1 }) +connection.model('embeddedRelations', AreaEmbeddedRelationsSchema) + export const getAreaModel = (name: string = 'areas'): mongoose.Model => connection.model(name, AreaSchema) diff --git a/src/db/import/ClimbTransformer.ts b/src/db/import/ClimbTransformer.ts deleted file mode 100644 index b90092e1..00000000 --- a/src/db/import/ClimbTransformer.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { geometry, Point } from '@turf/helpers' -import muuid from 'uuid-mongodb' -import { v5 as uuidv5, NIL } from 'uuid' - -import { ClimbType } from '../ClimbTypes.js' -import { defaultDisciplines, sanitizeDisciplines } from '../../GradeUtils.js' - -const transformClimbRecord = (row: any): ClimbType => { - /* eslint-disable-next-line */ - const { route_name, grade, gradeContext, safety, type, fa, metadata, description, protection, location } = row - /* eslint-disable-next-line */ - const { parent_lnglat, left_right_seq, mp_route_id, mp_sector_id } = metadata - - // in case mp_route_id is empty - const pkeyStr = mp_route_id === '' ? `${mp_sector_id as string}.${left_right_seq as string}` : mp_route_id - const uuid = muuid.from(uuidv5(pkeyStr, NIL)) - const disciplines = sanitizeDisciplines(type) ?? defaultDisciplines() - - const boulderingDiscipline = disciplines.bouldering === true ? { vscale: grade.YDS } : {} - - return { - _id: uuid, - name: route_name, - yds: grade.YDS, - grades: { - ...boulderingDiscipline, - yds: grade.YDS, - ewbank: grade.Ewbank, - font: grade.Font, - french: grade.French, - uiaa: grade.UIAA, - brazilian_crux: grade.BrazilianCrux - }, - gradeContext, - safety, - type: disciplines, - fa, - metadata: { - lnglat: geometry('Point', parent_lnglat) as Point, - left_right_index: left_right_seq, - mp_id: mp_route_id, - mp_crag_id: mp_sector_id, - areaRef: muuid.from(uuidv5(mp_sector_id, NIL)) - }, - content: { - description: Array.isArray(description) ? description.join('\n\n') : '', - location: Array.isArray(location) ? location.join('\n\n') : '', - protection: Array.isArray(protection) ? protection.join('\n\n') : '' - } - } -} - -export default transformClimbRecord diff --git a/src/db/import/__tests__/climb-data.json b/src/db/import/__tests__/climb-data.json deleted file mode 100644 index c5281073..00000000 --- a/src/db/import/__tests__/climb-data.json +++ /dev/null @@ -1,74 +0,0 @@ -[ - { - "route_name": "Orange Crush", - "grade": { - "YDS": "5.11b/c", - "French": "6c+", - "Ewbanks": "23", - "UIAA": "VIII-", - "ZA": "24", - "British": "E4 6a" - }, - "safety": "", - "type": { - "sport": true - }, - "fa": "Wade Griffith, Sterling Killion, Scott Williams", - "description": [ - "Pretty cool orange arete that sports some interesting climbing. Crimpy edges start you off climbing either side of the arete till you can climb out left to a small shelf that affords a great rest. Climb back out right to the arete and up some powerful moves through the crux and up to some easier 5.10 terrain." - ], - "location": [ - "The route is located on the far southern shoulder of Yeti dome which is actually located on the Southeastern side of Musick Mountain. Follow the dirt road from the Big Creek turnoff to the lookout tower/repeater station then follow the rounded ridge down to the cliffs. The base of the route requires scrambling down a slab a little bit to approach. It is easy to scramble up and around to the top of the route to set a TR off bolts as well." - ], - "protection": [ - "7 QD's" - ], - "metadata": { - "left_right_seq": "0", - "parent_lnglat": [ - -119.3094, - 37.1667 - ], - "parent_sector": "Yeti Dome (aka Musick Mountain)", - "mp_route_id": "105817201", - "mp_sector_id": "105817198", - "mp_path": "Western Sierra|Yeti Dome (aka Musick Mountain)" - } - }, - { - "route_name": "Random Impulse", - "grade": { - "YDS": "5.7", - "French": "5a", - "Ewbanks": "15", - "UIAA": "V+", - "ZA": "13", - "British": "MVS 4b" - }, - "safety": "", - "type": { - "trad": true - }, - "fa": "\"Unknown\" or", - "description": [ - "Some fun moves broken up by a few scree filled ledges and a big bush. Crux comes half way up where there is a nice corner finger crack for 10-15 feet. Either climb the finger crack or do some stem moves with the corner on the right and a boulder feature on the left. Continue up wandering terrain and build an anchor, or walk off." - ], - "location": [ - "25 feet to the right of Deep Springs Education." - ], - "protection": [ - "A small assortment of cams and maybe a nut or two" - ], - "metadata": { - "left_right_seq": "1", - "parent_lnglat": [ - -118.13831, - 37.3129 - ], - "parent_sector": "Westgard Pass East Side (Hwy 168)", - "mp_route_id": "119101118", - "mp_sector_id": "119100232", - "mp_path": "Sierra Eastside|Westgard Pass East Side (Hwy 168)" - } - } -] \ No newline at end of file diff --git a/src/db/import/usa/AreaTransformer.ts b/src/db/import/usa/AreaTransformer.ts deleted file mode 100644 index 236bcb76..00000000 --- a/src/db/import/usa/AreaTransformer.ts +++ /dev/null @@ -1,131 +0,0 @@ -import mongoose from 'mongoose' -import { geometry, Point } from '@turf/helpers' -import isoCountries from 'i18n-iso-countries' -import enJson from 'i18n-iso-countries/langs/en.json' assert { type: 'json' } - -import { getAreaModel } from '../../AreaSchema.js' -import { AreaType } from '../../AreaTypes' -import { Tree, AreaNode, createRootNode } from './AreaTree.js' -import { MUUID } from 'uuid-mongodb' - -isoCountries.registerLocale(enJson) - -export const createRoot = async (countryCode: string, shortCode?: string): Promise => { - if (!isoCountries.isValid(countryCode)) { - throw new Error('ISO code must be alpha 2 or 3') - } - const areaModel = getAreaModel('areas') - const countryNode = createRootNode(isoCountries.toAlpha3(countryCode).toUpperCase()) - const doc = makeDBArea(countryNode) - if (shortCode != null) { - doc.shortCode = shortCode - } - await areaModel.insertMany(doc, { ordered: false }) - return countryNode -} - -export const createAreas = async (root: AreaNode, areas: any[], areaModel: mongoose.Model): Promise => { - // Build a tree from each record in the state data file - const tree = new Tree(root) - areas.forEach(record => { - const { path }: { path: string } = record - /* eslint-disable-next-line */ - const fullPath = `${record.us_state}|${path}` // 'path' doesn't have a parent (a US state) - tree.insertMany(fullPath, record) - }) - - // Find the USA node in the db and add USA.children[] - // $push is used here because children[] may already have other states - await areaModel.findOneAndUpdate({ _id: root._id }, { $push: { children: tree.subRoot._id } }) - - // For each node in the tree, insert it to the database - let count = 0 - const chunkSize = 50 - let chunk: AreaType[] = [] - for await (const node of tree.map.values()) { - const area = makeDBArea(node) - chunk.push(area) - if (chunk.length % chunkSize === 0) { - count = count + chunk.length - await areaModel.insertMany(chunk, { ordered: false }) - chunk = [] - } - } - - if (chunk.length > 0) { - count = count + chunk.length - await areaModel.insertMany(chunk, { ordered: false }) - } - - return count -} - -/** - * Convert simple Area tree node to Mongo Area. - * @param node - */ -export const makeDBArea = (node: AreaNode): AreaType => { - const { key, isLeaf, children, _id, uuid } = node - - let areaName: string - if (node.countryName != null) { - areaName = node.countryName - } else { - areaName = isLeaf ? node.jsonLine.area_name : key.substring(key.lastIndexOf('|') + 1) - } - return { - _id, - uuid, - shortCode: '', - area_name: areaName, - metadata: { - isDestination: false, - leaf: isLeaf, - area_id: uuid, - lnglat: geometry('Point', isLeaf ? node.jsonLine.lnglat : [0, 0]) as Point, - bbox: [-180, -90, 180, 90], - leftRightIndex: -1, - ext_id: isLeaf ? extractMpId(node.jsonLine.url) : '' - }, - climbs: [], - embeddedRelations: { - children: Array.from(children), - pathTokens: node.getPathTokens(), - ancestors: uuidArrayToString(node.getAncestors()), - ancestorIndex: [] - }, - gradeContext: node.getGradeContext(), - aggregate: { - byGrade: [], - byDiscipline: {}, - byGradeBand: { - unknown: 0, - beginner: 0, - intermediate: 0, - advanced: 0, - expert: 0 - } - }, - density: 0, - totalClimbs: 0, - content: { - description: isLeaf ? (Array.isArray(node.jsonLine.description) ? node.jsonLine.description.join('\n\n') : '') : '' - } - } -} - -const URL_REGEX = /area\/(?\d+)\// -export const extractMpId = (url: string): string | undefined => URL_REGEX.exec(url)?.groups?.id - -/** - * Similar to String.join(',') but also convert each UUID to string before joining them - * @param a - * @returns - */ -const uuidArrayToString = (a: MUUID[]): string => { - return a.reduce((acc: string, curr: MUUID, index) => { - acc = acc + curr.toUUID().toString() - if (index < a.length - 1) acc = acc + ',' - return acc - }, '') -} diff --git a/src/db/import/usa/AreaTree.ts b/src/db/import/usa/AreaTree.ts deleted file mode 100644 index 1855e85f..00000000 --- a/src/db/import/usa/AreaTree.ts +++ /dev/null @@ -1,230 +0,0 @@ -import assert from 'node:assert' -import mongoose from 'mongoose' -import muuid, { MUUID } from 'uuid-mongodb' -import { v5 as uuidv5, NIL } from 'uuid' -import { getCountriesDefaultGradeContext, GradeContexts } from '../../../GradeUtils.js' - -/** - * A tree-like data structure for storing area hierarchy during raw json files progressing. - */ -export class Tree { - root?: AreaNode - subRoot: AreaNode - map = new Map() - - constructor (root?: AreaNode) { - this.root = root - } - - prefixRoot (key: string): string { - if (this.root === undefined) { - return key - } - return `${this.root.key}|${key}` - } - - private insert ( - key: string, - isSubRoot: boolean, - isLeaf: boolean = false, - jsonLine = undefined - ): Tree { - if (this.map.has(key)) return this - - const newNode = new AreaNode(key, isLeaf, jsonLine, this) - - // Special case at the root node - if (isSubRoot && this.root !== undefined) { - this.root.children.add(newNode._id) - this.subRoot = newNode - } else { - // find this new node's parent - const parent = this.getParent(key) - if (parent === undefined) assert(false, "Parent path exists but parent node doesn't") - parent?.linkChild(newNode) - newNode.setParent(parent) - } - - this.map.set(key, newNode) - return this - } - - insertMany (path: string, jsonLine: any = undefined): Tree { - const tokens: string[] = path.split('|') - tokens.reduce((acc, curr, index) => { - if (acc.length === 0) { - acc = curr - } else { - acc = acc + '|' + curr - } - const isLeaf = index === tokens.length - 1 - const isSubRoot = index === 0 - this.insert(acc, isSubRoot, isLeaf, jsonLine) - return acc - }, '') - return this - } - - getParent (key: string): AreaNode | undefined { - const parentPath = key.substring(0, key.lastIndexOf('|')) - const parent = this.atPath(parentPath) - return parent - } - - atPath (path: string): AreaNode | undefined { - return this.map.get(path) - } - - getAncestors (node: AreaNode): MUUID[] { - if (this.root === undefined) { - // Country root shouldn't have an ancestor so return itself - return [node.uuid] - } - const pathArray: MUUID[] = [this.root.uuid] - const { key } = node - const tokens: string[] = key.split('|') - - // Example node.key = 'Oregon|Central Oregon|Paulina Peak|Vigilantes de Obsidiana|Roca Rhodales' - // 0. Split key into array - // 1. Reconstruct key str by concatenating each array element. Oregon, Oregon|Central Oregon, Oregon|Central Oregon|Paulina Peak - // 2. In each iteration, look up node by key. Add node._id to pathArray[] - tokens.reduce((path, curr) => { - if (path.length === 0) { - path = curr - } else { - path = path + '|' + curr - } - const parent = this.map.get(path) - assert(parent !== undefined, 'Parent should exist') - pathArray.push(parent.uuid) - return path - }, '') - return pathArray - } - - getPathTokens (node: AreaNode): string[] { - const { key, countryName } = node - const tokens: string[] = key.split('|') - - if (this.root === undefined) { - assert(tokens.length === 1, 'Country root node should not have a parent') - // We're at country node - // - `countryName` is undefined when loading data from json files - // - we pass countryName when calling from addCountry() API - return countryName != null ? [countryName] : tokens - } - // use countryName if exists - tokens.unshift(this.root?.countryName ?? this.root.key) - return tokens - } - - /** - * - * @param node - * @returns the grade context for this tree - * Inherits from parent tree if current tree does not have one - * Country root is the highest default grade context - */ - getGradeContext (node: AreaNode): GradeContexts { - const countriesDefaultGradeContext = getCountriesDefaultGradeContext() - const USGradeContext = countriesDefaultGradeContext.US - const { key, jsonLine } = node - // country level, return key - if (this.root === undefined) { return countriesDefaultGradeContext[key] ?? USGradeContext } - // imported grade context for current area - if (jsonLine?.gradeContext !== undefined) { return jsonLine.gradeContext ?? USGradeContext } - // check grade context for parent area - const parent = this.getParent(key) - if (parent !== undefined) return parent.getGradeContext() - return countriesDefaultGradeContext[this.root.key] - } -} - -export class AreaNode { - key: string - countryName?: string // only used by create country - _id = new mongoose.Types.ObjectId() - uuid: MUUID - isLeaf: boolean - jsonLine: any = undefined - parentRef: mongoose.Types.ObjectId | null = null - children: Set = new Set() - treeRef: Tree - - constructor (key: string, isLeaf: boolean, jsonLine = undefined, treeRef: Tree, countryName?: string) { - this.uuid = getUUID(key, isLeaf, jsonLine) - this.key = key - this.isLeaf = isLeaf - if (isLeaf) { - // because our data files contain only leaf area data - this.jsonLine = jsonLine - } - this.treeRef = treeRef - this.countryName = countryName - } - - // create a ref to parent for upward traversal - setParent (parent: AreaNode | undefined): AreaNode { - if (parent !== undefined) { - const { _id } = parent - this.parentRef = _id - } - return this - } - - // add a child node to this node - linkChild (child: AreaNode): AreaNode { - const { _id } = child - this.children.add(_id) - return this - } - - /** - * Return an array of ancestor refs of this node (inclusive) - */ - getAncestors (): MUUID[] { - const a = this.treeRef.getAncestors(this) - return a - } - - /** - * Return an array of ancestor area name of this node (inclusive) - */ - getPathTokens (): string[] { - return this.treeRef.getPathTokens(this) - } - - /** - * Return the grade context for node - * Inherits from parent node if current node does not have one - */ - getGradeContext (): GradeContexts { - return this.treeRef.getGradeContext(this) - } -} - -export const createRootNode = (countryCode: string, countryName?: string): AreaNode => { - return new AreaNode(countryCode, false, undefined, new Tree(), countryName) -} - -/** - * Generate MUUID from path or mp id - * @param key path (US|Oregon|Smith Rock) - * @param isLeaf leaf node - * @param jsonLine raw data - * @returns MUUID - */ -export const getUUID = (key: string, isLeaf: boolean, jsonLine: any): MUUID => { - let idStr = key - if (isLeaf) { - assert(jsonLine !== undefined) - const extId = extractMpId(jsonLine.url) - if (extId !== undefined) { - idStr = extId - } - } - return muuid.from(uuidv5(idStr, NIL)) // Note: we should leave this alone to preserve existing stable IDs for USA -} - -const URL_REGEX = /area\/(?\d+)\// -export const extractMpId = (url: string): string | undefined => URL_REGEX.exec(url)?.groups?.id diff --git a/src/db/import/usa/LinkClimbsWithCrags.ts b/src/db/import/usa/LinkClimbsWithCrags.ts deleted file mode 100644 index fa3e9ba5..00000000 --- a/src/db/import/usa/LinkClimbsWithCrags.ts +++ /dev/null @@ -1,29 +0,0 @@ -import mongoose from 'mongoose' -import { AreaType } from '../../AreaTypes.js' -import { ClimbType } from '../../ClimbTypes.js' - -/** - * Add climb IDs to Area.climbs[] aka link climbs to their corresponding crags. - * We need this function because climbs and areas are stored in 2 separate json files. - * 1. Group climbs in climb model by crag_id - * 2. For each group, find the corresponding crag and update the 'climbs' field - * @param climbModel - * @param areaModel - * @returns void - */ -export const linkClimbsWithAreas = async ( - climbModel: mongoose.Model, - areaModel: mongoose.Model): Promise => { - // Group all climbs by crag - const climbsGroupByCrag: Array<{ _id: mongoose.Types.ObjectId, climbs: ClimbType[] }> = await climbModel.aggregate([ - { $group: { _id: '$metadata.areaRef', climbs: { $push: '$$ROOT._id' } } } - ]).allowDiskUse(true) - - // Populate area.climbs array with climb IDs - for await (const climbGroup of climbsGroupByCrag) { - const cragId = climbGroup._id - const { climbs } = climbGroup - await areaModel.findOneAndUpdate({ 'metadata.area_id': cragId }, { climbs, totalClimbs: climbs.length }).clone() - } - return await Promise.resolve() -} diff --git a/src/db/import/usa/SeedState.ts b/src/db/import/usa/SeedState.ts deleted file mode 100644 index 3c09f94b..00000000 --- a/src/db/import/usa/SeedState.ts +++ /dev/null @@ -1,87 +0,0 @@ -import mongoose from 'mongoose' -import readline from 'node:readline' -import fs from 'node:fs' - -import { getAreaModel } from '../../index.js' -import { AreaType } from '../../AreaTypes.js' -import { linkClimbsWithAreas } from './LinkClimbsWithCrags.js' -import { getClimbModel } from '../../ClimbSchema.js' -import { ClimbType } from '../../ClimbTypes.js' -import transformClimbRecord from '../ClimbTransformer.js' -import { createAreas } from './AreaTransformer.js' -import { AreaNode } from './AreaTree.js' -import { logger } from '../../../logger.js' - -export interface JobStats { - state: string - areaCount: number - climbCount: number -} - -export const seedState = async (root: AreaNode, stateCode: string, fileClimbs: string, fileAreas: string): Promise => { - console.time('Loaded ' + stateCode) - - const areaModel: mongoose.Model = getAreaModel('areas') - const climbModel: mongoose.Model = getClimbModel('climbs') - logger.info('start', stateCode) - const stats = await Promise.all([ - loadClimbs(fileClimbs, climbModel), - loadAreas(root, fileAreas, areaModel) - ]) - logger.info('link', stateCode) - await linkClimbsWithAreas(climbModel, areaModel) - - console.timeEnd('Loaded ' + stateCode) - - return await Promise.resolve({ state: stateCode, climbCount: stats[0], areaCount: stats[1] }) -} - -export const dropCollection = async (name: string): Promise => { - try { - await mongoose.connection.db.dropCollection(name) - } catch (e) { - } -} - -const loadClimbs = async (fileName: string, model: mongoose.Model): Promise => { - let count = 0 - const chunkSize = 100 - let chunk: ClimbType[] = [] - - const rl = readline.createInterface({ - input: fs.createReadStream(fileName), - terminal: false - }) - - for await (const line of rl) { - const jsonLine = JSON.parse(line) - const record = transformClimbRecord(jsonLine) - chunk.push(record) - if (chunk.length % chunkSize === 0) { - count = count + chunk.length - await model.insertMany(chunk, { ordered: false }) - chunk = [] - } - } - - if (chunk.length > 0) { - count = count + chunk.length - await model.insertMany(chunk, { ordered: false }) - } - return count -} - -const loadAreas = async (root: AreaNode, fileName: string, model: mongoose.Model): Promise => { - const buffer: any[] = [] - - const rl = readline.createInterface({ - input: fs.createReadStream(fileName), - terminal: false - }) - - for await (const line of rl) { - buffer.push(JSON.parse(line)) - } - - return await createAreas(root, buffer, model) -} diff --git a/src/db/import/usa/USADay0Seed.ts b/src/db/import/usa/USADay0Seed.ts deleted file mode 100644 index 6540f86b..00000000 --- a/src/db/import/usa/USADay0Seed.ts +++ /dev/null @@ -1,77 +0,0 @@ -import fs from 'node:fs' -import pLimit from 'p-limit' - -import { connectDB, gracefulExit, createIndexes } from '../../index.js' -import { createRoot } from './AreaTransformer.js' -import US_STATES from './us-states.js' -import { seedState, dropCollection, JobStats } from './SeedState.js' -import { logger } from '../../../logger.js' - -const contentDir: string = process.env.CONTENT_BASEDIR ?? '' - -const DEFAULT_CONCURRENT_JOBS = 4 -const concurrentJobs: number = - process.env.OB_SEED_JOBS !== undefined - ? parseInt(process.env.OB_SEED_JOBS) - : DEFAULT_CONCURRENT_JOBS - -logger.info('Data dir', contentDir) -logger.info('Max concurrent jobs: ', concurrentJobs) - -if (contentDir === '') { - logger.error('Missing CONTENT_BASEDIR env') - process.exit(1) -} - -const main = async (): Promise => { - const limiter = pLimit( - concurrentJobs > 0 ? concurrentJobs : DEFAULT_CONCURRENT_JOBS - ) - - // TODO: Allow update. Right now we drop the entire collection on each run. - await dropCollection('areas') - await dropCollection('climbs') - - console.time('Creating indexes') - await createIndexes() - console.timeEnd('Creating indexes') - - const rootNode = await createRoot('US', 'USA') - - const stats: Array = await Promise.all>( - US_STATES.map(async state => { - const code = state.code.toLowerCase() - const fRoutes = `${contentDir}/${code}-routes.jsonlines` - const fAreas = `${contentDir}/${code}-areas.jsonlines` - - if (fs.existsSync(fRoutes) && fs.existsSync(fAreas)) { - /* eslint-disable-next-line */ - return limiter(seedState, rootNode, code, fRoutes, fAreas) - } - return await Promise.resolve() - }) - ) - - printStats(stats) - - await gracefulExit() - return await Promise.resolve() -} - -const printStats = (stats: Array): void => { - logger.info('------------------ Summary -------------------') - const sums = { states: 0, climbs: 0, areas: 0 } - for (const entry of stats) { - if (entry !== undefined) { - logger.info(entry) - const e = entry as JobStats - sums.climbs += e.climbCount - sums.areas += e.climbCount - sums.states += 1 - } - } - logger.info('---------------------------------------------') - logger.info('Total: ', sums) -} - -void connectDB(main) diff --git a/src/db/import/usa/__tests__/Tree.test.ts b/src/db/import/usa/__tests__/Tree.test.ts deleted file mode 100644 index ebb61a16..00000000 --- a/src/db/import/usa/__tests__/Tree.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { Tree, createRootNode } from '../AreaTree' - -const path1 = 'Oregon|Central Oregon|Paulina Peak|Vigilantes de Obsidiana|Roca Rhodales' -const path2 = 'Oregon|Central Oregon|Smith Rock|Spiderman Buttress' - -const jsonLine1 = { - url: '/area/117795688/foo-bar' -} - -const jsonLine2 = { - url: '/area/1234567/foo-bar' -} - -describe('Area Tree data structure', () => { - it('should create a tree from path string', () => { - const root = createRootNode('US') - const tree = new Tree(root) - tree.insertMany(path1, jsonLine1) - expect(tree.map.size).toEqual(path1.split('|').length) - expect(tree.atPath('Oregon|Central Oregon')?.children.size).toEqual(1) - }) - - it('shoud add a branch', () => { - const tree = new Tree(createRootNode('US')) - tree.insertMany(path1, jsonLine1) - tree.insertMany(path2, jsonLine2) // Central Oregon should now have 2 children - - expect(tree.atPath('Oregon')?.children.size).toEqual(1) - const node = tree.atPath('Oregon|Central Oregon') - expect(node?.children.size).toEqual(2) - - // verify Central Oregon children - if (node?.children !== undefined) { - const ids = Array.from(node.children.values()) - const child1 = tree.atPath('Oregon|Central Oregon|Paulina Peak') - const child2 = tree.atPath('Oregon|Central Oregon|Smith Rock') - expect([child1?._id, child2?._id]).toEqual(expect.arrayContaining(ids)) - } - }) - - it('builds complete path to root', () => { - const countryRoot = createRootNode('US') - const tree = new Tree(countryRoot) - tree.insertMany(path1, jsonLine1) - const leaf = tree.atPath(path1) - if (leaf !== undefined) { - const ancestors = leaf.getAncestors() - console.log(ancestors) - expect(ancestors.length).toEqual(path1.split('|').length + 1) // all element of path1 + 1 for US root - expect(ancestors[0]).toEqual(countryRoot?.uuid) - const stateRoot = tree.atPath('Oregon') - expect(ancestors[1]).toEqual(stateRoot?.uuid) - } - }) -}) diff --git a/src/db/import/usa/__tests__/Utils.test.ts b/src/db/import/usa/__tests__/Utils.test.ts deleted file mode 100644 index 371fe006..00000000 --- a/src/db/import/usa/__tests__/Utils.test.ts +++ /dev/null @@ -1,8 +0,0 @@ -import { extractMpId } from '../AreaTransformer.js' - -test('Verify area url parser', () => { - expect(extractMpId('/area/117795688/foo-bar')).toEqual('117795688') - // test again since Regex matcher can be stateful - expect(extractMpId('/area/123/abc')).toEqual('123') - expect(extractMpId('/area//apple')).toEqual(undefined) -}) diff --git a/src/db/import/usa/us-states.ts b/src/db/import/usa/us-states.ts deleted file mode 100644 index eda66b3a..00000000 --- a/src/db/import/usa/us-states.ts +++ /dev/null @@ -1,204 +0,0 @@ -const US_STATES = [ - { - name: 'Alabama', - code: 'AL' - }, - { - name: 'Alaska', - code: 'AK' - }, - { - name: 'Arizona', - code: 'AZ' - }, - { - name: 'Arkansas', - code: 'AR' - }, - { - name: 'California', - code: 'CA' - }, - { - name: 'Colorado', - code: 'CO' - }, - { - name: 'Connecticut', - code: 'CT' - }, - { - name: 'Delaware', - code: 'DE' - }, - { - name: 'Florida', - code: 'FL' - }, - { - name: 'Georgia', - code: 'GA' - }, - { - name: 'Hawaii', - code: 'HI' - }, - { - name: 'Idaho', - code: 'ID' - }, - { - name: 'Illinois', - code: 'IL' - }, - { - name: 'Indiana', - code: 'IN' - }, - { - name: 'Iowa', - code: 'IA' - }, - { - name: 'Kansas', - code: 'KS' - }, - { - name: 'Kentucky', - code: 'KY' - }, - { - name: 'Louisiana', - code: 'LA' - }, - { - name: 'Maine', - code: 'ME' - }, - { - name: 'Maryland', - code: 'MD' - }, - { - name: 'Massachusetts', - code: 'MA' - }, - { - name: 'Michigan', - code: 'MI' - }, - { - name: 'Minnesota', - code: 'MN' - }, - { - name: 'Mississippi', - code: 'MS' - }, - { - name: 'Missouri', - code: 'MO' - }, - { - name: 'Montana', - code: 'MT' - }, - { - name: 'Nebraska', - code: 'NE' - }, - { - name: 'Nevada', - code: 'NV' - }, - { - name: 'New Hampshire', - code: 'NH' - }, - { - name: 'New Jersey', - code: 'NJ' - }, - { - name: 'New Mexico', - code: 'NM' - }, - { - name: 'New York', - code: 'NY' - }, - { - name: 'North Carolina', - code: 'NC' - }, - { - name: 'North Dakota', - code: 'ND' - }, - { - name: 'Ohio', - code: 'OH' - }, - { - name: 'Oklahoma', - code: 'OK' - }, - { - name: 'Oregon', - code: 'OR' - }, - { - name: 'Pennsylvania', - code: 'PA' - }, - { - name: 'Rhode Island', - code: 'RI' - }, - { - name: 'South Carolina', - code: 'SC' - }, - { - name: 'South Dakota', - code: 'SD' - }, - { - name: 'Tennessee', - code: 'TN' - }, - { - name: 'Texas', - code: 'TX' - }, - { - name: 'Utah', - code: 'UT' - }, - { - name: 'Vermont', - code: 'VT' - }, - { - name: 'Virginia', - code: 'VA' - }, - { - name: 'Washington', - code: 'WA' - }, - { - name: 'West Virginia', - code: 'WV' - }, - { - name: 'Wisconsin', - code: 'WI' - }, - { - name: 'Wyoming', - code: 'WY' - } -] - -export default US_STATES diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index 22be6123..857c10d6 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/src/model/AreaRelationsEmbeddings.ts @@ -35,5 +35,5 @@ export async function computeEmbeddedRelations (rootId: mongoose.Types.ObjectId) } ]) - throw new Error("not implemented yet") + throw new Error('not implemented yet') } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 29c8d893..692477ae 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -8,7 +8,6 @@ import mongoose, { ClientSession } from 'mongoose' import { NIL, v5 as uuidv5 } from 'uuid' import muuid, { MUUID } from 'uuid-mongodb' -import { GradeContexts } from '../GradeUtils.js' import CountriesLngLat from '../data/countries-with-lnglat.json' assert {type: 'json'} import { AreaDocumnent, @@ -29,8 +28,7 @@ import { createInstance as createExperimentalUserDataSource } from '../model/Exp import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' -import { withTransaction } from '../utils/helpers.js' -import { arMA } from 'date-fns/locale' +import { muuidToString, withTransaction } from '../utils/helpers.js' isoCountries.registerLocale(enJson) @@ -64,11 +62,15 @@ export default class MutableAreaDataSource extends AreaDataSource { // that the name is unique for this context let neighbours: string[] + const common = { + _deleting: { $exists: false } + } + if (parent !== null) { - neighbours = (await this.areaModel.find({ parent: parent._id })).map(i => i.area_name) + neighbours = (await this.areaModel.find({ parent: parent._id, ...common })).map(i => i.area_name) } else { // locate nodes with no direct parent (roots) - neighbours = (await this.areaModel.find({ parent: { $exists: false } })).map(i => i.area_name) + neighbours = (await this.areaModel.find({ parent: { $exists: false }, ...common })).map(i => i.area_name) } neighbours = neighbours.map(neighbour => this.areaNameCompare(neighbour)) @@ -262,7 +264,12 @@ export default class MutableAreaDataSource extends AreaDataSource { const rs1 = await this.areaModel.insertMany(newArea, { session }) // Make sure parent knows about this new area - parent.embeddedRelations.children.push(newArea._id) + if (parent.embeddedRelations.children === null) { + parent.embeddedRelations.children = [newArea._id] + } else { + parent.embeddedRelations.children.push(newArea._id) + } + await parent.save({ timestamps: false }) return rs1[0].toObject() } @@ -309,28 +316,13 @@ export default class MutableAreaDataSource extends AreaDataSource { operation: OperationType.deleteArea, seq: 0 } - // Remove this area id from the parent.children[] + + // Remove this area id from the parents denormalized children await this.areaModel.updateMany( + { _id: area.parent }, { - 'embeddedRelations.children': area._id - }, - [{ - $set: { - 'embeddedRelations.children': { - $filter: { - input: '$children', - as: 'child', - cond: { $ne: ['$$child', area._id] } - } - }, - updatedBy: user, - '_change.prevHistoryId': '$_change.historyId', - _change: produce(_change, draft => { - draft.seq = 0 - }) - } - }] - , { + $pull: { 'embeddedRelations.children': area._id } + }, { timestamps: false }).orFail().session(session) @@ -404,7 +396,7 @@ export default class MutableAreaDataSource extends AreaDataSource { // 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)) + await this.validateUniqueAreaName(areaName, await this.areaModel.findOne({ _id: area.parent }).session(session)) } const opType = OperationType.updateArea @@ -431,8 +423,6 @@ export default class MutableAreaDataSource extends AreaDataSource { if (areaName != null) { const sanitizedName = sanitizeStrict(areaName) area.set({ area_name: sanitizedName }) - - // change our pathTokens await this.computeEmbeddedAncestors(area) } @@ -631,16 +621,25 @@ export default class MutableAreaDataSource extends AreaDataSource { const parentArea = await this.areaModel.findOne({ 'metadata.area_id': parentUuid }) .batchSize(10) - .populate<{ children: AreaDocumnent[] }>({ path: 'children', model: this.areaModel }) + .populate<{ embeddedRelations: { children: AreaDocumnent[] } }>({ + path: 'embeddedRelations.children', + model: this.areaModel + }) .allowDiskUse(true) .session(session) .orFail() const acc: StatsSummary[] = [] + + if (parentArea.embeddedRelations.children === null) { + console.log(await this.areaModel.findOne({ 'metadata.area_id': parentUuid })) + throw new Error('Oopie doopers') + } + /** * Collect existing stats from all children. For affected node, use the stats from previous calculation. */ - for (const childArea of parentArea.children) { + for (const childArea of parentArea.embeddedRelations.children) { if (childArea._id.equals(area._id)) { if (childSummary != null) acc.push(childSummary) } else { @@ -696,9 +695,11 @@ export const newAreaHelper = (areaName: string, parent: AreaType): AreaType => { climbs: [], embeddedRelations: { children: [], - ancestors: parent.embeddedRelations.ancestors + `,${uuid}`, + // ancestors and pathTokens terminate at the present node, while the ancestorIndex + // terminates at the parent. + ancestors: parent.embeddedRelations.ancestors + `,${muuidToString(uuid)}`, pathTokens: [...parent.embeddedRelations.pathTokens, areaName], - ancestorIndex: [...parent.embeddedRelations.ancestorIndex, _id] + ancestorIndex: [...parent.embeddedRelations.ancestorIndex, parent._id] }, gradeContext: parent.gradeContext, aggregate: { diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 239a8d3f..c11daeed 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -6,7 +6,6 @@ import { AreaType, OperationType } from "../../db/AreaTypes" import { ChangeRecordMetadataType } from "../../db/ChangeLogType" import { UserInputError } from "apollo-server-core" - describe("Test area mutations", () => { let areas: MutableAreaDataSource let rootCountry: AreaType @@ -50,12 +49,12 @@ describe("Test area mutations", () => { areas = MutableAreaDataSource.getInstance() // We need a root country, and it is beyond the scope of these tests - rootCountry = await areas.addCountry("USA") + rootCountry = await areas.addCountry("USA") }) - afterAll(inMemoryDB.close) + afterAll(inMemoryDB.close) - describe("Add area param cases", () => { + 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 => { @@ -197,43 +196,45 @@ describe("Test area mutations", () => { })) describe("updating of areas should propogate embeddedRelations", () => { - async function growTree(depth: number, bredth: number = 1): Promise { - const tree: AreaType[] = [rootCountry] + const defaultDepth = 5 + async function growTree(depth: number = defaultDepth, bredth: number = 1): Promise { + const tree: AreaType[] = [rootCountry, await addArea()] async function grow(from: AreaType, level: number = 0) { if (level >= depth) return await Promise.all(Array.from({ length: bredth }) - .map(() => addArea(undefined, { parent: from }) + .map((_ ,idx) => addArea(`${level}-${idx}`, { parent: from }) .then(area => { + if (!area.parent?.equals(from._id)) { + throw new Error(`${area.parent} should have been ${from._id}`) + } tree.push(area) return grow(area, level + 1) }))) } - await grow(await addArea(undefined, { parent: rootCountry})) + await grow(tree.at(-1)!) + return tree } - test('computing ancestors from reified node', async () => growTree(5).then(async (tree) => { - // The 'tree' does not branch so it should be a straightforward line - expect(tree.length).toBe(6) - - // This will validate that the flat tree structure flows from root at ordinal 0 to leaf - // at ordinal tree[-1] - // tree.reduce((previous, current) => { - // expect(current.parent).toEqual(previous._id) - // return current - // }) - + test('computing ancestors from reified node', async () => growTree().then(async (tree) => { let computedAncestors = await areas.computeAncestorsFor(tree.at(-1)!._id) - expect(computedAncestors.length).toBe(tree.length) - // Similar to the previous validation step + // Check that each node refers specifically to the previous one as its parent + // - this will check that the areas are in order and that no nodes are skipped. computedAncestors.reduce((previous, current, idx) => { - expect(current.ancestor.parent).toEqual(previous.ancestor._id) + expect(current.ancestor.parent?.equals(previous.ancestor._id)) + expect(current.ancestor._id.equals(tree[idx]._id)) return current }) })) + + test('ancestors should be computed on area add.', async () => growTree(5).then(async (tree) => { + let leaf = tree.at(-1)! + expect(leaf.embeddedRelations.pathTokens.join(',')).toEqual(tree.map(i => i.area_name).join(',')) + expect(leaf.embeddedRelations.ancestors).toEqual(tree.map(i => i.metadata.area_id).join(',')) + })) }) }) \ No newline at end of file diff --git a/src/model/__tests__/updateAreas.ts b/src/model/__tests__/updateAreas.ts index 56a26b88..e366c054 100644 --- a/src/model/__tests__/updateAreas.ts +++ b/src/model/__tests__/updateAreas.ts @@ -178,7 +178,8 @@ describe('Areas', () => { let usaInDB = await areas.findOneAreaByUUID(usa.metadata.area_id) // verify number of child areas in parent - expect(usaInDB.embeddedRelations.children as any[]).toHaveLength(3) + expect(Array.isArray(usaInDB.embeddedRelations.children)) + expect(usaInDB.embeddedRelations.children).toHaveLength(3) // verify child area IDs in parent expect(usaInDB.embeddedRelations.children).toEqual([ @@ -190,9 +191,9 @@ describe('Areas', () => { await areas.deleteArea(testUser, ca.metadata.area_id) usaInDB = await areas.findOneAreaByUUID(usa.metadata.area_id) - + console.log(usaInDB.embeddedRelations) // verify child area IDs (one less than before) - expect(usaInDB.embeddedRelations.children as any[]).toHaveLength(2) + expect(usaInDB.embeddedRelations.children).toHaveLength(2) expect(usaInDB.embeddedRelations.children).toEqual([ or._id, wa._id From 2e9c3782d099f25ffe4fb4429fa7e90617c14b94 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Tue, 12 Nov 2024 21:06:49 +0200 Subject: [PATCH 22/27] Denormalize related data together I'm not sure why I didn't do this initially --- db-migrations/0006-area-structure.js | 46 ++-- db-migrations/0007-area-parent-index.js | 10 - shell.nix | 2 + src/db/AreaSchema.ts | 35 +-- src/db/AreaTypes.ts | 34 +-- src/db/export/Typesense/Client.ts | 7 +- src/db/export/Typesense/transformers.ts | 2 +- src/db/export/json/area.resolver.test.ts | 9 +- src/db/export/json/area.resolver.ts | 4 +- src/db/utils/jobs/MapTiles/exportCmd.ts | 6 +- src/graphql/resolvers.ts | 2 +- src/model/AreaRelationsEmbeddings.ts | 111 ++++++--- src/model/MutableAreaDataSource.ts | 231 +++++++----------- src/model/MutableMediaDataSource.ts | 5 +- .../__tests__/AreaRelationsEmbeddings.test.ts | 153 ++++++++++++ src/model/__tests__/BulkDataSource.test.ts | 18 +- src/model/__tests__/MediaDataSource.ts | 2 +- .../__tests__/MutableAreaDataSource.test.ts | 42 ---- src/model/__tests__/updateAreas.ts | 6 +- src/utils/testUtils.ts | 2 +- 20 files changed, 427 insertions(+), 300 deletions(-) delete mode 100644 db-migrations/0007-area-parent-index.js create mode 100644 src/model/__tests__/AreaRelationsEmbeddings.test.ts diff --git a/db-migrations/0006-area-structure.js b/db-migrations/0006-area-structure.js index 5f09958d..9921759e 100644 --- a/db-migrations/0006-area-structure.js +++ b/db-migrations/0006-area-structure.js @@ -46,39 +46,30 @@ db.areas.find().forEach((doc) => { { $group: { _id: "$_id", - pathTokens: { $push: "$ancestorPath.area_name" }, - ancestors: { $push: "$ancestorPath.metadata.area_id" }, - ancestorIndex: { $push: "$ancestorPath._id" }, + ancestors: { $push: '$ancestorPath' } }, }, ]).toArray(); - const pathTokens = [...(pathDocs[0]?.pathTokens ?? []), doc.area_name]; - const ancestors = [ - ...(pathDocs[0]?.ancestors ?? []), - doc.metadata.area_id, - ].join(","); - const ancestorIndex = pathDocs[0]?.ancestorIndex ?? []; - const embeddedRelations = { children: childrenMap[doc._id] || [], - pathTokens, - ancestors, - ancestorIndex, + // map out the ancestors of this doc (terminating at the current node for backward-compat reasons) + // We take out the relevant data we would like to be denormalized. + ancestors: [...(pathDocs[0]?.ancestors ?? []), doc].map(i => ({ + _id: i._id, + name: i.area_name, + uuid: i.metadata.area_id + })) }; - if (pathTokens.join(",") !== doc.pathTokens.join(",")) { - throw `Path tokens did not match (${pathTokens} != ${doc.pathTokens})`; + if (embeddedRelations.ancestors.map(i => i.name).join(",") !== doc.pathTokens.join(",")) { + throw `Path tokens did not match (${embeddedRelations.ancestors.map(i => i.name)} != ${doc.pathTokens})`; } - if (ancestors !== doc.ancestors) { - throw `Path tokens did not match (${ancestors} != ${doc.ancestors})`; + if (embeddedRelations.ancestors.map(i => i.uuid).join(',') !== doc.ancestors) { + throw `Ancestors did not match (${embeddedRelations.ancestors.map(i => i.uuid)} != ${doc.ancestors})`; } - if (ancestorIndex.length !== pathTokens.length - 1) { - print({ ancestorIndex, pathTokens }); - throw "ancestorIndex is the wrong shape"; - } // Use bulkWrite for efficient updates db.areas.updateOne( @@ -92,4 +83,15 @@ print("Removing old fields."); // Remove the unneeded values since all ops have run without raising an assertion issue db.areas.updateMany({}, { $unset: { children: "", pathTokens: "", ancestors: "" }, -}); \ No newline at end of file +}); + +printjson(db.areas.createIndex({ parent: 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.children': 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.ancestors._id': 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.ancestors.uuid': 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.ancestors.name': 1 })) + +// https://www.mongodb.com/docs/v6.2/reference/method/db.collection.createIndex/#create-an-index-on-a-multiple-fields +// > The order of fields in a compound index is important for supporting sort() operations using the index. +// It is not clear to me if there is a $lookup speed implication based on the direction of the join. +printjson(db.areas.createIndex({ parent: 1, _id: 1 })) diff --git a/db-migrations/0007-area-parent-index.js b/db-migrations/0007-area-parent-index.js deleted file mode 100644 index e5363725..00000000 --- a/db-migrations/0007-area-parent-index.js +++ /dev/null @@ -1,10 +0,0 @@ - -printjson(db.areas.createIndex({ parent: 1 })) -printjson(db.areas.createIndex({ 'embeddedRelations.ancestorIndex': 1 })) -printjson(db.areas.createIndex({ 'embeddedRelations.children': 1 })) - -// https://www.mongodb.com/docs/v6.2/reference/method/db.collection.createIndex/#create-an-index-on-a-multiple-fields -// > The order of fields in a compound index is important for supporting sort() operations using the index. - -// It is not clear to me if there is a $lookup speed implication based on the direction of the join. -printjson(db.areas.createIndex({ parent: 1, _id: 1 })) diff --git a/shell.nix b/shell.nix index 616cb18f..149f87ae 100644 --- a/shell.nix +++ b/shell.nix @@ -13,6 +13,8 @@ pkgs.mkShell { mongodb-compass mongosh gsettings-desktop-schemas + + deno ]; # MONGOMS_DOWNLOAD_URL = "https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu2404-8.0.1.tgz"; diff --git a/src/db/AreaSchema.ts b/src/db/AreaSchema.ts index ea2ecbfa..582102bb 100644 --- a/src/db/AreaSchema.ts +++ b/src/db/AreaSchema.ts @@ -104,6 +104,22 @@ const AggregateSchema = new Schema({ byGradeBand: CountByGradeBandSchema }, { _id: false }) +const AreaEmbeddedRelationsAncestor = new Schema({ + _id: { + type: mongoose.Types.ObjectId, + required: true, + index: true, + ref: 'areas' + }, + name: { type: String, required: true, index: true }, + uuid: { + type: 'object', + value: { type: 'Buffer' }, + required: true, + index: true + } +}, { _id: false }) + export const AreaEmbeddedRelationsSchema = new Schema({ /** * All child area documents that are contained within this area. @@ -115,27 +131,14 @@ export const AreaEmbeddedRelationsSchema = new Schema({ children: [{ type: Schema.Types.ObjectId, ref: 'areas', - required: false, - default: [], index: true }], + /** * ancestors ids of this areas parents, traversing up the heirarchy to the root area. - * This is encoded as a string, but is really an array delimited by comma. - * @see https://www.mongodb.com/docs/manual/tutorial/model-tree-structures-with-materialized-paths/ - * - * computed from the remote documents parent field - */ - ancestors: { type: String, required: true, index: true }, - /** - * pathTokens names of this areas parents, traversing up the heirarchy to the root area - * with the current area being the last element. - */ - pathTokens: [{ type: String, required: true, index: true }], - /** - * Rather than doing a graph lookup, the ancestry can be traced from here. + * computed from the remote documents field */ - ancestorIndex: [{ type: Schema.Types.ObjectId, ref: 'areas', required: false, default: [], index: true }] + ancestors: [{ type: AreaEmbeddedRelationsAncestor, required: true }] }, { _id: false }) export const AreaSchema = new Schema({ diff --git a/src/db/AreaTypes.ts b/src/db/AreaTypes.ts index 3903624c..de1cc4c4 100644 --- a/src/db/AreaTypes.ts +++ b/src/db/AreaTypes.ts @@ -108,15 +108,7 @@ export interface IAreaProps extends AuthorMetadata { _deleting?: Date } -export interface AreaEmbeddedRelations { - /** - * All child area documents that are contained within this area. - * This has a strong relation to the areas collection, and contains only direct - * child areas - rather than all descendents. - * - * computed from the remote documents parent field - */ - children: mongoose.Types.ObjectId[] +export interface DenormalizedAreaSummary { /** * ancestors ids of this areas parents, traversing up the heirarchy to the root area. * This is encoded as a string, but is really an array delimited by comma. @@ -124,16 +116,28 @@ export interface AreaEmbeddedRelations { * * computed from the remote documents parent field */ - ancestors: string + uuid: MUUID /** - * Trace ancestors back to root, can be used as an index rather than computing + * Trace ancestors back to root, can be used as an index rather than computing it + */ + _id: mongoose.Types.ObjectId + /** + * pathTokens names of this areas parents, traversing up the heirarchy to the root area + * with the current area being the last element. */ - ancestorIndex: mongoose.Types.ObjectId[] + name: string +} + +export interface AreaEmbeddedRelations { /** - * pathTokens names of this areas parents, traversing up the heirarchy to the root area - * with the current area being the last element. + * All child area documents that are contained within this area. + * This has a strong relation to the areas collection, and contains only direct + * child areas - rather than all descendents. + * + * computed from the remote documents parent field */ - pathTokens: string[] + children: mongoose.Types.ObjectId[] + ancestors: DenormalizedAreaSummary[] } export interface IAreaMetadata { diff --git a/src/db/export/Typesense/Client.ts b/src/db/export/Typesense/Client.ts index 2e0a9736..4f7f23aa 100644 --- a/src/db/export/Typesense/Client.ts +++ b/src/db/export/Typesense/Client.ts @@ -8,6 +8,7 @@ import { DBOperation } from '../../ChangeLogType.js' import Config from '../../../Config.js' import { ClimbExtType, ClimbType } from '../../ClimbTypes.js' import MutableAreaDataSource from '../../../model/MutableAreaDataSource.js' +import { muuidToString } from '../../../utils/helpers.js' /** * Return a Typesense client. @@ -69,12 +70,12 @@ export const updateClimbIndex = async (climb: ClimbType | null, op: DBOperation) } // Look up additional attrs required by Climb index in Typesense. - const { pathTokens, ancestors } = (await MutableAreaDataSource.getInstance().findOneAreaByUUID(climb.metadata.areaRef)).embeddedRelations + const { ancestors } = (await MutableAreaDataSource.getInstance().findOneAreaByUUID(climb.metadata.areaRef)).embeddedRelations const climbExt: ClimbExtType = { ...climb, - pathTokens, - ancestors + pathTokens: ancestors.map(i => i.name), + ancestors: ancestors.map(i => muuidToString(i.uuid)).join(',') } switch (op) { diff --git a/src/db/export/Typesense/transformers.ts b/src/db/export/Typesense/transformers.ts index 79b4dbd3..a6ff235c 100644 --- a/src/db/export/Typesense/transformers.ts +++ b/src/db/export/Typesense/transformers.ts @@ -13,7 +13,7 @@ export function mongoAreaToTypeSense (doc: AreaType): AreaTypeSenseItem { id: doc.metadata.area_id.toUUID().toString(), areaUUID: doc.metadata.area_id.toUUID().toString(), name: doc.area_name ?? '', - pathTokens: doc.embeddedRelations.pathTokens, + pathTokens: doc.embeddedRelations.ancestors.map(i => i.name), areaLatLng: geoToLatLng(doc.metadata.lnglat), leaf: doc.metadata.leaf, isDestination: doc.metadata.isDestination, diff --git a/src/db/export/json/area.resolver.test.ts b/src/db/export/json/area.resolver.test.ts index 0ce3bbb0..9c653471 100644 --- a/src/db/export/json/area.resolver.test.ts +++ b/src/db/export/json/area.resolver.test.ts @@ -1,5 +1,7 @@ +import mongoose from 'mongoose' import { resolveAreaFileName, resolveAreaSubPath } from './area.resolver' import path from 'path' +import muid from 'uuid-mongodb' describe('area resolvers', () => { describe('area name resolver', () => { @@ -43,13 +45,16 @@ describe('area resolvers', () => { ] function assertSubPathResolver (path: string[], expected: string) { - expect(resolveAreaSubPath({ embeddedRelations: { pathTokens: path }})).toBe(expected) + const uuid = muid.v4() + const _id = new mongoose.Types.ObjectId() + expect(resolveAreaSubPath({ embeddedRelations: { children: [], ancestors: path.map(name => ({ name, _id, uuid }))} })) + .toBe(expected) } testCases.forEach(testCase => { it(testCase.name, () => { assertSubPathResolver(testCase.input, testCase.expected) - }) + }) }) }) }) diff --git a/src/db/export/json/area.resolver.ts b/src/db/export/json/area.resolver.ts index dd82d781..bf9c52c3 100644 --- a/src/db/export/json/area.resolver.ts +++ b/src/db/export/json/area.resolver.ts @@ -6,8 +6,8 @@ export function resolveAreaFileName (area: Partial): string { if (name === undefined || name === '') { return 'unknown' } else { return name } } -export function resolveAreaSubPath (area: Partial & { embeddedRelations: Partial }>): string { - const paths: string[] = area?.embeddedRelations?.pathTokens?.map(normalizeName) +export function resolveAreaSubPath (area: Partial): string { + const paths: string[] = area?.embeddedRelations?.ancestors.map((a) => normalizeName(a.name)) .map(token => token ?? '') .filter(token => token !== '') ?? [] return path.join(...paths) diff --git a/src/db/utils/jobs/MapTiles/exportCmd.ts b/src/db/utils/jobs/MapTiles/exportCmd.ts index 87b7929b..0a82b1e4 100644 --- a/src/db/utils/jobs/MapTiles/exportCmd.ts +++ b/src/db/utils/jobs/MapTiles/exportCmd.ts @@ -23,6 +23,7 @@ import { logger } from '../../../../logger.js' import { ClimbType } from '../../../ClimbTypes.js' import MutableMediaDataSource from '../../../../model/MutableMediaDataSource.js' import { workingDir } from './init.js' +import { muuidToString } from '../../../../utils/helpers.js' const MEDIA_PROJECTION = { width: 1, @@ -73,9 +74,10 @@ async function exportLeafCrags (): Promise { totalClimbs } = doc - const { pathTokens, ancestors } = doc.embeddedRelations + const { ancestors } = doc.embeddedRelations + const ancestorArray = ancestors.map(i => muuidToString(i.uuid)) + const pathTokens = ancestors.map(i => i.name) - const ancestorArray = ancestors.split(',') const pointFeature = point( doc.metadata.lnglat.coordinates, { diff --git a/src/graphql/resolvers.ts b/src/graphql/resolvers.ts index f83ed0da..1e23369c 100644 --- a/src/graphql/resolvers.ts +++ b/src/graphql/resolvers.ts @@ -270,7 +270,7 @@ const resolvers = { organizations: async (node: AreaType, args: any, { dataSources }: Context) => { const { organizations } = dataSources - const areaIdsToSearch = [node.metadata.area_id, ...node.embeddedRelations.ancestors.split(',').map(s => muid.from(s))] + const areaIdsToSearch = [node.metadata.area_id, ...node.embeddedRelations.ancestors.map(i => i.uuid)] const associatedOrgsCursor = await organizations.findOrganizationsByFilter({ associatedAreaIds: { includes: areaIdsToSearch }, // Remove organizations that explicitly request not to be associated with this area. diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index 857c10d6..045834dc 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/src/model/AreaRelationsEmbeddings.ts @@ -1,39 +1,82 @@ import mongoose from 'mongoose' -import { getAreaModel } from '../db' +import AreaDataSource from './AreaDataSource' +import { AreaType } from '../db/AreaTypes' -export async function computeEmbeddedRelations (rootId: mongoose.Types.ObjectId): Promise { - const areaModel = getAreaModel() - const result = await areaModel.aggregate([ - { - $match: { _id: rootId } - }, - { - $graphLookup: { - from: 'areas', - startWith: '$parent', - connectFromField: 'parent', - connectToField: '_id', - as: 'computed_ancestors', - depthField: 'depth' - } - }, - { - $addFields: { - ancestorIndex: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor._id' } }, - pathTokens: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor.area_name' } }, - children: [], // Initialize empty children array - ancestors: { $map: { input: '$computed_ancestors', as: 'ancestor', in: '$$ancestor.area_name' } } - } - }, - { - $project: { - ancestors: 1, - ancestorIndex: 1, - pathTokens: 1, - children: 1 - } +export class AreaRelationsEmbeddings { + constructor (public areaModel: AreaDataSource['areaModel']) {} + + /*** + * For a given area, ensure that the parent has a forward link to it in its embedded + * relations. + */ + async ensureChildReference (area: AreaType): Promise { + if (area.parent === undefined) { + throw new Error('No child reference can be reified for this area because its parent is undefined.') } - ]) + await this.areaModel.updateOne( + { _id: area.parent }, + { $addToSet: { 'embeddedRelations.children': area._id } } + ) + } + + /** + * For a given area, delete any child references that exist that are no longer + * backed by the parent reference. + */ + async deleteStaleReferences (area: AreaType): Promise { + await this.areaModel.updateMany( + { _id: { $ne: area.parent }, 'embeddedRelations.children': area._id }, + { $pull: { 'embeddedRelations.children': area._id } } + ) + } + + /** + * When an area changes its parent reference there are some effects that need to be processed. + * Its parent needs to be informed of the change, its old parent needs to have its index invalidated, + * and all of its children may need to be informed of the change - since they hold denormalized data + * regarding thier ancestry. + */ + async computeEmbeddedAncestors (area: AreaType): Promise { + await Promise.all([ + // ensure the parent has a reference to this area + this.ensureChildReference(area), + // ensure there are no divorced references to this area + this.deleteStaleReferences(area) + ]) + } + + async syncNamesInEmbeddings (area: AreaType): Promise { + await this.areaModel.updateMany( + { 'embeddedRelations.ancestors._id': area._id }, + { $set: { 'embeddedRelations.ancestors.$[elem].name': area.area_name } }, + { arrayFilters: [{ 'elem._id': area._id }] } + ) + } - throw new Error('not implemented yet') + async computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise> { + return await this.areaModel.aggregate([ + { $match: { _id } }, + { + $graphLookup: { + from: this.areaModel.collection.name, + startWith: '$parent', + // connect parent -> _id to trace up the tree + connectFromField: 'parent', + connectToField: '_id', + as: 'ancestor', + depthField: 'level' + } + }, + { + $unwind: '$ancestor' + }, + { + $project: { + _id: 0, + ancestor: 1 + } + }, + { $sort: { 'ancestor.level': -1 } } + ]) + } } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index 692477ae..ceef1c31 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -12,15 +12,12 @@ import CountriesLngLat from '../data/countries-with-lnglat.json' assert {type: ' import { AreaDocumnent, AreaEditableFieldsType, - AreaEmbeddedRelations, AreaType, OperationType, UpdateSortingOrderType } from '../db/AreaTypes.js' import { ChangeRecordMetadataType } from '../db/ChangeLogType.js' import { ExperimentalAuthorType } from '../db/UserTypes.js' -import { makeDBArea } from '../db/import/usa/AreaTransformer.js' -import { createRootNode } from '../db/import/usa/AreaTree.js' import { leafReducer, nodesReducer, StatsSummary } from '../db/utils/jobs/TreeUpdaters/updateAllAreas.js' import { bboxFrom } from '../geo-utils.js' import { logger } from '../logger.js' @@ -28,7 +25,9 @@ import { createInstance as createExperimentalUserDataSource } from '../model/Exp import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' -import { muuidToString, withTransaction } from '../utils/helpers.js' +import { withTransaction } from '../utils/helpers.js' +import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings' +import { GradeContexts } from '../GradeUtils' isoCountries.registerLocale(enJson) @@ -50,8 +49,39 @@ export interface UpdateAreaOptions { session?: ClientSession } +const defaultArea = { + shortCode: '', + metadata: { + isDestination: false, + leaf: false, + leftRightIndex: -1, + ext_id: '' + }, + climbs: [], + embeddedRelations: { + children: [] + }, + aggregate: { + byGrade: [], + byDiscipline: {}, + byGradeBand: { + unknown: 0, + beginner: 0, + intermediate: 0, + advanced: 0, + expert: 0 + } + }, + density: 0, + totalClimbs: 0, + content: { + description: '' + } +} + export default class MutableAreaDataSource extends AreaDataSource { experimentalUserDataSource = createExperimentalUserDataSource() + relations = new AreaRelationsEmbeddings(this.areaModel) private areaNameCompare (name: string): string { return name.trim().toLocaleLowerCase().split(' ').filter(i => i !== '').join(' ') @@ -127,16 +157,30 @@ export default class MutableAreaDataSource extends AreaDataSource { // Country code can be either alpha2 or 3. Let's convert it to alpha3. const alpha3 = countryCode.length === 2 ? isoCountries.toAlpha3(countryCode) : countryCode const countryName = isoCountries.getName(countryCode, 'en') - const countryNode = createRootNode(alpha3, countryName) - - // Build the Mongo document to be inserted - const doc = makeDBArea(countryNode) - doc.shortCode = alpha3 + const _id = new mongoose.Types.ObjectId() + const uuid = countryCode2Uuid(countryCode) + const country: AreaType = { + area_name: countryName, + ...defaultArea, + embeddedRelations: { + ...defaultArea.embeddedRelations, + ancestors: [{ _id, uuid, name: countryName }] + }, + metadata: { + ...defaultArea.metadata, + lnglat: CountriesLngLat[alpha3]?.lnglat, + area_id: uuid + }, + _id, + uuid, + gradeContext: GradeContexts.US + } // Look up the country lat,lng const entry = CountriesLngLat[alpha3] + if (entry != null) { - doc.metadata.lnglat = { + country.metadata.lnglat = { type: 'Point', coordinates: entry.lnglat } @@ -147,7 +191,7 @@ export default class MutableAreaDataSource extends AreaDataSource { await this.validateUniqueAreaName(countryName, null) - const rs = await this.areaModel.insertMany(doc) + const rs = await this.areaModel.insertMany(country) if (rs.length === 1) { return await rs[0].toObject() } @@ -243,7 +287,8 @@ export default class MutableAreaDataSource extends AreaDataSource { draft.prevHistoryId = parent._change?.historyId }) - const newArea = newAreaHelper(areaName, parent) + const newArea = this.subAreaHelper(areaName, parent) + if (isLeaf != null) { newArea.metadata.leaf = isLeaf } @@ -412,7 +457,8 @@ export default class MutableAreaDataSource extends AreaDataSource { area.set({ _change }) area.updatedBy = experimentalAuthorId ?? user - if (area.embeddedRelations.pathTokens.length === 1) { + // If this is a root area we disallow typical editing of it, as it is likely a country. + if (area.parent === undefined) { if (areaName != null || shortCode != null) throw new Error(`[${area.area_name}]: Area update error. Reason: Updating country name or short code is not allowed.`) } @@ -423,7 +469,8 @@ export default class MutableAreaDataSource extends AreaDataSource { if (areaName != null) { const sanitizedName = sanitizeStrict(areaName) area.set({ area_name: sanitizedName }) - await this.computeEmbeddedAncestors(area) + // sync names in all relevant references to this area. + await this.relations.syncNamesInEmbeddings(area) } if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) @@ -475,75 +522,6 @@ export default class MutableAreaDataSource extends AreaDataSource { } } - async passEmbeddedToChildren (_id: mongoose.Types.ObjectId, embeddedRelations?: AreaEmbeddedRelations, tree?: AreaType[]): Promise { - if (tree === undefined) { - tree = [] - } - - const area: Pick | null = await this.areaModel.findOne({ _id }, { embeddedRelations: 1, area_name: 1, 'metadata.area_id': 1 }) - if (area === null) { - logger.error('Programming error: A bad ID was passed from parent - a child is missing.') - return [] - } - - await Promise.all(area.embeddedRelations.children.map(async child => { - const ctx = { ...(embeddedRelations ?? area.embeddedRelations) } - ctx.ancestorIndex.push() - return await this.passEmbeddedToChildren(child, ctx) - })) - - return tree - } - - async computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise> { - return await this.areaModel.aggregate([ - { $match: { _id } }, - { - $graphLookup: { - from: this.areaModel.collection.name, - startWith: '$parent', - // connect parent -> _id to trace up the tree - connectFromField: 'parent', - connectToField: '_id', - as: 'ancestor', - depthField: 'level' - } - }, - { - $unwind: '$ancestor' - }, - { - $project: { - _id: 0, - ancestor: 1 - } - }, - { $sort: { 'ancestor.level': -1 } } - ]) - } - - /** - * When an area changes its parent it needs to have its children recieve the new derived data. - * There is a challenge here in that sharding the gql server may produce side-effects. - */ - async computeEmbeddedAncestors (area: AreaType): Promise { - await Promise.all([ - // ensure the parent has a reference to this area - this.areaModel.updateOne({ _id: area.parent }, { $addToSet: { 'embeddedRelations.children': area._id } }), - // ensure there are no divorced references to this area - this.areaModel.updateMany( - { _id: { $ne: area.parent }, 'embeddedRelations.children': area._id }, - { $pull: { 'embeddedRelations.children': area._id } } - ), - // We now compute the ancestors for this node - this.computeAncestorsFor(area._id).then(ancestors => { - }) - ]) - - // With all previous steps resolved we can pass the embedded data here to the children. - await Promise.all(area.embeddedRelations.children.map(async (child) => await this.passEmbeddedToChildren(child))) - } - /** * * @param user user id @@ -599,6 +577,36 @@ export default class MutableAreaDataSource extends AreaDataSource { return ret } + private subAreaHelper (areaName: string, parent: AreaType): AreaType { + const _id = new mongoose.Types.ObjectId() + const uuid = muuid.v4() + + return { + ...defaultArea, + _id, + uuid, + parent: parent._id, + area_name: areaName, + gradeContext: parent.gradeContext, + metadata: { + ...defaultArea.metadata, + area_id: uuid + }, + embeddedRelations: { + ...defaultArea.embeddedRelations, + // Initialize the ancestors by extending the parent's denormalized data + ancestors: [ + ...parent.embeddedRelations.ancestors, + { + _id, + uuid, + name: areaName + } + ] + } + } satisfies AreaType + } + /** * Update area stats and geo data for a given leaf node and its ancestors. * @param session @@ -611,15 +619,13 @@ export default class MutableAreaDataSource extends AreaDataSource { * Update function. For each node, recalculate stats and recursively update its acenstors until we reach the country node. */ const updateFn = async (session: ClientSession, changeRecord: ChangeRecordMetadataType, area: AreaDocumnent, childSummary?: StatsSummary): Promise => { - if (area.embeddedRelations.pathTokens.length <= 1) { + if (area.parent === undefined) { // we're at the root country node return } - const ancestors = area.embeddedRelations.ancestors.split(',') - const parentUuid = muuid.from(ancestors[ancestors.length - 2]) const parentArea = - await this.areaModel.findOne({ 'metadata.area_id': parentUuid }) + await this.areaModel.findOne({ _id: area.parent }) .batchSize(10) .populate<{ embeddedRelations: { children: AreaDocumnent[] } }>({ path: 'embeddedRelations.children', @@ -631,11 +637,6 @@ export default class MutableAreaDataSource extends AreaDataSource { const acc: StatsSummary[] = [] - if (parentArea.embeddedRelations.children === null) { - console.log(await this.areaModel.findOne({ 'metadata.area_id': parentUuid })) - throw new Error('Oopie doopers') - } - /** * Collect existing stats from all children. For affected node, use the stats from previous calculation. */ @@ -675,52 +676,6 @@ export default class MutableAreaDataSource extends AreaDataSource { } } -export const newAreaHelper = (areaName: string, parent: AreaType): AreaType => { - const _id = new mongoose.Types.ObjectId() - const uuid = muuid.v4() - - return { - _id, - uuid, - parent: parent._id, - shortCode: '', - area_name: areaName, - metadata: { - isDestination: false, - leaf: false, - area_id: uuid, - leftRightIndex: -1, - ext_id: '' - }, - climbs: [], - embeddedRelations: { - children: [], - // ancestors and pathTokens terminate at the present node, while the ancestorIndex - // terminates at the parent. - ancestors: parent.embeddedRelations.ancestors + `,${muuidToString(uuid)}`, - pathTokens: [...parent.embeddedRelations.pathTokens, areaName], - ancestorIndex: [...parent.embeddedRelations.ancestorIndex, parent._id] - }, - gradeContext: parent.gradeContext, - aggregate: { - byGrade: [], - byDiscipline: {}, - byGradeBand: { - unknown: 0, - beginner: 0, - intermediate: 0, - advanced: 0, - expert: 0 - } - }, - density: 0, - totalClimbs: 0, - content: { - description: '' - } - } -} - export const countryCode2Uuid = (code: string): MUUID => { if (!isoCountries.isValid(code)) { throw new Error(`Invalid country code: ${code}. Expect alpha2 or alpha3`) diff --git a/src/model/MutableMediaDataSource.ts b/src/model/MutableMediaDataSource.ts index d9bddfc5..b013a92e 100644 --- a/src/model/MutableMediaDataSource.ts +++ b/src/model/MutableMediaDataSource.ts @@ -5,6 +5,7 @@ import muuid from 'uuid-mongodb' import MediaDataSource from './MediaDataSource.js' import { EntityTag, EntityTagDeleteInput, MediaObject, MediaObjectGQLInput, AddTagEntityInput, NewMediaObjectDoc } from '../db/MediaObjectTypes.js' import MutableAreaDataSource from './MutableAreaDataSource.js' +import { muuidToString } from '../utils/helpers.js' export default class MutableMediaDataSource extends MediaDataSource { areaDS = MutableAreaDataSource.getInstance() @@ -25,7 +26,7 @@ export default class MutableMediaDataSource extends MediaDataSource { _id: new mongoose.Types.ObjectId(), targetId: entityUuid, type: entityType, - ancestors: climb.parent.embeddedRelations.ancestors, + ancestors: climb.parent.embeddedRelations.ancestors.map(i => muuidToString(i.uuid)).join(','), climbName: climb.name, areaName: climb.parent.area_name, lnglat: climb.metadata.lnglat @@ -47,7 +48,7 @@ export default class MutableMediaDataSource extends MediaDataSource { _id: new mongoose.Types.ObjectId(), targetId: entityUuid, type: entityType, - ancestors: area.embeddedRelations.ancestors, + ancestors: area.embeddedRelations.ancestors.map(i => i.uuid).join(','), areaName: area.area_name, lnglat: area.metadata.lnglat } diff --git a/src/model/__tests__/AreaRelationsEmbeddings.test.ts b/src/model/__tests__/AreaRelationsEmbeddings.test.ts new file mode 100644 index 00000000..0d16fa40 --- /dev/null +++ b/src/model/__tests__/AreaRelationsEmbeddings.test.ts @@ -0,0 +1,153 @@ +import { MUUID } from "uuid-mongodb" +import { AreaType } from "../../db/AreaTypes" +import MutableAreaDataSource from "../MutableAreaDataSource" +import muid from 'uuid-mongodb' +import { getAreaModel, createIndexes } from "../../db" +import inMemoryDB from "../../utils/inMemoryDB" +import { muuidToString } from "../../utils/helpers" + + +describe("updating of areas should propogate embeddedRelations", () => { + 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) + + const defaultDepth = 5 + async function growTree(depth: number = defaultDepth, bredth: number = 1): Promise { + const tree: AreaType[] = [rootCountry, await addArea()] + + async function grow(from: AreaType, level: number = 0) { + if (level >= depth) return + + await Promise.all(Array.from({ length: bredth }) + .map((_ ,idx) => addArea(`${level}-${idx}`, { parent: from }) + .then(area => { + if (!area.parent?.equals(from._id)) { + throw new Error(`${area.parent} should have been ${from._id}`) + } + tree.push(area) + return grow(area, level + 1) + }))) + } + + await grow(tree.at(-1)!) + + return tree + } + + test('computing ancestors from reified node', async () => growTree().then(async (tree) => { + let computedAncestors = await areas.relations.computeAncestorsFor(tree.at(-1)!._id) + + // 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. + expect(computedAncestors.length).toBe(tree.length - 1) + expect(computedAncestors).not.toContainEqual(tree.at(-1)) + expect(computedAncestors.map(i => i.ancestor._id).join(",")).toEqual(tree.slice(0, -1).map(i => i._id).join(",")) + expect(computedAncestors.map(i => i.ancestor.area_name).join()).toEqual(tree.slice(0, -1).map(i => i.area_name).join()) + + + // Check that each node refers specifically to the previous one as its parent + // - this will check that the areas are in order and that no nodes are skipped. + computedAncestors.reduce((previous, current, idx) => { + expect(current.ancestor.parent?.equals(previous.ancestor._id)) + expect(current.ancestor._id.equals(tree[idx]._id)) + return current + }) + })) + + test('ancestors should be computed on area add.', async () => growTree(5).then(async (tree) => { + let leaf = tree.at(-1)! + expect(leaf.embeddedRelations.ancestors.map(i => i.name).join(',')).toEqual(tree.map(i => i.area_name).join(',')) + expect(leaf.embeddedRelations.ancestors.map(i => muuidToString(i.uuid)).join(',')).toEqual(tree.map(i => i.metadata.area_id).join(',')) + expect(leaf.embeddedRelations.ancestors.map(i => i._id).join(',')).toEqual(tree.map(i => i._id).join(',')) + })) + + test("creating an area should update its immediate parent's children", async () => growTree(3).then(async (tree) => { + // add a new child to leaf + let leaf = await addArea(undefined, { parent: tree.at(-1)! }) + let parent = await areas.findOneAreaByUUID(tree.at(-1)!.metadata.area_id) + expect(parent.embeddedRelations.children).toContainEqual(leaf._id) + })) + + 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 areas.updateArea( + testUser, + tree[target].metadata.area_id, { + areaName: 'updated name' + }, + ) + + tree = (await areas.relations.computeAncestorsFor(tree.at(-1)!._id)).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') + })) + + test.todo("moving a leaf area to a new parent should update its old and new parent") + test.todo("moving an area with children to a new parent should update its old and new parent") + test.todo("moving an area with children to a new parent should update all of its children embeddings") + + test("re-naming a parent should update all descendant pathTokens", async () => growTree(5, 2).then(async tree => { + let target = 1 + let oldName = tree[target].area_name + await areas.updateArea( + testUser, + tree[target].metadata.area_id, { + areaName: 'updated name' + }, + ) + + // Check every node in the tree, with nodes of a certain depth needing to have their pathtokens checked. + for (const node of tree.filter(i => i.embeddedRelations.ancestors.length >= target)) { + let area = await areas.findOneAreaByUUID(node.metadata.area_id) + expect(area.embeddedRelations.ancestors.map(i => i.name)[target]).not.toEqual(oldName) + expect(area.embeddedRelations.ancestors.map(i => i.name)[target]).toEqual('updated name') + } + })) +}) diff --git a/src/model/__tests__/BulkDataSource.test.ts b/src/model/__tests__/BulkDataSource.test.ts index 0603f4ff..fdc753b3 100644 --- a/src/model/__tests__/BulkDataSource.test.ts +++ b/src/model/__tests__/BulkDataSource.test.ts @@ -144,26 +144,32 @@ describe('bulk import e2e', () => { addedAreas: [ { area_name: 'Test Area', - embeddedRelations: { pathTokens: ['United States of America', 'Test Area'] }, + embeddedRelations: { ancestors: ['United States of America', 'Test Area'] + .map(name => ({ name })) + }, }, { area_name: 'Test Area 2', - embeddedRelations: { pathTokens: [ + embeddedRelations: { ancestors: [ 'United States of America', 'Test Area', 'Test Area 2', - ] }, + ] + .map(name => ({ name })) + }, }, { area_name: 'Test Area 3', - embeddedRelations: { pathTokens: [ + embeddedRelations: { ancestors: [ 'United States of America', 'Test Area', 'Test Area 2', 'Test Area 3', - ] }, + ] + .map(name => ({ name })) }, - ] satisfies Array & { embeddedRelations: Partial }>>, + }, + ], }); }); diff --git a/src/model/__tests__/MediaDataSource.ts b/src/model/__tests__/MediaDataSource.ts index 17191d71..6164483c 100644 --- a/src/model/__tests__/MediaDataSource.ts +++ b/src/model/__tests__/MediaDataSource.ts @@ -132,7 +132,7 @@ describe('MediaDataSource', () => { targetId: climbTag.entityUuid, type: climbTag.entityType, areaName: areaForTagging1.area_name, - ancestors: areaForTagging1.embeddedRelations.ancestors, + ancestors: areaForTagging1.embeddedRelations.ancestors.map(i => i.uuid).join(","), climbName: newSportClimb1.name, lnglat: areaForTagging1.metadata.lnglat }) diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index c11daeed..1f66cfe1 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -195,46 +195,4 @@ describe("Test area mutations", () => { await addArea(nameShadow, { boulder: true, parent }) })) - describe("updating of areas should propogate embeddedRelations", () => { - const defaultDepth = 5 - async function growTree(depth: number = defaultDepth, bredth: number = 1): Promise { - const tree: AreaType[] = [rootCountry, await addArea()] - - async function grow(from: AreaType, level: number = 0) { - if (level >= depth) return - - await Promise.all(Array.from({ length: bredth }) - .map((_ ,idx) => addArea(`${level}-${idx}`, { parent: from }) - .then(area => { - if (!area.parent?.equals(from._id)) { - throw new Error(`${area.parent} should have been ${from._id}`) - } - tree.push(area) - return grow(area, level + 1) - }))) - } - - await grow(tree.at(-1)!) - - return tree - } - - test('computing ancestors from reified node', async () => growTree().then(async (tree) => { - let computedAncestors = await areas.computeAncestorsFor(tree.at(-1)!._id) - - // Check that each node refers specifically to the previous one as its parent - // - this will check that the areas are in order and that no nodes are skipped. - computedAncestors.reduce((previous, current, idx) => { - expect(current.ancestor.parent?.equals(previous.ancestor._id)) - expect(current.ancestor._id.equals(tree[idx]._id)) - return current - }) - })) - - test('ancestors should be computed on area add.', async () => growTree(5).then(async (tree) => { - let leaf = tree.at(-1)! - expect(leaf.embeddedRelations.pathTokens.join(',')).toEqual(tree.map(i => i.area_name).join(',')) - expect(leaf.embeddedRelations.ancestors).toEqual(tree.map(i => i.metadata.area_id).join(',')) - })) - }) }) \ No newline at end of file diff --git a/src/model/__tests__/updateAreas.ts b/src/model/__tests__/updateAreas.ts index e366c054..9758c8c2 100644 --- a/src/model/__tests__/updateAreas.ts +++ b/src/model/__tests__/updateAreas.ts @@ -6,6 +6,7 @@ import MutableClimbDataSource from '../MutableClimbDataSource.js' import { createIndexes, getAreaModel, getClimbModel } from '../../db/index.js' import { AreaEditableFieldsType, UpdateSortingOrderType } from '../../db/AreaTypes.js' import inMemoryDB from '../../utils/inMemoryDB.js' +import { muuidToString } from '../../utils/helpers.js' describe('Areas', () => { let areas: MutableAreaDataSource @@ -70,9 +71,10 @@ describe('Areas', () => { // Verify paths and ancestors if (theBug != null) { // make TS happy - expect(theBug.embeddedRelations.ancestors) + expect(theBug.embeddedRelations.ancestors.map(i => muuidToString(i.uuid)).join(',')) .toEqual(`${canada.metadata.area_id.toUUID().toString()},${theBug?.metadata.area_id.toUUID().toString()}`) - expect(theBug.embeddedRelations.pathTokens) + + expect(theBug.embeddedRelations.ancestors.map(i => i.name)) .toEqual([canada.area_name, theBug.area_name]) } }) diff --git a/src/utils/testUtils.ts b/src/utils/testUtils.ts index ed13ce2e..96a6a96e 100644 --- a/src/utils/testUtils.ts +++ b/src/utils/testUtils.ts @@ -21,7 +21,7 @@ export interface QueryAPIProps { body?: any } -/* +/** * Helper function for querying the locally-served API. It mocks JWT verification * so we can pretend to have an role we want when calling the API. */ From 431f285fa4c888c6f573f5428010898605c7cea5 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Thu, 14 Nov 2024 14:29:29 +0200 Subject: [PATCH 23/27] Area ancestry deconvolution There is a new, simple, source of truth for area structure derivations. Each area document holds a parent reference, which points to its desired reference (or none, if it is a root node). The derived fields such as - Children - Ancestry have been collated in `embeddedRelations`. the denormalized fields `ancestors` and `pathTokens` have also been colocated into a single entity path that makes propogating updates to denormalized data easier (e.g, when updating an areas name). It leaves us a space for adding new fields, and finally exposes the ACTUAL id of the document so that we can index effectively. I would suggest that using the parent field is the most straightforward and safest thing to do in pretty much all scenarios, but if you are a mongodb veteran you may find need for the array indexing features - though as far as I can tell, this is rarely the case? - Added tests - Updated some existing tests - Data returned at the gql layer should be identical, but I have not fully de-risked those tests. All I can say is that they are passing. This commit does not address #126, but gives us an enviable headstart at it. It is however needed by #407 --- db-migrations/0006-area-structure.js | 3 + src/model/AreaRelationsEmbeddings.ts | 12 +- src/model/MutableAreaDataSource.ts | 9 +- .../__tests__/AreaRelationsEmbeddings.test.ts | 38 ++++--- src/model/__tests__/MediaDataSource.ts | 3 +- src/model/__tests__/MutableClimbDataSource.ts | 7 +- src/model/__tests__/updateAreas.ts | 106 +----------------- 7 files changed, 45 insertions(+), 133 deletions(-) diff --git a/db-migrations/0006-area-structure.js b/db-migrations/0006-area-structure.js index 9921759e..ef64255c 100644 --- a/db-migrations/0006-area-structure.js +++ b/db-migrations/0006-area-structure.js @@ -91,6 +91,9 @@ printjson(db.areas.createIndex({ 'embeddedRelations.ancestors._id': 1 })) printjson(db.areas.createIndex({ 'embeddedRelations.ancestors.uuid': 1 })) printjson(db.areas.createIndex({ 'embeddedRelations.ancestors.name': 1 })) +printjson(db.areas.createIndex({ 'embeddedRelations.ancestors._id': 1, 'embeddedRelations.ancestors.name': 1 })) + + // https://www.mongodb.com/docs/v6.2/reference/method/db.collection.createIndex/#create-an-index-on-a-multiple-fields // > The order of fields in a compound index is important for supporting sort() operations using the index. // It is not clear to me if there is a $lookup speed implication based on the direction of the join. diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index 045834dc..c76d31b8 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/src/model/AreaRelationsEmbeddings.ts @@ -13,6 +13,7 @@ export class AreaRelationsEmbeddings { if (area.parent === undefined) { throw new Error('No child reference can be reified for this area because its parent is undefined.') } + await this.areaModel.updateOne( { _id: area.parent }, { $addToSet: { 'embeddedRelations.children': area._id } } @@ -45,11 +46,18 @@ export class AreaRelationsEmbeddings { ]) } + /** + * When an area name changes, there may be denormalized references to it elsewhere in the collection + * that we would like to change. + */ async syncNamesInEmbeddings (area: AreaType): Promise { await this.areaModel.updateMany( - { 'embeddedRelations.ancestors._id': area._id }, + // TODO: My vision for this function was that the (exists.name != new.name) clause should not have been necessary, + // but the function goes into a spin-loop otherwise. So, perhaps a changestream is firing somewhere else. + // I didn't spend much time in the debugger parsing the stack, but I would like to know what's happening here. + { 'embeddedRelations.ancestors._id': area._id, 'embeddedRelations.ancestors.name': { $ne: area.area_name } }, { $set: { 'embeddedRelations.ancestors.$[elem].name': area.area_name } }, - { arrayFilters: [{ 'elem._id': area._id }] } + { arrayFilters: [{ 'elem._id': area._id }], timestamps: false } ) } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index ceef1c31..b71c34d8 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -26,8 +26,8 @@ import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' import { withTransaction } from '../utils/helpers.js' -import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings' -import { GradeContexts } from '../GradeUtils' +import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings.js' +import { getCountriesDefaultGradeContext, GradeContexts } from '../GradeUtils.js' isoCountries.registerLocale(enJson) @@ -160,8 +160,9 @@ export default class MutableAreaDataSource extends AreaDataSource { const _id = new mongoose.Types.ObjectId() const uuid = countryCode2Uuid(countryCode) const country: AreaType = { - area_name: countryName, ...defaultArea, + area_name: countryName, + shortCode: alpha3, embeddedRelations: { ...defaultArea.embeddedRelations, ancestors: [{ _id, uuid, name: countryName }] @@ -173,7 +174,7 @@ export default class MutableAreaDataSource extends AreaDataSource { }, _id, uuid, - gradeContext: GradeContexts.US + gradeContext: getCountriesDefaultGradeContext()[alpha3] ?? GradeContexts.US } // Look up the country lat,lng diff --git a/src/model/__tests__/AreaRelationsEmbeddings.test.ts b/src/model/__tests__/AreaRelationsEmbeddings.test.ts index 0d16fa40..08de4ff3 100644 --- a/src/model/__tests__/AreaRelationsEmbeddings.test.ts +++ b/src/model/__tests__/AreaRelationsEmbeddings.test.ts @@ -6,13 +6,24 @@ import { getAreaModel, createIndexes } from "../../db" import inMemoryDB from "../../utils/inMemoryDB" import { muuidToString } from "../../utils/helpers" - describe("updating of areas should propogate embeddedRelations", () => { let areas: MutableAreaDataSource let rootCountry: AreaType let areaCounter = 0 const testUser = muid.v4() + 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) + 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' @@ -43,18 +54,6 @@ describe("updating of areas should propogate embeddedRelations", () => { ) } - 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) - const defaultDepth = 5 async function growTree(depth: number = defaultDepth, bredth: number = 1): Promise { const tree: AreaType[] = [rootCountry, await addArea()] @@ -129,10 +128,6 @@ describe("updating of areas should propogate embeddedRelations", () => { expect(tree[target].embeddedRelations.ancestors.map(i => i.name)[target]).toEqual('updated name') })) - test.todo("moving a leaf area to a new parent should update its old and new parent") - test.todo("moving an area with children to a new parent should update its old and new parent") - test.todo("moving an area with children to a new parent should update all of its children embeddings") - test("re-naming a parent should update all descendant pathTokens", async () => growTree(5, 2).then(async tree => { let target = 1 let oldName = tree[target].area_name @@ -144,10 +139,17 @@ describe("updating of areas should propogate embeddedRelations", () => { ) // Check every node in the tree, with nodes of a certain depth needing to have their pathtokens checked. - for (const node of tree.filter(i => i.embeddedRelations.ancestors.length >= target)) { + for (const node of tree.filter(i => i.embeddedRelations.ancestors.length > target)) { let area = await areas.findOneAreaByUUID(node.metadata.area_id) expect(area.embeddedRelations.ancestors.map(i => i.name)[target]).not.toEqual(oldName) expect(area.embeddedRelations.ancestors.map(i => i.name)[target]).toEqual('updated name') } })) + + + describe("effects related to changing an areas parent", () => { + test.todo("moving a leaf area to a new parent should update its old and new parent") + test.todo("moving an area with children to a new parent should update its old and new parent") + test.todo("moving an area with children to a new parent should update all of its children embeddings") + }) }) diff --git a/src/model/__tests__/MediaDataSource.ts b/src/model/__tests__/MediaDataSource.ts index 6164483c..558b32b7 100644 --- a/src/model/__tests__/MediaDataSource.ts +++ b/src/model/__tests__/MediaDataSource.ts @@ -16,6 +16,7 @@ import { } from '../../db/MediaObjectTypes.js' import { newSportClimb1 } from './MutableClimbDataSource.js' import inMemoryDB from '../../utils/inMemoryDB.js' +import { muuidToString } from '../../utils/helpers.js' const TEST_MEDIA: MediaObjectGQLInput = { userUuid: 'a2eb6353-65d1-445f-912c-53c6301404bd', @@ -132,7 +133,7 @@ describe('MediaDataSource', () => { targetId: climbTag.entityUuid, type: climbTag.entityType, areaName: areaForTagging1.area_name, - ancestors: areaForTagging1.embeddedRelations.ancestors.map(i => i.uuid).join(","), + ancestors: areaForTagging1.embeddedRelations.ancestors.map(i => muuidToString(i.uuid)).join(','), climbName: newSportClimb1.name, lnglat: areaForTagging1.metadata.lnglat }) diff --git a/src/model/__tests__/MutableClimbDataSource.ts b/src/model/__tests__/MutableClimbDataSource.ts index 16a5808c..1cf469f1 100644 --- a/src/model/__tests__/MutableClimbDataSource.ts +++ b/src/model/__tests__/MutableClimbDataSource.ts @@ -464,7 +464,7 @@ describe('Climb CRUD', () => { { ...newSportClimb1, grade: 'V6' }]) // bad UIAA grade (V-scale used) expect(newIDs).toHaveLength(4) - + console.log(muid.from(newIDs[0])) const climb1 = await climbs.findOneClimbByMUUID(muid.from(newIDs[0])) expect(climb1?.grades).toEqual({ uiaa: '6+' }) @@ -479,7 +479,8 @@ describe('Climb CRUD', () => { }) it('can update boulder problems', async () => { - const newDestination = await areas.addArea(testUser, 'Bouldering area A100', null, 'fr') + const gradeContext = 'fr' + const newDestination = await areas.addArea(testUser, 'Bouldering area A100', null, gradeContext) if (newDestination == null) fail('Expect new area to be created') @@ -518,7 +519,7 @@ describe('Climb CRUD', () => { expect(updated).toHaveLength(2) const actual1 = await climbs.findOneClimbByMUUID(muid.from(newIDs[0])) - + expect(actual1?.gradeContext?.toLocaleLowerCase()).toBe(gradeContext) expect(actual1).toMatchObject({ name: changes[0].name, grades: { diff --git a/src/model/__tests__/updateAreas.ts b/src/model/__tests__/updateAreas.ts index 9758c8c2..bd56298d 100644 --- a/src/model/__tests__/updateAreas.ts +++ b/src/model/__tests__/updateAreas.ts @@ -152,7 +152,7 @@ describe('Areas', () => { a1Updated = await areas.updateArea(testUser, a1?.metadata.area_id, doc2) expect(a1Updated?.metadata.lnglat).toEqual(geometry('Point', [doc2.lng, doc2.lat])) expect(a1Updated?.metadata.isDestination).toEqual(doc2.isDestination) - }) + }, 1000) it('should not update country name and code', async () => { const country = await areas.addCountry('lao') @@ -290,108 +290,4 @@ describe('Areas', () => { }) })) }) - - 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 236374046c94fa93682b5d60a4d07ae2bfc4dbea Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Mon, 18 Nov 2024 07:32:36 +0200 Subject: [PATCH 24/27] Checkpoint commit -- pls squash I'm going to go live in the mountains for a week or so, but will take a laptop so may want to work on this a bit. --- src/db/AreaTypes.ts | 8 ++ src/model/AreaRelationsEmbeddings.ts | 81 ++++++++++++++++--- src/model/MutableAreaDataSource.ts | 64 ++++++++++++++- .../__tests__/AreaRelationsEmbeddings.test.ts | 6 +- .../__tests__/MutableAreaDataSource.test.ts | 8 ++ src/utils/helpers.ts | 17 +++- 6 files changed, 162 insertions(+), 22 deletions(-) diff --git a/src/db/AreaTypes.ts b/src/db/AreaTypes.ts index de1cc4c4..67dfce8f 100644 --- a/src/db/AreaTypes.ts +++ b/src/db/AreaTypes.ts @@ -280,6 +280,14 @@ export enum OperationType { /** signals that a user has pushed new user-changeable data has been pushed into an area document. */ updateArea = 'updateArea', + /** + * signals that a user has reorganized an area structure by moving it (and, therefore, its children) to a new parent. + * Doing ancestor archeology cannot be established through derived fields, so there is minor complexity in tracing + * lineage over time, but all data required to build this audit trail should be preserved in the history of each + * documents .parent field. + **/ + changeAreaParent = 'changeAreaParent', + /** Set areas' sorting index */ orderAreas = 'orderArea' } diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index c76d31b8..a768b188 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/src/model/AreaRelationsEmbeddings.ts @@ -1,6 +1,7 @@ -import mongoose from 'mongoose' +import mongoose, { ClientSession } from 'mongoose' import AreaDataSource from './AreaDataSource' -import { AreaType } from '../db/AreaTypes' +import { AreaType, DenormalizedAreaSummary } from '../db/AreaTypes' +import { MUUID } from 'uuid-mongodb' export class AreaRelationsEmbeddings { constructor (public areaModel: AreaDataSource['areaModel']) {} @@ -9,7 +10,7 @@ export class AreaRelationsEmbeddings { * For a given area, ensure that the parent has a forward link to it in its embedded * relations. */ - async ensureChildReference (area: AreaType): Promise { + async ensureChildReference (area: AreaType, session: ClientSession): Promise { if (area.parent === undefined) { throw new Error('No child reference can be reified for this area because its parent is undefined.') } @@ -17,18 +18,20 @@ export class AreaRelationsEmbeddings { await this.areaModel.updateOne( { _id: area.parent }, { $addToSet: { 'embeddedRelations.children': area._id } } - ) + ).session(session) } /** * For a given area, delete any child references that exist that are no longer * backed by the parent reference. */ - async deleteStaleReferences (area: AreaType): Promise { + async deleteStaleReferences (area: AreaType, session: ClientSession): Promise { await this.areaModel.updateMany( + // The parent passed to us here is the DESIRED parent, not necessarily yet the reified + // parent - but might be. { _id: { $ne: area.parent }, 'embeddedRelations.children': area._id }, { $pull: { 'embeddedRelations.children': area._id } } - ) + ).session(session) } /** @@ -37,12 +40,14 @@ export class AreaRelationsEmbeddings { * and all of its children may need to be informed of the change - since they hold denormalized data * regarding thier ancestry. */ - async computeEmbeddedAncestors (area: AreaType): Promise { + async computeEmbeddedAncestors (area: AreaType, session: ClientSession): Promise { await Promise.all([ // ensure the parent has a reference to this area - this.ensureChildReference(area), + this.ensureChildReference(area, session), // ensure there are no divorced references to this area - this.deleteStaleReferences(area) + this.deleteStaleReferences(area, session), + // pass the embeddings down the hierarchy child-to-child. + this.syncEmbeddedRelations(area, session) ]) } @@ -50,7 +55,7 @@ export class AreaRelationsEmbeddings { * When an area name changes, there may be denormalized references to it elsewhere in the collection * that we would like to change. */ - async syncNamesInEmbeddings (area: AreaType): Promise { + async syncNamesInEmbeddings (area: AreaType, session: ClientSession): Promise { await this.areaModel.updateMany( // TODO: My vision for this function was that the (exists.name != new.name) clause should not have been necessary, // but the function goes into a spin-loop otherwise. So, perhaps a changestream is firing somewhere else. @@ -58,7 +63,7 @@ export class AreaRelationsEmbeddings { { 'embeddedRelations.ancestors._id': area._id, 'embeddedRelations.ancestors.name': { $ne: area.area_name } }, { $set: { 'embeddedRelations.ancestors.$[elem].name': area.area_name } }, { arrayFilters: [{ 'elem._id': area._id }], timestamps: false } - ) + ).session(session) } async computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise> { @@ -87,4 +92,58 @@ export class AreaRelationsEmbeddings { { $sort: { 'ancestor.level': -1 } } ]) } + + /** + * For a given area with a set parent reference, we want to perform a lookup at that node + * to get its ancestry and then pass that ancestry context down the tree updating children. + * + * .children + * and .ancestors + * + * will be updated with the relevant values. + */ + async syncEmbeddedRelations (area: AreaSinkReference, session: ClientSession, precompute?: DenormalizedAreaSummary[]): Promise { + if (precompute === undefined) { + precompute = (await this.computeAncestorsFor(area._id)).map(({ ancestor }) => ({ + name: ancestor.area_name, + _id: ancestor._id, + uuid: ancestor.metadata.area_id + })) + } else { + precompute = [ + ...precompute, + { + name: area.area_name, + _id: area._id, + uuid: area.metadata.area_id + }] + } + + const children = await this.areaModel.find( + { parent: area._id, _deleting: { $exists: false } }, + { _id: 1, area_name: 1, 'metadata.area_id': 1 } + ) + + await Promise.all([ + this.areaModel.updateOne( + { _id: area._id }, + { + 'embeddedRelations.ancestors': precompute, + // We've gone through the trouble of fetching this data, so we will update. + 'embeddedRelations.children': children.map(area => ({ + name: area.area_name, + _id: area._id, + uuid: area.metadata.area_id + })) + } + ).session(session), + ...children.map(async child => await this.syncEmbeddedRelations(child, session, precompute)) + ]) + } +} + +interface AreaSinkReference { + _id: mongoose.Types.ObjectId + area_name: string + metadata: { area_id: MUUID } } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index b71c34d8..c6450362 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -25,7 +25,7 @@ import { createInstance as createExperimentalUserDataSource } from '../model/Exp import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' -import { withTransaction } from '../utils/helpers.js' +import { resolveTransaction, useOrCreateTransaction, withTransaction } from '../utils/helpers.js' import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings.js' import { getCountriesDefaultGradeContext, GradeContexts } from '../GradeUtils.js' @@ -399,6 +399,66 @@ export default class MutableAreaDataSource extends AreaDataSource { return await this.updateArea(user, areaUuid, document, session) } + /** + * Modify an areas parent. This will mutate the areas parent reference, so all of its children + * will come along with it (Same as if you were to move a directory with files in it to another directory) + * + * Note: + * If you are a bolder free-soloist than I, you could try and merge this function into the updateArea + * function so that parent is a mutable field in the same way that areaName is. However: it is harder. + * Simple as that, you will have to deal with substantially more complex effects than simply treating + * it as its own sequential operation. + */ + async setAreaParent (user: MUUID, areaUuid: MUUID, newParent: MUUID, sessionCtx?: ClientSession): Promise { + return await resolveTransaction(this.areaModel, sessionCtx, async (session) => { + const area = await this.areaModel.findOne({ 'metadata.area_id': areaUuid }) + .orFail() + .session(session) + + if (area._deleting !== undefined) { + throw new UserInputError('This area is slated for deletion and cannot be edited') + } + + 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.') + } + + const oldParent = await this.areaModel.findOne({ + _id: area.parent + }).lean() + .session(session) + .orFail() + + if (oldParent._id.equals(area.parent)) { + // The request is a no-op, waste no time. nothing has changed, so submit. + throw new UserInputError('no-op, the requested parent is ALREADY the parent.') + } + + const nextParent = await this.areaModel.findById(oldParent._id).orFail().session(session) + + area.parent = nextParent._id + await this.relations.computeEmbeddedAncestors(area, session) + + const change = await changelogDataSource.create(session, user, OperationType.changeAreaParent) + + area.set({ + _change: { + user, + historyId: change._id, + prevHistoryId: area._change?.historyId._id, + operation: OperationType.changeAreaParent, + seq: 0 + } satisfies ChangeRecordMetadataType, + updatedBy: user, + }) + + const cursor = await area.save({ session }) + return await cursor.toObject() + }) + } + /** * Update one or more area fields. * @@ -471,7 +531,7 @@ export default class MutableAreaDataSource extends AreaDataSource { const sanitizedName = sanitizeStrict(areaName) area.set({ area_name: sanitizedName }) // sync names in all relevant references to this area. - await this.relations.syncNamesInEmbeddings(area) + await this.relations.syncNamesInEmbeddings(area, session) } if (shortCode != null) area.set({ shortCode: shortCode.toUpperCase() }) diff --git a/src/model/__tests__/AreaRelationsEmbeddings.test.ts b/src/model/__tests__/AreaRelationsEmbeddings.test.ts index 08de4ff3..d9d4c76c 100644 --- a/src/model/__tests__/AreaRelationsEmbeddings.test.ts +++ b/src/model/__tests__/AreaRelationsEmbeddings.test.ts @@ -147,9 +147,5 @@ describe("updating of areas should propogate embeddedRelations", () => { })) - describe("effects related to changing an areas parent", () => { - test.todo("moving a leaf area to a new parent should update its old and new parent") - test.todo("moving an area with children to a new parent should update its old and new parent") - test.todo("moving an area with children to a new parent should update all of its children embeddings") - }) + test.todo("syncEmbeddedRelations") }) diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 1f66cfe1..16b689b8 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -195,4 +195,12 @@ describe("Test area mutations", () => { await addArea(nameShadow, { boulder: true, parent }) })) + describe("cases for changing an areas parent",() => { + test.todo('Can update an areas parent reference') + test.todo('Updating an areas parents reference adds an area to its new parents children') + test.todo('Updating an areas parents reference REMOVED an area from its old parents children') + 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.todo('Updating an areas parent reference should update an areas child embeddedRelations') + }) }) \ No newline at end of file diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 4adc57fc..6ef028c7 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -30,10 +30,7 @@ export const canonicalizeUsername = (username: string): string => username.repla // see https://jira.mongodb.org/browse/NODE-2014 export const withTransaction = async (session: ClientSession, closure: () => Promise): Promise => { let result: T | undefined - await session.withTransaction(async () => { - result = await closure() - return result - }) + await session.withTransaction(async () => { result = await closure() }) as T return result } @@ -57,3 +54,15 @@ export const useOrCreateTransaction = async(owner: SessionStartable, session: } } } + +/** Like useOrCreateTransaction but will treat any call to `session.abortTransaction()` as an + * exception ( + * which is not necessarily best practice, but the assumption here is that we have no + * meaningful way of resolving data to a user unless the transaction succeeds all at once. + * ) + */ +export const resolveTransaction = async(owner: SessionStartable, session: ClientSession | undefined, closure: (session: ClientSession) => Promise): Promise => { + const result = await useOrCreateTransaction(owner, session, closure) + if (result === undefined) throw new Error('Transaction was explicitly ended but we did not account for that logic here') + return result +} From 99e77d86cc95135539d007521f7d975f13be2896 Mon Sep 17 00:00:00 2001 From: CocoIsBuggy Date: Mon, 25 Nov 2024 17:53:07 +0200 Subject: [PATCH 25/27] Checkpoint - back from mountains --- src/model/AreaRelationsEmbeddings.ts | 32 +++-- src/model/MutableAreaDataSource.ts | 15 ++- .../__tests__/AreaRelationsEmbeddings.test.ts | 21 ++- .../__tests__/MutableAreaDataSource.test.ts | 120 +++++++++++++++++- 4 files changed, 162 insertions(+), 26 deletions(-) diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index a768b188..23f1800b 100644 --- a/src/model/AreaRelationsEmbeddings.ts +++ b/src/model/AreaRelationsEmbeddings.ts @@ -66,6 +66,11 @@ export class AreaRelationsEmbeddings { ).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 computeAncestorsFor (_id: mongoose.Types.ObjectId): Promise> { return await this.areaModel.aggregate([ { $match: { _id } }, @@ -102,25 +107,24 @@ export class AreaRelationsEmbeddings { * * will be updated with the relevant values. */ - async syncEmbeddedRelations (area: AreaSinkReference, session: ClientSession, precompute?: DenormalizedAreaSummary[]): Promise { - if (precompute === undefined) { - precompute = (await this.computeAncestorsFor(area._id)).map(({ ancestor }) => ({ + async syncEmbeddedRelations (area: AreaSinkReference, session: ClientSession, ancestorPath?: DenormalizedAreaSummary[]): Promise { + if (ancestorPath === undefined) { + ancestorPath = (await this.computeAncestorsFor(area._id)).map(({ ancestor }) => ({ name: ancestor.area_name, _id: ancestor._id, uuid: ancestor.metadata.area_id })) - } else { - precompute = [ - ...precompute, - { - name: area.area_name, - _id: area._id, - uuid: area.metadata.area_id - }] } + ancestorPath = [ + ...ancestorPath, + { + name: area.area_name, + _id: area._id, + uuid: area.metadata.area_id + }] + const children = await this.areaModel.find( - { parent: area._id, _deleting: { $exists: false } }, { _id: 1, area_name: 1, 'metadata.area_id': 1 } ) @@ -128,7 +132,7 @@ export class AreaRelationsEmbeddings { this.areaModel.updateOne( { _id: area._id }, { - 'embeddedRelations.ancestors': precompute, + 'embeddedRelations.ancestors': ancestorPath, // We've gone through the trouble of fetching this data, so we will update. 'embeddedRelations.children': children.map(area => ({ name: area.area_name, @@ -137,7 +141,7 @@ export class AreaRelationsEmbeddings { })) } ).session(session), - ...children.map(async child => await this.syncEmbeddedRelations(child, session, precompute)) + ...children.map(async child => await this.syncEmbeddedRelations(child, session, ancestorPath)) ]) } } diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index c6450362..ffcb805b 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -25,7 +25,7 @@ import { createInstance as createExperimentalUserDataSource } from '../model/Exp import { sanitizeStrict } from '../utils/sanitize.js' import AreaDataSource from './AreaDataSource.js' import { changelogDataSource } from './ChangeLogDataSource.js' -import { resolveTransaction, useOrCreateTransaction, withTransaction } from '../utils/helpers.js' +import { muuidToString, resolveTransaction, withTransaction } from '../utils/helpers.js' import { AreaRelationsEmbeddings } from './AreaRelationsEmbeddings.js' import { getCountriesDefaultGradeContext, GradeContexts } from '../GradeUtils.js' @@ -425,19 +425,24 @@ export default class MutableAreaDataSource extends AreaDataSource { throw new Error('You cannot migrate, what appears to be, a country.') } + // Retrieve the current parent for this area. const oldParent = await this.areaModel.findOne({ _id: area.parent }).lean() .session(session) .orFail() - if (oldParent._id.equals(area.parent)) { - // The request is a no-op, waste no time. nothing has changed, so submit. + if (muuidToString(oldParent.metadata.area_id) === muuidToString(newParent)) { + // The request is a no-op, waste no time. nothing has changed + // we notify the user that this change has already been acknowledged with an error + // so that we are absolutely clear that nothing has changed. throw new UserInputError('no-op, the requested parent is ALREADY the parent.') } - const nextParent = await this.areaModel.findById(oldParent._id).orFail().session(session) + const nextParent = await this.areaModel.findOne({ 'metadata.area_id': newParent }).orFail().session(session) + // 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) @@ -451,7 +456,7 @@ export default class MutableAreaDataSource extends AreaDataSource { operation: OperationType.changeAreaParent, seq: 0 } satisfies ChangeRecordMetadataType, - updatedBy: user, + updatedBy: user }) const cursor = await area.save({ session }) diff --git a/src/model/__tests__/AreaRelationsEmbeddings.test.ts b/src/model/__tests__/AreaRelationsEmbeddings.test.ts index d9d4c76c..61a006a2 100644 --- a/src/model/__tests__/AreaRelationsEmbeddings.test.ts +++ b/src/model/__tests__/AreaRelationsEmbeddings.test.ts @@ -1,11 +1,30 @@ import { MUUID } from "uuid-mongodb" -import { AreaType } from "../../db/AreaTypes" +import { AreaType, DenormalizedAreaSummary } from "../../db/AreaTypes" import MutableAreaDataSource from "../MutableAreaDataSource" import muid from 'uuid-mongodb' import { getAreaModel, createIndexes } from "../../db" import inMemoryDB from "../../utils/inMemoryDB" import { muuidToString } from "../../utils/helpers" +export function embeddedRelationsReducer(path: AreaType[]) { + let trace: DenormalizedAreaSummary[] = [] + path.forEach((area, idx) => { + trace.push({ + uuid: area.metadata.area_id, + _id: area._id, + name: area.area_name + }) + + expect(area.embeddedRelations.ancestors).toMatchObject(trace) + + if (idx === 0) { + return + } + + area.parent?.equals(path[idx]._id) + }) +} + describe("updating of areas should propogate embeddedRelations", () => { let areas: MutableAreaDataSource let rootCountry: AreaType diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 16b689b8..67d60d60 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -5,6 +5,8 @@ import muid, { MUUID } from 'uuid-mongodb' import { AreaType, OperationType } from "../../db/AreaTypes" import { ChangeRecordMetadataType } from "../../db/ChangeLogType" import { UserInputError } from "apollo-server-core" +import { muuidToString, resolveTransaction, useOrCreateTransaction } from "../../utils/helpers" +import { embeddedRelationsReducer } from "./AreaRelationsEmbeddings.test" describe("Test area mutations", () => { let areas: MutableAreaDataSource @@ -196,11 +198,117 @@ describe("Test area mutations", () => { })) describe("cases for changing an areas parent",() => { - test.todo('Can update an areas parent reference') - test.todo('Updating an areas parents reference adds an area to its new parents children') - test.todo('Updating an areas parents reference REMOVED an area from its old parents children') - 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.todo('Updating an areas parent reference should update an areas child embeddedRelations') + test('Can update an areas parent reference', async () => addArea() + .then(parent => addArea(undefined, { parent })) + .then(async area => { + let otherArea = await addArea() + await areas.setAreaParent(testUser, area.metadata.area_id, otherArea.metadata.area_id) + expect(area.parent).toBeDefined() + expect(area.parent!.equals(otherArea._id)) + })) + + test('Updating an areas parents reference to the one already specified should throw', async () => addArea() + .then(async parent => [ await addArea(undefined, { parent }), parent]) + .then(async ([area, parent]) => { + expect(area.parent?.equals(parent._id)) + await expect( + () => areas + .setAreaParent(testUser, area.metadata.area_id, parent.metadata.area_id) + ) + .rejects + .toThrowError(UserInputError) + })) + + test('Updating an areas parents reference adds an area to its new parents children', async () => addArea(undefined) + .then(async area => { + let other = await addArea(undefined) + expect(other.embeddedRelations.children).toHaveLength(0) + await areas.setAreaParent(testUser, area.metadata.area_id, other.metadata.area_id) + other = await areas.areaModel.findById(other._id).orFail() + expect(other.embeddedRelations.children).toHaveLength(1) + expect(other.embeddedRelations.children.some(child => child.equals(area._id))) + })) + + test('test the unit of code that pulls children from the embedded array when there is no parent field to back it.', async () => addArea(undefined) + .then(async parent => { + let child = await addArea(undefined, { parent }) + let otherParent = await addArea(undefined) + + parent = await areas.areaModel.findById(child.parent).orFail() + + // We expect the parent to now have a child-reference to the area that points back to its parent + expect(child.parent?.equals(parent._id)) + expect(parent.embeddedRelations.children.some(child => child.equals(child._id))).toBeTruthy() + + // Manually change the parent reference + // This should produce no effects and as a result our + // await areas.areaModel.updateOne({ _id: child._id }, { parent: otherParent._id }) + child.parent = otherParent._id + + await useOrCreateTransaction(areas.areaModel, undefined, async (session) => { + await areas.relations.deleteStaleReferences(child, session) + }) + + parent = await areas.areaModel.findById(parent._id).orFail() + expect(parent.embeddedRelations.children.some(child => child.equals(child._id))).not.toBeTruthy() + })) + + test('Updating an areas parents reference REMOVED an area from its old parents children', async () => addArea(undefined) + .then(async area => { + await addArea(undefined) + let other = await addArea(undefined) + let original = await areas.areaModel.findById(area.parent).orFail() + + // We expect the original area to have a relation present to this node + expect(original.embeddedRelations.children.some(child => child.equals(area._id))).toBeTruthy() + + await areas.setAreaParent(testUser, area.metadata.area_id, other.metadata.area_id) + original = await areas.areaModel.findById(area.parent).orFail() + + // Now we expect that embedding to have updated + 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('Modifying an areas parent should update its child embeddedRelations', async () => { + let railLength = 7 + let rail: AreaType[] = [rootCountry] + + for (const idx in Array.from({ length: railLength }).map((_, idx) => idx)) { + rail.push(await addArea(undefined, { parent: rail[idx] })) + } + + expect(rail).toHaveLength(railLength + 1) + + const offset = 1 + let newParent = await addArea() + await areas.setAreaParent(testUser, rail[offset].metadata.area_id, newParent.metadata.area_id) + + for (const oldAreaData of rail.slice(1 + offset)) { + // 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!)) + + 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 + expect(pathElement._id.equals(oldAreaData.embeddedRelations.ancestors[offset]._id)).toEqual(false) + // This will validate that the element at [offset] has been set to our target newParent + expect(pathElement._id.equals(newParent._id)) + + // If the above expectations are met but these following ones are not, then the ID was correctly migrated but the + // 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') }) }) \ No newline at end of file From ffd43632839ed6484130055e9aa53bfd72cf320b Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Thu, 28 Nov 2024 15:30:26 +0200 Subject: [PATCH 26/27] Area parent migration implemented and tested --- src/model/AreaRelationsEmbeddings.ts | 53 ++++++++++++------ src/model/MutableAreaDataSource.ts | 23 ++++++-- .../__tests__/AreaRelationsEmbeddings.test.ts | 31 ++++++----- .../__tests__/MutableAreaDataSource.test.ts | 54 ++++++++++++++++--- 4 files changed, 121 insertions(+), 40 deletions(-) diff --git a/src/model/AreaRelationsEmbeddings.ts b/src/model/AreaRelationsEmbeddings.ts index 23f1800b..73d30a9b 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 ffcb805b..c55730aa 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 61a006a2..b9dd1e01 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 67d60d60..34a703a6 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 From 79511ee16a99be46506a6de7a1c7b54966818db4 Mon Sep 17 00:00:00 2001 From: CocoisBuggy Date: Thu, 28 Nov 2024 16:25:51 +0200 Subject: [PATCH 27/27] Expose functionality via the gql layer It's all in, I think. I added a new test to cover name uniqueness but no doubt more cases will come up. --- src/__tests__/areas.ts | 23 +++++++++++++++ src/graphql/area/AreaMutations.ts | 18 ++++++++++++ src/graphql/schema/AreaEdit.gql | 18 ++++++++++++ src/model/MutableAreaDataSource.ts | 3 ++ .../__tests__/MutableAreaDataSource.test.ts | 28 +++++++++++++------ 5 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/__tests__/areas.ts b/src/__tests__/areas.ts index e57b11f7..66d63617 100644 --- a/src/__tests__/areas.ts +++ b/src/__tests__/areas.ts @@ -168,5 +168,28 @@ describe('areas API', () => { expect(response.statusCode).toBe(200) }) + + it('should allow calling of the setAreaParent gql endpoint.', async () => { + const testArea = await areas.addArea(muuid.from(userUuid), 'A Rolling Stone', usa.metadata.area_id) + + const response = await queryAPI({ + query: ` + mutation SetAreaParent($area: ID!, $newParent: ID!) { + setAreaParent(area: $area, newParent: $newParent) { + areaName + area_name + } + } + `, + operationName: 'SetAreaParent', + userUuid, + app, + // Move it to canada + variables: { area: testArea.metadata.area_id, newParent: ca.metadata.area_id } + }) + + console.log(response.body) + expect(response.statusCode).toBe(200) + }) }) }) diff --git a/src/graphql/area/AreaMutations.ts b/src/graphql/area/AreaMutations.ts index b454fda4..f598cfe5 100644 --- a/src/graphql/area/AreaMutations.ts +++ b/src/graphql/area/AreaMutations.ts @@ -4,6 +4,7 @@ import { AreaType } from '../../db/AreaTypes.js' import { ContextWithAuth } from '../../types.js' import type MutableAreaDataSource from '../../model/MutableAreaDataSource.js' import { BulkImportInputType, BulkImportResultType } from '../../db/BulkImportTypes.js' +import { UserInputError } from 'apollo-server-core' const AreaMutations = { @@ -70,6 +71,23 @@ const AreaMutations = { ) }, + setAreaParent: async (_, { input }, { dataSources, user }: ContextWithAuth): Promise => { + const { areas } = dataSources + + if (user?.uuid == null) throw new UserInputError('Missing user uuid') + if (input?.area == null) throw new UserInputError('Missing area uuid') + if (input?.newParent == null) throw new UserInputError('Missing area new parent uuid') + + const areaUuid = muuid.from(input.uuid) + const newParentUuid = muuid.from(input.newParent) + + return await areas.setAreaParent( + user.uuid, + areaUuid, + newParentUuid + ) + }, + updateAreasSortingOrder: async (_, { input }, { dataSources, user }: ContextWithAuth): Promise => { const { areas } = dataSources diff --git a/src/graphql/schema/AreaEdit.gql b/src/graphql/schema/AreaEdit.gql index 53808577..9a3b17d7 100644 --- a/src/graphql/schema/AreaEdit.gql +++ b/src/graphql/schema/AreaEdit.gql @@ -4,6 +4,24 @@ type Mutation { """ addArea(input: AreaInput): Area + """ + Move an area from one parent to another. + + When areas are created, an initial parent must be assigned. this is to prevent a buildup of floating orphan + nodes that haven't been organized. Nevertheless, you may find that an area and its children may need to be + re-organized. This could happen for any number of reasons, but when it does need to happen you will need to + tap this mutation to update the areas parent. + + When you migrate an area, it will move (along with all the sub-areas inside it) to the targeted area. + The best way to conceptualize this is as a directory, so the usual rules will apply. If you could not + create the area as a child of the target (for example, because a name is already taken), then you should + not reasonably be able to move the area into this new parent. + + Caveats + - This mutation does not affect countries + """ + setAreaParent(area: ID!, newParent: ID!): Area + """ Update area attributes """ diff --git a/src/model/MutableAreaDataSource.ts b/src/model/MutableAreaDataSource.ts index c55730aa..de3a586d 100644 --- a/src/model/MutableAreaDataSource.ts +++ b/src/model/MutableAreaDataSource.ts @@ -454,6 +454,9 @@ export default class MutableAreaDataSource extends AreaDataSource { throw new AreaStructureError('CIRCULAR STRUCTURE: The requested parent is already a descendant, and so cannot also be a parent.') } + // the name of the area being moved into this area must be unique in its new context + await this.validateUniqueAreaName(area.area_name, nextParent) + // 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 diff --git a/src/model/__tests__/MutableAreaDataSource.test.ts b/src/model/__tests__/MutableAreaDataSource.test.ts index 34a703a6..7ef307a2 100644 --- a/src/model/__tests__/MutableAreaDataSource.test.ts +++ b/src/model/__tests__/MutableAreaDataSource.test.ts @@ -199,14 +199,26 @@ describe("Test area mutations", () => { })) describe("cases for changing an areas parent",() => { - test('Can update an areas parent reference', async () => addArea() - .then(parent => addArea(undefined, { parent })) - .then(async area => { - let otherArea = await addArea() - await areas.setAreaParent(testUser, area.metadata.area_id, otherArea.metadata.area_id) - expect(area.parent).toBeDefined() - expect(area.parent!.equals(otherArea._id)) - })) + test('Can update an areas parent reference', async () => addArea() + .then(parent => addArea(undefined, { parent })) + .then(async area => { + let otherArea = await addArea() + await areas.setAreaParent(testUser, area.metadata.area_id, otherArea.metadata.area_id) + expect(area.parent).toBeDefined() + expect(area.parent!.equals(otherArea._id)) + })) + + test('Updating an area will not produce duplicate named children in the target', async () => addArea() + .then(async parent => addArea(undefined, { parent })) + .then(async area => { + let otherArea = await addArea() + // put a duplicate name inside otherArea + addArea(area.area_name, { parent: otherArea }) + + await expect( + () => areas.setAreaParent(testUser, area.metadata.area_id, otherArea.metadata.area_id) + ).rejects.toThrowError(UserInputError) + })) test('Updating an areas parents reference to the one already specified should throw', async () => addArea() .then(async parent => [ await addArea(undefined, { parent }), parent])