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

MongoDB 5.1+ driver compatibility #2691

Merged
merged 36 commits into from
Oct 19, 2022
Merged

Conversation

WebFreak001
Copy link
Contributor

@WebFreak001 WebFreak001 commented Sep 30, 2022

So there was a bunch of legacy code from ancient MongoDB driver versions still around, which made this a bit difficult. I mostly kept old compatibility in, however since this release we are only going to officially support MongoDB 3.6 and up. (which has long been EOLd anyway) In theory a bunch of stuff still works with even older servers though, but I would not give any guarantees there. Methods that I could not upgrade transparently I simply deprecated with a note that they will stop working on MongoDB 5.1 and up, due to them dropping support for all packets except OP_MSG, which is now used everywhere.

The basic find method without and with query was upgraded transparently, however you can no longer use $query and query fields inside your query to overwrite the whole query. (was never documented anyway and I'm sure that's more of a security vulnerability than a feature)

The other stuff is still mostly there, but we now implement the proper CRUD API that the MongoDB driver spec mandates, making us consistent with all other drivers as well as supporting all the MongoDB server versions since 3.6.

I have updated the CI, it no longer runs the MongoDB tests on the general CI actions on all OSes, however now it instead it only runs the MongoDB tests on Ubuntu-20.04, but with all the stable versions from 3.6 up to 6.0.

Still needs some more testing and CI fixes, but the basic OP_MSG implementation and adoption of cursors, handshake, authentication, CRUD methods and runCommand is there.

Maybe the PR looks a little large at first, but these components are really quite interconnected. It would be possible to split it in a few places though, if desired.

This PR contains full CRUD API implementation because since we cannot use OP_QUERY anymore, we need to use query commands, which are defined in the driver spec as this CRUD API.

What I very much like about the unification of the runCommand, query and OP_MSG things into a single runCommand function now: a lot of code duplication of error checking has been removed and unified into the single function, which makes the code a lot more resilient imo. User-custom methods still run without error checking by default in the deprecated overload, but it recommends them to use the overload that explicitly takes in a boolean if we want error checking or not.

A few of the MongoDB index APIs have been creating custom cursors before, manually parsing the cursor response object. This has now all been moved into the MongoCursor class and it now only has a single implementation, no longer a Find and a Generic implementation, which I think makes it also easier to read.

@WebFreak001
Copy link
Contributor Author

performance impact on my local machine in the bench-mongodb example: queries are about 25% slower now

@WebFreak001
Copy link
Contributor Author

supersedes #2679 and #2616

as we simply discard these parameters, we declare those scope, as they
might be scope in the type definition if the template parameter is true.

Old DMD versions don't allow us to pass delegates here if we omit scope
on scope parameters.
@gedaiu
Copy link
Contributor

gedaiu commented Sep 30, 2022

nice! thank you!

please also have a look at my small change for adding basic support for replica sets #2690

@WebFreak001
Copy link
Contributor Author

need to consult the driver spec about replica sets again, there is a document about server selection we probably want to support in the long run.

@WebFreak001 WebFreak001 mentioned this pull request Oct 18, 2022
37 tasks
Copy link
Contributor

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty hard to review such a diff. Any chance you could break some chunks off of it ? If it's too much work, I'd rather err on the side of merging, since we 1) need this ASAP and 2) have a better CI with this diff.
I made a few comments though.

.github/workflows/mongo.yml Outdated Show resolved Hide resolved
.github/workflows/mongo.yml Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/database.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/impl/crud.d Show resolved Hide resolved
mongodb/vibe/db/mongo/impl/crud.d Show resolved Hide resolved
mongodb/vibe/db/mongo/cursor.d Show resolved Hide resolved
mongodb/vibe/db/mongo/cursor.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/cursor.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/settings.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/settings.d Outdated Show resolved Hide resolved
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