-
Notifications
You must be signed in to change notification settings - Fork 283
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
Do mongo handshake and store ServerDescription on connection #2201
Conversation
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.
Added a few initial comments.
Would be amazing to have a test for this.
it would be amazing to have mongodb tests in general... we only have |
@wilzbach I have added a mongodb test runner now which contains the old mongodb unittest and now also a unittest testing several connection/auth methods. I couldn't get username/password auth to behave properly with vibe.d, it doesn't throw errors on login failure and it receives unknown data when trying to iterate over data. (both logged in and anonymous) |
Once the tests pass, this can be merged from my side. |
are we sure the current mongodb login connection even works as expected? I modelled the exceptions that are supposed to be thrown after the mongo console behaviour and because there isn't thrown anything on failure it makes me think that maybe the insertion/removal code is missing checks for successful completion. |
Could you rebase? |
also makes authentication only use SCRAM on 3.0+
it's just ignored in old versions according to mongo spec
d4f26a1
to
8db268d
Compare
bump, #2327 depends on this now |
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.
Thanks a lot for this essential work @WebFreak001, but as it touches a bit of the test system too, I prefer like to wait until @s-ludwig has had a chance to look over it too.
bump, I think the current indexes code broke with MongoDB 4.2 now because #2327 is probably needed and this PR is needed to add the version-dependent code |
bump, tests passing now and all reviews handled in code now |
the changes aren't that much here, the tests and (de)serialization structs/enums which get handled automatically make up about 50%+20% of the PR. Also there is added security and unittest on the sasl.d file which could really be moved into a separate PR but I don't think it hurts here. If that's the blocker, please tell me so I can move that to a separate PR. |
It seems not all types in the struct are used but this is required for future changes like server selection or changed APIs (see #2200)
Also makes the vibe app more distinguishable in the mongodb admin log because you know which application was doing stuff then.