OpenBeta / openbeta-graphql

The open source rock climbing API
https://openbeta.io
GNU Affero General Public License v3.0
41 stars 32 forks source link

Breadcrumbs not updated when users updating area name #213

Closed vnugent closed 1 year ago

vnugent commented 1 year ago

When an area is created, we also generate its bread crumbs data in pathTokens field. However, when we update an area's name, we don't update pathTokens.

openbeta-breadcrumbs-bug

To do:

CocoisBuggy commented 1 year ago

I can fix this but I just want to make absolutely sure we don't want to resolve this at query time rather than at mod time?

We chatted about reads being far more frequent than writes / modifications but I just want to make sure that the pathTokens system is still the way we want to go?

vnugent commented 1 year ago

I can fix this but I just want to make absolutely sure we don't want to resolve this at query time rather than at mod time?

We chatted about reads being far more frequent than writes / modifications but I just want to make sure that the pathTokens system is still the way we want to go?

Correct. We need to update pathToken, either iteratively or recursively, when doing updateArea(),

saferthanhouses commented 1 year ago

@vnugent Happy to look into this if it's available.

zichongkao commented 1 year ago

@saferthanhouses @CocoisBuggy I know it's been a while since we've updated this issue. Are either of you still open to taking it on?

andrew-jp commented 1 year ago

I'm new here/have no idea what I'm doing, but I've been working on this all day. What do you guys think of the general idea?

  updateArea()...
  ...
      const area = await this.areaModel.findOne(filter).session(session)
      if (area == null) {
        throw new Error('Area update error.  Reason: Area not found.')
      }

      const { areaName, description, shortCode, isDestination, isLeaf, isBoulder, lat, lng, experimentalAuthor } = document

  ...
      if (areaName != null) {
        const sanitizedName = sanitizeStrict(areaName)
        area.set({ area_name: sanitizedName })
        area.set({ pathTokens: area.pathTokens.slice(0, -1).push(sanitizedName) })

        if (area.children.length > 0) {
          await this.updatePathTokens(area.children, sanitizedName)
        }
      }

    ...
  async updatePathTokens (areaChildren: Types.ObjectId[], areaName: string, index: number = 2): Promise<void> {
    const session = await this.areaModel.startSession()

    for (const childId of areaChildren) {
      const filter = { _id: childId }

      const area = await this.areaModel.findOne(filter).session(session)
      if (area != null) {
        const newPath = area.pathTokens
        newPath[newPath.length - index] = areaName
        area.set({ pathTokens: newPath })

        if (area.children.length > 0) {
          await this.updatePathTokens(area.children, areaName, index + 1)
        }
        // save the changes
        await area.save()
      }
    }
  }

I really have no idea what I'm doing here, but I'm trying to figure it out. I would test it, but I believe I need API keys to edit anything even in a local env.

vnugent commented 1 year ago

@andrew-jp thanks for taking on this important issue. I think you're in the right direction.

Can you send me an email: viet at openbeta dot io ? I'll reply with API keys.

andrew-jp commented 1 year ago

My solution works! Just had to use a more JS/TS-like array copy. It looks good in testing so far, although recursion with queries/async operations involved is very expensive/slow. I'll see if I can improve the performance a bit.

Additionally, I'm trying to figure out if I need to use sessions in the "save" operation of the recursive function. Does the session protect/validate the Mongo transaction? I read some docs, but their purpose isn't totally clear to me. Any advice here would be helpful.

Central Wasatch - 20 (large) areas/3200 climbs - ~7.5s Little Cottonwood Canyon - 59 areas/1800 climbs - ~4s Both timed in real time - not the actual backend op.

Screen Shot 2023-06-12 at 12 39 07 PM Screen Shot 2023-06-12 at 12 39 22 PM
vnugent commented 1 year ago

@andrew-jp great to hear!

In general it's not going to be perfomant when making individual db calls for each child.

In async updatePathTokens() I think you might be able to eliminate the for loop and multiple find() calls by either:

  1. Calling mongoose populate() function to turn the array of area IDs into array of area objects.
    1. Using the $in operator to get all area objects in 1 call

Similar code https://github.com/OpenBeta/openbeta-graphql/blob/develop/src/db/utils/jobs/TreeUpdater.ts#L47