Sort area children by leftRightIndex #296
Conversation
src/utils/helpers.ts
Outdated
| @@ -21,3 +23,17 @@ export function exhaustiveCheck (_value: never): never { | |||
|
|
|||
| export const geojsonPointToLongitude = (point: Point): number => point.coordinates[0] | |||
| export const geojsonPointToLatitude = (point: Point): number => point.coordinates[1] | |||
|
|
|||
| export function compareAreaLeftRightIndex (a: AreaType, b: AreaType): number { | |||
There was a problem hiding this comment.
I considered merging these two, but then I'd need to be able to narrow the type of a and b. And that's not very easy since these are custom types, not just strings or numbers. And so it gets a bit messy. I figure just splitting them out is a bit repetitive but a lot more readable.
zichongkao
force-pushed
the
kao-leftrightsort
branch
2 times, most recently
from
3fbd76e
to
6320b57
Compare
zichongkao
force-pushed
the
kao-leftrightsort
branch
from
6320b57
to
127faf5
Compare
src/graphql/resolvers.ts
Outdated
| @@ -202,7 +202,7 @@ const resolvers = { | |||
|
|
|||
| children: async (parent: AreaType, _, { dataSources: { areas } }: Context) => { | |||
| if (parent.children.length > 0) { | |||
| return await areas.findManyByIds(parent.children) | |||
| return (await areas.findManyByIds(parent.children)).sort(compareAreaLeftRightIndex) | |||
There was a problem hiding this comment.
What do you think if we let Mongo handle the sorting here and in findManyClimbsByUuids(). This way we don't have to write comparators.
Something like:
// inside AreaDataSource
...
return await this.areaModel.find().
where('metadata.leftRightIndex').
in(parent.children).
sort({ 'metadata.leftRightIndex': 1 }).lean()edit: add lean()
|
Yah. That's way better. Db should handle it indeed. I can look at it later
today!
…On Sun, May 21, 2023, 10:19 AM Viet Nguyen ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/graphql/resolvers.ts
<#296 (comment)>
:
> @@ -202,7 +202,7 @@ const resolvers = {
children: async (parent: AreaType, _, { dataSources: { areas } }: Context) => {
if (parent.children.length > 0) {
- return await areas.findManyByIds(parent.children)
+ return (await areas.findManyByIds(parent.children)).sort(compareAreaLeftRightIndex)
What do you think if we let Mongo handle the sorting here and in
findManyClimbsByUuids(). This way we don't have to write comparators.
Something like:
// inside AreaDataSource
...
return await this.areaModel.find().
where('metadata.leftRightIndex').
in(parent.children).
sort({ 'metadata.leftRightIndex': 1 })
—
Reply to this email directly, view it on GitHub
<#296 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3ZADFVPZI7LMDTJ2P2AFDXHI57PANCNFSM6AAAAAAYJF3TQU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| }, | ||
| { | ||
| $set: { | ||
| climbs: { $sortArray: { input: '$climbs', sortBy: { 'metadata.left_right_index': 1 } } }, |
There was a problem hiding this comment.
$sortArray requires Mongo 5.2 and up. Otherwise we need something like this: https://stackoverflow.com/questions/15388127/mongodb-sort-inner-array
| { | ||
| $set: { | ||
| climbs: { $sortArray: { input: '$climbs', sortBy: { 'metadata.left_right_index': 1 } } }, | ||
| children: { $sortArray: { input: '$children', sortBy: { 'metadata.leftRightIndex': 1 } } } |
There was a problem hiding this comment.
can you confirm children[] contains actual area objects? I think it only contains a list of area _id's. When we ask for an area.children[] in GQL, we're making another DB query here:
https://github.com/OpenBeta/openbeta-graphql/blob/develop/src/graphql/resolvers.ts#L203
There was a problem hiding this comment.
Oh yeah you're right! I've not tested this far because it throws a "No such operation as $sortArray" error.
There was a problem hiding this comment.
What if we did a $lookup for children above this $set so that we have the full object?
There was a problem hiding this comment.
What if we did a $lookup for children above this $set so that we have the full object?
I thought the query would get too wildly complicated for a small performance gain, whereas the GQL field resolution way in my comment above is more elegant and simpler (but requires an additional query).
zichongkao
force-pushed
the
kao-leftrightsort
branch
from
0d8503c
to
38515ff
Compare
|
I'd be content with whatever solution you come with that doesn't require a DB upgrade, or we can leave this PR open until we do the upgrade. |
|
Agree that we can hold this back until we have time to upgrade Mongo to v6 as tracked here: #300 |
"Area" graphql objects have a children field containing other areas or climbs. Currently these are unsorted -- we leave the frontend to sort them. But the frontend tends to sort them the same way based on their
leftRightIndexso why not just return that as a default order returned by the backend? This way frontends won't have to do any sorting.In response to issue: #294 and first discussed in this PR: OpenBeta/open-tacos#696 (comment)