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

Do mongo handshake and store ServerDescription on connection #2201

Merged
merged 12 commits into from
Dec 11, 2019

Conversation

WebFreak001
Copy link
Contributor

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.

Copy link
Member

@wilzbach wilzbach left a 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.

mongodb/vibe/db/mongo/connection.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/connection.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/connection.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/connection.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/connection.d Outdated Show resolved Hide resolved
mongodb/vibe/db/mongo/connection.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
@WebFreak001
Copy link
Contributor Author

it would be amazing to have mongodb tests in general... we only have void test() in mongo unittest which is pretty useless for auto testing

@WebFreak001
Copy link
Contributor Author

@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)

@WebFreak001
Copy link
Contributor Author

bump @wilzbach @s-ludwig can I get a review on this, the better automated mongo tests and version information is pretty much a base for all other mongo features I had planned to add

@s-ludwig
Copy link
Member

s-ludwig commented Sep 2, 2018

Once the tests pass, this can be merged from my side.

@WebFreak001
Copy link
Contributor Author

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.

@WebFreak001
Copy link
Contributor Author

@wilzbach

@wilzbach
Copy link
Member

Could you rebase?

@WebFreak001
Copy link
Contributor Author

bump, #2327 depends on this now

Copy link
Member

@wilzbach wilzbach left a 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.

mongodb/vibe/db/mongo/connection.d Show resolved Hide resolved
mongodb/vibe/db/mongo/sasl.d Outdated Show resolved Hide resolved
tests/mongodb/_connection/run.sh Show resolved Hide resolved
travis-ci.sh Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@WebFreak001
Copy link
Contributor Author

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

@WebFreak001
Copy link
Contributor Author

bump, tests passing now and all reviews handled in code now

@WebFreak001
Copy link
Contributor Author

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.

@dlang-bot dlang-bot merged commit c31a3e0 into vibe-d:master Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants