Skip to content
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

[Blocked] Reverse link nested objects failure #387

Closed
wants to merge 2 commits into from

Conversation

theodorDiaconu
Copy link
Contributor

@theodorDiaconu theodorDiaconu commented Oct 15, 2019

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
Copy link
Contributor

bhunjadi commented Nov 4, 2019

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
Copy link
Contributor Author

@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
Copy link
Contributor

bhunjadi commented Nov 5, 2019

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
Copy link
Contributor Author

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

@bhunjadi
Copy link
Contributor

bhunjadi commented Nov 7, 2019

It could be closed in favor of #400

@StorytellerCZ
Copy link
Collaborator

Closing in favor of #400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants