Fix issue on count with Geo constraints and mongo (issue #5285) #5286
Conversation
|
Thanks for reporting the issue this way! This is very helpful. I believe this is related to the way mongodb’s driver behaves since the 3.0 update. I recall it changed the default count behaviors. Are you willing to have a look for the fix? Most likely, this is located in the storage adapter implementation. |
|
Caused by #5025 There is a guide in the following link: Migrating count to countDocuments |
|
I dug into the code, and the differences between 3.0 (the last working version) and 3.1. I wasn't able to spot the precise cause of the problem (disclaimer: I’m really new to backend / JS / Mongo dev :/ ). I saw the Mongo Adapter refactor. It might be related to this, but I'm unable to say how. |
|
@jlnquere In my previous comment I posted the cause and a potential solution. The query must be transformed before passing it into |
|
@dplewis Thanks for the pointer. I read again your post and dug into the code. I don't see how I can replace Is it something that should be done in |
… $geoWithin) All credit goes to @dplewis !
Codecov Report
@@ Coverage Diff @@
## master #5286 +/- ##
==========================================
+ Coverage 93.95% 94.48% +0.52%
==========================================
Files 123 123
Lines 8970 10485 +1515
==========================================
+ Hits 8428 9907 +1479
- Misses 542 578 +36
Continue to review full report at Codecov.
|
|
I just pushed a fix based on a @dplewis idea. Simply rewriting the query to replace for (const key in query) {
if (query[key].$nearSphere) {
const geoQuery = {
$geoWithin: {
$centerSphere: [query[key].$nearSphere, query[key].$maxDistance],
},
};
query[key] = geoQuery;
}
}All credit goes to @dplewis. Thanks for the idea 👍 |
|
@jlnquere Actually I think its best to keep the transforms inside of Edit: You also have to add transform for |
jlnquere
changed the title
|
@dplewis I don't see how we can do that in |
A little debugging here will answer all your questions.
Add an extra parameter to transformWhere like isCount. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@acinader Mongo driver update that forces us to use the non-sort geo query when we use count. Since we aren't sorting the query this results in faster requests. Moving the logic to the |
| collections.map( | ||
| collection => (fast ? collection.deleteMany({}) : collection.drop()) | ||
| collections.map(collection => | ||
| fast ? collection.deleteMany({}) : collection.drop() |
There was a problem hiding this comment.
is this change covered in a test?
There was a problem hiding this comment.
prettier making the code look good
…ity#5285) (parse-community#5286) * Add a tests that fails due to issue parse-community#5285 * Make test code much simpler * Fix parse-community#5285 by rewriting query (replacing $nearSphere by $geoWithin) All credit goes to @dplewis ! * move logic to transform
Here is a unit test that demonstrates count issues with Geo constraints ( cf #5285 ).
This test actually fails.