Allow disabling workaround for since-fixed MongoDB bug

[NotBobTheBuilder]

Hi!

Sorry in advance for the wall of text in this PR, but I have tried to bring all the relevant context of the problem into one place along with an explanation of my proposed solution.

This is a follow-up PR to #3476 which I filed a couple of years ago.

The original PR was related to this MongoDB bug, SERVER-13732, which at the time was a longstanding bug in index selection in MongoDB.

The bug meant that any queries using both an $or filter and another query predicate at the top level of the query would suffer from needlessly poor index selectivity, resulting in more load on the server per query than needed.

The workaround implemented inside Parse is to have the DatabaseController change affected queries with this structure

{ a: 1, $or: [{b: 1}, {c: 1}]}

To have this structure, which is logically equivalent:

{ $or: [{a: 1, b: 1}, {a: 1, c: 1}]}

The good news is that the bug was fixed in 3.6, meaning the workaround code I filed a PR for a couple of years ago is now redundant for anyone using Parse with MongoDB 3.6 or above.

The bad news is that, in version 3.6, some queries involving $ors and sorts are now less performant with the workaround applied than they would be without it.

I noticed from the README.md file that tests are still running against 3.2 and 3.4, so I didn't want to remove the workaround completely. Those versions of MongoDB do still have the bug, and the benefit of keeping it available is huge. But at the same time for 3.6 and likely later versions too the query planner can benefit from the simpler queries produced when the workaround logic is avoided.

Since most deployments using MongoDB 3.2 and 3.4 will benefit from having the workaround in place, and some deployments using 3.6 or later will benefit from not having the workaround remain, my proposal is to add a flag allowing users running 3.6 and 4.0 to disable the feature, but leave it enabled-by-default.

In future, if Parse drops support for MongoDB versions before 3.6, the flag and the workaround behind it can be removed entirely, as they will be unnecessary.

I did my best to update the code in a way that respects the existing codebase and creates as little disruption as possible. The main changes are the addition of the new CLI flag/env variable, and a new parameter to the DatabaseController constructor representing that flag. I also updated the tests to have cases for specifying that flag as enabled & disabled.

I'd be very happy to update any relevant documentation as needed or change the PR if you have another approach to this in mind.

Thanks for taking the time to maintain this project!

[codecov]

Codecov Report

Merging #5617 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5617      +/-   ##
==========================================
+ Coverage   94.16%   94.18%   +0.01%     
==========================================
  Files         129      129              
  Lines        9244     9247       +3     
==========================================
+ Hits         8705     8709       +4     
+ Misses        539      538       -1

Impacted Files	Coverage Δ
src/Controllers/index.js	96.55% <ø> (ø)	⬆️
src/Config.js	94.82% <ø> (ø)	⬆️
src/Options/index.js	100% <ø> (ø)	⬆️
src/Controllers/SchemaController.js	96.89% <ø> (+0.01%)	⬆️
src/RestWrite.js	93.32% <ø> (+0.02%)	⬆️
src/Options/Definitions.js	100% <ø> (ø)	⬆️
src/Controllers/DatabaseController.js	95.07% <100%> (+0.06%)	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	92.45% <100%> (-0.08%)	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	96.81% <100%> (-0.1%)	⬇️
src/Adapters/Auth/httpsRequest.js	95.23% <0%> (-4.77%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d06d020...6b6bbb4. Read the comment docs.

[oallouch]

Can't we use a server status to retrieve the MongoDB server version at init ?

[NotBobTheBuilder]

That would definitely be possible. I didn't see anything like that being done at the moment though, so I thought a cli flag would be a less disruptive change

[acinader]

@NotBobTheBuilder thanks for the detailed issue and pr. Much appreciated.

This seems reasonable to me.

@davimacedo @dplewis any feedback?

[dplewis]

@oallouch Can you add a test to check if that flag can be set?

Also, I'm not asking you to do this but since travis runs Mongo 3.6 and 4.0 you could check if SERVER-13732 is indeed fixed.

Getting database version would a good feature in the future. (Maybe MasterKey API / serverInfo)

[oallouch]

I don't really see in which actual object of the API it would fit. (and in which Router)

[dplewis]

FeatureRouter, we can discuss it in a separate issue

[oallouch]

Just a link to the PR with an API for the database version : #5627
I think it's mergeable.

[NotBobTheBuilder]

I just resolved the merge conflict and will keep a look out for whether that merge has broken any tests

[acinader]

@dplewis is this ready to merge?

[oallouch]

Wouldn't it be better to just use the new database version this PR added ( #5627 ) instead of adding another option ?

[NotBobTheBuilder]

I'll remove the CLI flag and rework on top of the databaseVersion implementation (hadn't noticed that PR has been merged). Thanks!

[NotBobTheBuilder]

Updated as described!

[dplewis]

@NotBobTheBuilder I re-added the CLI option because we removed the database version feature #5681

Everything looks good to me.

[acinader]

sorry for the delay...