cult-of-coders / grapher

Grapher: Meteor Collection Joins + Reactive GraphQL like queries
https://atmospherejs.com/cultofcoders/grapher
MIT License
275 stars 53 forks source link

[Blocked] Reverse link nested objects failure #387

Closed theodorDiaconu closed 12 months ago

theodorDiaconu commented 5 years ago

There is a failing test here, and it's related to retrieving nested objects from the reverse side. As you can see in tests.

The problem basically is this, when we perform the aggregation:

{
    $group: {
        "_id": "$doctorId",
        "data": {
            $push: {
                "_id": "$_id",
                "ratings___rating": "$ratings.rating",
                "ratings___dimension": "$ratings.dimension"
            }
        }
    }
}

Which is ok, but when we get the response its:

{
    "_id" : "X6sTcQFCQD87fCx4t",
    "data" : [ 
        {
            "_id" : "8hFEtceELWQZ82cqe",
            "ratings___rating" : [ 
                4, 
                1, 
                3, 
                4, 
                4
            ],
            "ratings___dimension" : [ 
                "Dimension-1", 
                "Dimension-2", 
                "Dimension-3", 
                "Dimension-4", 
                "Dimension-5"
            ]
        }
    ]
}

Even if ratings looks like:

[
   { rating, dimension },
   ....
]

Possible solutions

  1. Maybe there is a way to distinguish at db level these type of fields?
  2. Allow a hook or a way to specify these types of fields at grapher level?
bhunjadi commented 4 years ago

I would say that we have a problem in aggregation pipeline. I think this would be better:

[
     {
         "$match": {
             "bId": {
                 "$in": [
                     "xbghvEDmn79A9ZS4f"
                 ]
             }
         }
     },
     {
         "$project": {
             "ratings.rating": 1,
             "ratings.dimension": 1,
             "bId": 1
         }
     },
     {
         "$group": {
             "_id": "$bId",
             "data": {
                 "$push": "$$ROOT"
             }
         }
}
]

In that way grapher is not required to know details of the database schema and we basically have identical approach in both find and aggregate - via projection. Which would probably be something what you meant under 1) - DB is doing all the work regarding fields.

Additional benefit - we do not require dotted field replacement anymore. Regarding performance, I think we are good here since we are doing $project before $group.

I tried to fix this and it looks like it is not complicated, you can see this commit.

All tests are passing with that changes (of course, including the failing one you wrote).

@theodorDiaconu What do you think?

theodorDiaconu commented 4 years ago

@bhunjadi look here https://github.com/kaviarjs/nova/tree/master/src/core/query/hypernova how I approached it in the npm version. I would have merged it completely. But meta links have been an extreme pain to implement, and do nicely.

bhunjadi commented 4 years ago

Here is another attempt - using find instead of aggregate. Works with meta links.

Caveat: slicing (limit, skip) cannot be done on db server since results are not grouped. But this applies to all links, not just meta.

This approach is similar to npm version, but handles meta links and supports slicing

theodorDiaconu commented 4 years ago

Looks good, let's bring it here in a PR

bhunjadi commented 4 years ago

It could be closed in favor of #400

StorytellerCZ commented 12 months ago

Closing in favor of #400