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

Add maxTimeMS to FindOptions #340

Merged
merged 14 commits into from
Apr 1, 2022
Merged

Add maxTimeMS to FindOptions #340

merged 14 commits into from
Apr 1, 2022

Conversation

BrunoBernardino
Copy link
Contributor

This is allowed, works, and is already being sent over, so it's really just a type thing.

This is allowed, works, and is already being sent over, so it's really just a type thing.
@lucsoft
Copy link
Collaborator

lucsoft commented Mar 31, 2022

can you add a test case please?

@BrunoBernardino
Copy link
Contributor Author

Alright, will do!

@lucsoft
Copy link
Collaborator

lucsoft commented Apr 1, 2022

one more thing the spec says that maxTimeMS should be a function like find().maxTimeMS() not an option parameter

but the tests look good! thanks so much!

@manyuanrong
Copy link
Member

Before 4.4, the documentation didn't explain much about errObj, and the codeName field might not be available before 4.2.
refs:
https://www.mongodb.com/docs/v4.0/reference/method/db.getLastErrorObj/#db.getLastErrorObj
https://www.mongodb.com/docs/v4.4/reference/method/db.getLastErrorObj/#db.getLastErrorObj

@BrunoBernardino
Copy link
Contributor Author

one more thing the spec says that maxTimeMS should be a function like find().maxTimeMS() not an option parameter

but the tests look good! thanks so much!

I think I've seen that for the mongo shell function but not for the node.js driver documentation. Can you link to where you saw that?

@BrunoBernardino
Copy link
Contributor Author

Took quite a while, but I finally nailed the problem. findOne doesn't seem to support maxTimeMS in mongo 4.0, only 4.2+, so it wasn't failing. I'm not certain if it's an implementation bug (unlikely since it works as expected for 4.2+), or simply something I didn't see in the documentation. You can see my journey in the commits, but hopefully the tests are good now.

@BrunoBernardino BrunoBernardino requested a review from lucsoft April 1, 2022 16:33
@lucsoft lucsoft merged commit f7e439c into denodrivers:main Apr 1, 2022
@BrunoBernardino BrunoBernardino deleted the patch-1 branch April 1, 2022 17:38
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.

3 participants