-
-
Notifications
You must be signed in to change notification settings - Fork 752
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
fix(mongodb): MongoDB Aggregation improvements #3366
Conversation
This is looking good. |
Some more updates.
See further comments in the |
Another note about counting and aggregation. We could finesse the pipleline with a |
@marshallswain I believe I am done with this PR. I am currently testing it in a project locally and everything seems to be working as expected! |
This PR is complete. @marshallswain and @daffl |
I'm letting @marshallswain have a look at this since he did a lot of that work before. Any idea what's going on with the tests? |
I am not sure what you mean about the tests. They seem to be working fine for me. I think it's a good idea to let Marshall look at this too. I do believe this is a major version bump for two reasons. The adapter previously used Some other added features and perf gains to consider
I am also terrible at TS. So there may be some tweaks that need to be made there. |
I meant that the MongoDB tests are not passing in CI: https://github.com/feathersjs/feathers/actions/runs/8850748298/job/24305731299?pr=3366#step:8:1588 Is there a way to make this more backwards compatible? Otherwise this'd have to wait for v6 - or we start moving things out again already which we were thinking of doing anyway. |
Oh that's odd. I had just been running only the adapter tests locally and they worked. I hadn't noticed them failing in the CI suite. I can poke around at that later. As far as backwards compatibility, there are three things I think. The Using For the Previously, using 1 - If a user does not use So 1 works differently but shouldn't break anything. And 4 works differently, but previously it didn't work as expected. So maybe it's not a breaking change? |
I finally got around to fixing the broken test and added a few more. I think this is ready for @marshallswain to look at. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready to ship. 🎉 Thank you!
Excellent, thank you for putting this together! |
This PR's aim is to improve/extend the usage of the aggregation framework to be used on all methods. But, it also solves a number of bugs and improves general performance.
Get Method
This method can now use the aggregation pipeline.
Find Method
We have accomplished better performance when not using the pipeline. This is mainly accomplished with how
filters.$limit === 0
now only counts the docs and doesn't do the actualmodel.find
because it's not needed.Create Method
This method can now use the aggregation pipeline.
Update Method
This method can now use the aggregation pipeline. It also solves #3365 and #3363. This is accomplished by using
findOneAndReplace
instead ofreplaceOne
. BecausefindOneAndReplace
actually returns the updated document, there is no need for the two other lookups.Patch Method
This method can now use the aggregation pipeline. It should also get a performance boost with the usage of
findOneAndUpdate
when possible. It now supports$sort
,$limit
, and$skip
for multi. I am not sure why these operators were not supported before, but it seems like a valuable query to have access to.Remove Method
This method can now use the aggregation pipeline. It should also get a performance boost with the usage of
findOneAndDelete
when possible. It now supports$sort
,$limit
, and$skip
for multi. I am not sure why these operators were not supported before, but it seems like a valuable query to have access to.