Skip to content

Refactor mutable area datasource to prevent param explosion #475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/__tests__/areas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ describe('areas API', () => {
areas = MutableAreaDataSource.getInstance()
organizations = MutableOrganizationDataSource.getInstance()
usa = await areas.addCountry('usa')
ca = await areas.addArea(user, 'CA', usa.metadata.area_id)
wa = await areas.addArea(user, 'WA', usa.metadata.area_id)
ca = await areas.addArea(user, { areaName: 'CA', parentUuid: usa.metadata.area_id })
wa = await areas.addArea(user, { areaName: 'WA', parentUuid: usa.metadata.area_id })
})

afterAll(async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/bulkImport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe('bulkImportAreas', () => {
beforeEach(async () => {
await inMemoryDB.clear()
await bulkImport.addCountry('usa')
testArea = await bulkImport.addArea(user, "Test Area", null, "us")
testArea = await bulkImport.addArea(user, { areaName: "Test Area", countryCode: "us" })
})

afterAll(async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ describe('history API', () => {
it('queries recent change history successfully', async () => {
// Make changes to be tracked.
usa = await areas.addCountry('usa')
ca = await areas.addArea(user, 'CA', usa.metadata.area_id)
ca = await areas.addArea(user, { areaName: 'CA', parentUuid: usa.metadata.area_id })
const alphaFields = {
displayName: 'Alpha OpenBeta Club',
associatedAreaIds: [usa.metadata.area_id],
Expand Down
4 changes: 2 additions & 2 deletions src/__tests__/organizations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ describe('organizations API', () => {
areas = MutableAreaDataSource.getInstance()
organizations = MutableOrganizationDataSource.getInstance()
usa = await areas.addCountry('usa')
ca = await areas.addArea(user, 'CA', usa.metadata.area_id)
wa = await areas.addArea(user, 'WA', usa.metadata.area_id)
ca = await areas.addArea(user, { areaName: 'CA', parentUuid: usa.metadata.area_id })
wa = await areas.addArea(user, { areaName: 'WA', parentUuid: usa.metadata.area_id })
})

afterAll(async () => {
Expand Down
4 changes: 2 additions & 2 deletions src/__tests__/ticks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ describe('ticks API', () => {

// Add climbs because add/update tick requires type validation
await areas.addCountry('usa')
const newDestination = await areas.addArea(user, 'California', null, 'usa')
const routesArea = await areas.addArea(user, 'Sport & Trad', newDestination.metadata.area_id)
const newDestination = await areas.addArea(user, { areaName: 'California', countryCode: 'usa' })
const routesArea = await areas.addArea(user, { areaName: 'Sport & Trad', parentUuid: newDestination.metadata.area_id })

const newIDs = await climbs.addOrUpdateClimbs(user, routesArea.metadata.area_id, newClimbsToAdd)
// Update tick inputs with generated climb IDs
Expand Down
15 changes: 9 additions & 6 deletions src/graphql/area/AreaMutations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,15 @@ const AreaMutations = {
if (user?.uuid == null) throw new Error('Missing user uuid')

return await areas.addArea(
user.uuid, name,
parentUuid == null ? null : muuid.from(parentUuid),
countryCode,
experimentalAuthor,
isLeaf,
isBoulder
user.uuid,
{
areaName: name,
countryCode,
experimentalAuthor,
isLeaf,
isBoulder,
parentUuid: parentUuid == null ? undefined : muuid.from(parentUuid)
}
)
},

Expand Down
2 changes: 2 additions & 0 deletions src/graphql/schema/AreaEdit.gql
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ input AreaInput {
isLeaf: Boolean
isBoulder: Boolean
experimentalAuthor: ExperimentalAuthorType
lat: Float
lng: Float
}

"""
Expand Down
13 changes: 7 additions & 6 deletions src/model/BulkImportDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,14 @@ export default class BulkImportDataSource extends MutableAreaDataSource {
throw new Error(`area with id ${areaNode.uuid.toUUID().toString()} (${areaNode.areaName ?? 'unknown name'}) not found`)
}
} else if (areaNode.areaName != null) {
area = await this.addAreaWith({
user,
areaName: areaNode.areaName,
countryCode: areaNode.countryCode,
parentUuid,
area = await this.addAreaWith(user,
{
areaName: areaNode.areaName,
countryCode: areaNode.countryCode,
parentUuid
},
session
}).then(async (area) => {
).then(async (area) => {
return await this.updateArea(user, area.metadata.area_id, {
description: areaNode.description,
leftRightIndex: areaNode.leftRightIndex,
Expand Down
62 changes: 31 additions & 31 deletions src/model/MutableAreaDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ import { getAreaModel } from '../db/AreaSchema.js'

isoCountries.registerLocale(enJson)

export interface AddAreaOptions {
user: MUUID
interface AddAreaOptions {
areaName: string
parentUuid?: MUUID | null
parentUuid?: MUUID
countryCode?: string
experimentalAuthor?: ExperimentalAuthorType
isLeaf?: boolean
isBoulder?: boolean
session?: ClientSession
lat?: number
lng?: number
}

export interface UpdateAreaOptions {
Expand Down Expand Up @@ -158,17 +158,10 @@ export default class MutableAreaDataSource extends AreaDataSource {
throw new Error('Error inserting ' + countryCode)
}

async addAreaWith ({
user,
areaName,
parentUuid = null,
countryCode,
experimentalAuthor,
isLeaf,
isBoulder,
session
}: AddAreaOptions): Promise<AreaType> {
return await this.addArea(user, areaName, parentUuid, countryCode, experimentalAuthor, isLeaf, isBoulder, session)
async addAreaWith (user: MUUID, props: AddAreaOptions, session?: ClientSession): Promise<AreaType> {
return await this.addArea(
user, props, session
)
}

/**
Expand All @@ -178,14 +171,13 @@ export default class MutableAreaDataSource extends AreaDataSource {
* @param parentUuid
* @param countryCode
*/
async addArea (user: MUUID,
areaName: string,
parentUuid: MUUID | null,
countryCode?: string,
experimentalAuthor?: ExperimentalAuthorType,
isLeaf?: boolean,
isBoulder?: boolean,
sessionCtx?: ClientSession): Promise<AreaType> {
async addArea (
user: MUUID,
props: AddAreaOptions,
sessionCtx?: ClientSession
): Promise<AreaType> {
const { parentUuid, countryCode, areaName } = props

if (parentUuid == null && countryCode == null) {
throw new Error(`Adding area "${areaName}" failed. Must provide parent Id or country code`)
}
Expand All @@ -199,12 +191,15 @@ export default class MutableAreaDataSource extends AreaDataSource {
throw new Error(`Adding area "${areaName}" failed. Unable to determine parent id or country code`)
}

// We can update the prop in place and pass it on to the rest of the logic
props.parentUuid = parentId

const session = sessionCtx ?? await this.areaModel.startSession()
try {
if (session.inTransaction()) {
return await this._addArea(session, user, areaName, parentId, experimentalAuthor, isLeaf, isBoulder)
return await this._addArea(session, user, props)
} else {
return await withTransaction(session, async () => await this._addArea(session, user, areaName, parentId, experimentalAuthor, isLeaf, isBoulder))
return await withTransaction(session, async () => await this._addArea(session, user, props))
}
} finally {
if (sessionCtx == null) {
Expand All @@ -213,13 +208,18 @@ export default class MutableAreaDataSource extends AreaDataSource {
}
}

async _addArea (session, user: MUUID, areaName: string, parentUuid: MUUID, experimentalAuthor?: ExperimentalAuthorType, isLeaf?: boolean, isBoulder?: boolean): Promise<any> {
async _addArea (session, user: MUUID, props: AddAreaOptions): Promise<any> {
const { areaName, parentUuid, isLeaf, isBoulder, experimentalAuthor } = props
const parentFilter = { 'metadata.area_id': parentUuid }
const parent = await this.areaModel.findOne(parentFilter).session(session).orFail(new GraphQLError(`[${areaName}]: Expecting country or area parent, found none with id ${parentUuid.toString()}`, {
extensions: {
code: ApolloServerErrorCode.BAD_USER_INPUT
}
}))
const parent = await this
.areaModel
.findOne(parentFilter)
.session(session)
.orFail(new GraphQLError(`[${areaName}]: Expecting country or area parent, found none with id ${(parentUuid ?? 0).toString()}`, {
extensions: {
code: ApolloServerErrorCode.BAD_USER_INPUT
}
}))

if (parent.metadata.leaf || (parent.metadata?.isBoulder ?? false)) {
if (parent.children.length > 0 || parent.climbs.length > 0) {
Expand Down
12 changes: 6 additions & 6 deletions src/model/__tests__/AreaHistoryDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ describe('Area history', () => {
const newArea = await areas.findOneAreaByUUID(usa.metadata.area_id)
expect(newArea.area_name).toEqual(usa.area_name)

const or = await areas.addArea(testUser, 'oregon', usa.metadata.area_id)
const nv = await areas.addArea(testUser, 'nevada', usa.metadata.area_id)
const or = await areas.addArea(testUser, { areaName: 'oregon', parentUuid: usa.metadata.area_id })
const nv = await areas.addArea(testUser, { areaName: 'nevada', parentUuid: usa.metadata.area_id })

expect(nv?._id).toBeTruthy()
expect(or?._id).toBeTruthy()
Expand Down Expand Up @@ -94,7 +94,7 @@ describe('Area history', () => {

it('should record multiple Areas.setDestination() calls ', async () => {
const canada = await areas.addCountry('can')
const squamish = await areas.addArea(testUser, 'squamish', canada.metadata.area_id)
const squamish = await areas.addArea(testUser, { areaName: 'squamish', parentUuid: canada.metadata.area_id })

expect(squamish?._id).toBeTruthy()

Expand All @@ -121,7 +121,7 @@ describe('Area history', () => {

it('should record an Areas.deleteArea() call', async () => {
const greece = await areas.addCountry('grc')
const leonidio = await areas.addArea(testUser, 'Leonidio', greece.metadata.area_id)
const leonidio = await areas.addArea(testUser, { areaName: 'Leonidio', parentUuid: greece.metadata.area_id })

if (leonidio == null) fail()

Expand All @@ -139,11 +139,11 @@ describe('Area history', () => {

it('should not record a failed Areas.deleteArea() call', async () => {
const spain = await areas.addCountry('esp')
const margalef = await areas.addArea(testUser, 'margalef', spain.metadata.area_id)
const margalef = await areas.addArea(testUser, { areaName: 'margalef', parentUuid: spain.metadata.area_id })

if (margalef == null) fail()

const newChild = await areas.addArea(testUser, 'One', margalef.metadata.area_id)
const newChild = await areas.addArea(testUser, { areaName: 'One', parentUuid: margalef.metadata.area_id })

if (newChild == null) fail()

Expand Down
6 changes: 3 additions & 3 deletions src/model/__tests__/MediaDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ describe('MediaDataSource', () => {
await createIndexes()

await areas.addCountry('USA')
areaForTagging1 = await areas.addArea(muuid.v4(), 'Yosemite NP', null, 'USA')
areaForTagging2 = await areas.addArea(muuid.v4(), 'Lake Tahoe', null, 'USA')
areaForTagging3 = await areas.addArea(muuid.v4(), 'Shelf Road', null, 'USA')
areaForTagging1 = await areas.addArea(muuid.v4(), { areaName: 'Yosemite NP', countryCode: 'USA' })
areaForTagging2 = await areas.addArea(muuid.v4(), { areaName: 'Lake Tahoe', countryCode: 'USA' })
areaForTagging3 = await areas.addArea(muuid.v4(), { areaName: 'Shelf Road', countryCode: 'USA' })
if (areaForTagging1 == null || areaForTagging2 == null || areaForTagging3 == null) fail('Fail to pre-seed test areas')

const rs = await climbs.addOrUpdateClimbs(muuid.v4(), areaForTagging1.metadata.area_id, [newSportClimb1])
Expand Down
22 changes: 12 additions & 10 deletions src/model/__tests__/MutableAreaDataSource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ describe("Test area mutations", () => {

return areas.addArea(
testUser,
name,
parent ?? rootCountry.metadata.area_id,
undefined,
undefined,
extra?.leaf,
extra?.boulder
{
areaName: name,
parentUuid: parent ?? rootCountry.metadata.area_id,
isLeaf: extra?.leaf,
isBoulder: extra?.boulder,
}
)
}

Expand All @@ -57,7 +57,7 @@ describe("Test area mutations", () => {

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)
.addArea(testUser, { areaName: 'Texas2', parentUuid: rootCountry.metadata.area_id })
.then(area => {
expect(area?._change).toMatchObject({
user: testUser,
Expand All @@ -66,10 +66,12 @@ describe("Test area mutations", () => {
}))

test("Add an area with an unknown UUID parent should fail",
async () => await expect(() => areas.addArea(testUser, 'Texas', muid.v4())).rejects.toThrow())
async () => await expect(() => areas.addArea(testUser, { areaName: 'Texas', parentUuid: muid.v4() })).rejects.toThrow())

test("Add a simple area with no specifications using a country code", () => areas.addArea(testUser, 'Texas part 2', null, 'USA')
.then(texas => areas.addArea(testUser, 'Texas Child', texas.metadata.area_id)))
test("Add a simple area with no specifications using a country code", () =>
areas.addArea(testUser, { areaName: 'Texas part 2', countryCode: 'USA' })
.then(texas => areas.addArea(testUser, { areaName: 'Texas Child', parentUuid: texas.metadata.area_id }))
)

test("Add a simple area, then specify a new child one level deep", () => addArea('California')
.then(async parent => {
Expand Down
Loading
Loading