Skip to content

RUST-396 Parse responses with command errors into Errors in run_command #174

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

Merged

Conversation

patrickfreed
Copy link
Contributor

@patrickfreed patrickfreed commented May 7, 2020

RUST-396

This PR updates run_command to parse command errors from server responses and return them as Error rather than as part of the command response.

e.g.

// before
db.run_command(doc! { "asdfaf": true }, None).await; // Ok(Document({ "ok": 0, ... })

// after
db.run_command(doc! { "asdfaf": true }, None).await; // Err(ErrorKind::CommandError { ... })

Without this logic, it can be easy to ignore errors unintentionally, as is seen in the test fix included in this PR.

For completeness, I redid my survey of the other drivers, and there does not appear to be a consensus on this behavior:

  • libmongoc - validates
  • CXX - validates
  • Swift - validates
  • PHP - validates
  • Ruby - result that can be validated by user
  • Go - result that is implicitly validated in all its methods
  • Java - no validation
  • Python - optional validation, on by default
  • C# - no validation
  • Node - validates

So tallying that up, it seems 2 drivers do no validation, 2 do it optionally, and the rest validate.
I think that if we wanted to preserve the no-validation behavior, adding it as an option to run_command that is off by default would be reasonable.

@patrickfreed patrickfreed marked this pull request as ready for review May 8, 2020 00:43
@patrickfreed patrickfreed requested a review from saghm May 8, 2020 00:43
@@ -497,35 +497,6 @@ async fn saslprep_options() {
auth_test_options("IX", "I\u{00AD}X", None, true).await;
auth_test_options("\u{2168}", "IV", None, true).await;
auth_test_options("\u{2168}", "I\u{00AD}V", None, true).await;
}

#[cfg_attr(feature = "tokio-runtime", tokio::test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about why this was deleted; what does this test have to do with run_command errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The setup created the same user twice, which was always failing. The assertions are preserved, just the duplicated setup was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see. I was confused by the fact that the two tests were merged into one.

@patrickfreed patrickfreed merged commit 4ccd448 into mongodb:master May 8, 2020
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.

2 participants