-
Notifications
You must be signed in to change notification settings - Fork 265
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
Replacing the operator $where with aggregation pipeline #4148
Comments
Thank you very much for your feedback! We started to use $where operation in a far away past (when MongoDB 2.4/2.6 was the newest MongoDB version :) and it was not possible to solve the problem with the aggregation pipeline stages at that moment. Now the proper aggregation stages exists for that, so I think is a good idea what you propose. Please, go ahead with the pull request fixing this. I can provide some degree of support (best effort mode) if you need it. By the way, your issue report is pretty comprehensive. However, it would be great if you could cite (from MongoDB official documentation) the place in which they say $where operator is deprecated. |
Here are some of the links which mention that $where is either deprecated, not usable or poses security threat:
EXAMPLE $where and map-reduce are not supported.
In contrast, aggregation pipeline is supported in all instances, has no security issues in using it and also is not deprecated. Hope this answers your query. |
Hi @fgalan , Can you please provide the sample document /document structure and the query that is being executed to fetch the document as per fiware-orion/src/lib/mongoBackend/MongoCommonUpdate.cpp |
Hello @fgalan Reaching out as we (@nsrivastava9 and I) need some high level guidance for making this change. What we want to do is replace $where with aggregation pipeline [https://www.mongodb.com/docs/manual/core/aggregation-pipeline/] So effectively we would like to make $where an alias for aggregation pipeline, meaning whenever the user calls @where we end up calling command aggregate with appropriate arguments. In code we are looking into MongoCommonUpdate.cpp [https://github.com/telefonicaid/fiware-orion/blob/e1b079c8843a759b3d53d458eb2034dc8a9793e3/src/lib/mongoBackend/MongoCommonUpdate.cpp#L1257] But could you give some guidance in terms what changes in the code need to be made for this? |
For Orion logs (
After a JSON pretty print ;) looks this way:
|
Especifically regarding
|
Not sure of fully understand what you mean by alias... in general in MongoDB, you cannot "blindly" replace usage of "$where in find()" by "$where in aggregation pipeline" as they are different operations (i.e. find() is a operation to query a database collection while aggregate() is a wrapper for a more complex command in MongoDB). Having said that, note that "$where" is currently used in only one place in the Orion code: Thus, under my understanding, I think it is just a matter of replace The structured of the current query has been posted in a previous comment. I hope it helps in the re-implementation of |
Hi @fgalan Thanks for your detailed message. It helps clarify how we can re-implement addTriggeredSubscriptions_noCache(). I have a couple of follow up questions. The way I understand is this that in its current implementation, it builds Instead now we will have to build a pipeline just like it is built in mongoQueryTypes.cpp and then call My follow up question is that after using
Also, how will the structure of the rest of the query (or json) will change? I was looking for a couple of examples on how to construct the pipeline object (which can be passed as an argument to |
I think your understanding is correct.
Not sure what you are asking here... :) Maybe you are asking for and explanation on what the function() in the $where clause does? The purpose of that function() is to to a "reverse regex" operation. That it, in the query we have the entity (id "Room1" and type "Room" in the example above) and we want to retrieve the documents in the csubs collection which match it. This way is more readable:
It works in the following way.
More on this approach to do "reverse regex" in this post at SOF
If you look for orion::collectionAggregate() in the code base, we can find 3 examples. The way in which the pipeline is build could be a bit "obscure" (due to the "nested append" approach) but you will find description comments with the pipeline structure. In particular:
|
Having a look to the cited post at SOF maybe a simpler solution to aggregation pipeline is possible :) In particular, to follow this advice and use This example is closer to what we need: https://mongoplayground.net/p/ibWLonDkxFT |
Trying to get even more closer, maybe there are some problems... see https://stackoverflow.com/questions/73317911/expr-with-regexmatch-doesnt-work-when-the-pattern-is-inside-an-array |
A solution to the problem was provided (https://stackoverflow.com/a/73318486/1485926):
|
Hi @fgalan Thanks for these pointers. So now what we'd be doing is replacing the current "reverse-regex" implementation which uses $where operator to $expr as you pointed out in your last comment. In your earlier comment (#4148 (comment)) you mentioned the compiled query looks like this (after json pretty print)
Notice that I have replaced all the 3 occurrences of $where with $expr. The value of $expr is something we are working on currently so we will plug in those values when we are ready. So, wanted to check is this JSON structure correct? If yes, then follow up question is how do we replace the line (
We are not doing appendCode instead only an expression so should we do something like this:
where reverse_regex_expression_json_object is correct implementation which has structure like this:
Just wanted to make sure my current understanding is correct, so let me know your comments. |
The structure seems to be correct in the sense it replaces $where by $expr. I have noticed that To see if your implementation is correct, create a pull request with the change. We have a lot of regression tests for subscriptions in "noCache" way (when this functionality is used) so if your pull requests pass the tests that would be ok.
It makes sense to change |
Hi @fgalan We have written the expression for the 4 conditions in the current code and the new expression is in the playground: Playground Link. I am pasting it here as well:
There is still one condition not correct: it requires a check that isTypePattern field is not present. I couldn't find an operator to perform this check so I am currently checking if it is null, which of course is not the same. I wanted to get feedback before I commit it and raise a PR. So kindly let me know. |
Maybe the |
Hi @fgalan , I played around with $exists operator, it works when it is part of boolean expression but when I embed it within "in" of $map then the compiler throws an error. Here is the link playground So I'm not sure how to use $exists within $map. If you can suggest me how to go about doing it, that would be great. |
I'd suggest to ask at StackOverflow using the "mongodb" and "mondob-query" labels, in the case it helps. |
Hey @fgalan So someone responded at the Stackoverflow. I will create a PR with this solution. If you have comments let me know. |
Great! I'll have a look to the PR and provide comments on it. Thanks! |
Hi @fgalan I created a pull request. I have made the change only for function fill_idPtypeNP. Once it is approved I will make the similar changes for the other three functions:
Looking forward to your comments. Thanks! |
I see there are three failing tests:
Can you give me some guidance on how to fix? Also, is there a way I can run these tests locally and know the results? |
With regars to style issue (https://github.com/telefonicaid/fiware-orion/actions/runs/3052031682/jobs/4921123995) I think this is the cause:
With regards to functional test failing it's a bit weird... I could have expected that "no cache" test were failing (due to some problem with the new query) but the fail in "cache" tests is weird, as they are not using that part of the code at all. The procedure to run tests locally is described in this section: https://fiware-orion.readthedocs.io/en/master/admin/build_source.html#testing-and-coverage (the I'll try to have a look myself but it could take some time, so maybe you prefer to do progress on your own... |
This is the makefile: https://github.com/telefonicaid/fiware-orion/blob/master/makefile |
Maybe it is useful to have a look to CI Docker at https://github.com/telefonicaid/fiware-orion/tree/master/ci. This Docker container is built to run unit and functional test, so if you mimics the same steps in your environment you will end with a functional environment to run Orion tests. |
This will be part of version 3.8.0, to be released soon. There is only one issue pending to freeze the release (this one: #4085) but it is a complex functionality, so it could take some time. In the meanwhile, the |
PR #4226 with additional code comments and documentation fixes. |
Is your feature request related to a problem / use case? Please describe.
I'm part of Azure Cosmos DB team and we are working to make our Mongo API work with Fiware-Orion without any issues. One of these issues is: the use of Mongo DB operator $where [1] in the codebase [2]. Since $where has been deprecated by Mongo DB and would not be supported in future releases (it is presently supported though), we recommend to replace $where with the aggregation pipeline [3].
fiware-orion/src/lib/mongoBackend/MongoCommonUpdate.cpp
Line 1257 in e1b079c
Describe the solution you'd like
We recommend moving away from $where operator and use aggregation pipeline instead. The codebase at [2] to be replaced with using aggregation pipeline.
Describe alternatives you've considered
$where is a deprecated feature in MongoDB so considering an alternate is going to be a lot more work as we will supporting a deprecated feature in the future. So we did not consider any other alternative.
Describe why you need this feature
Additional information
Add any other information, diagrams or screenshots about the feature request here.
Do you have the intention to implement the solution
The text was updated successfully, but these errors were encountered: