Skip to content

Conversation

beckerinj
Copy link
Contributor

The database run_command api only takes a Document, while internally it is later converted to a RawDocumentBuf before execution. In a situation where you already have the command as a RawDocumentBuf in the first place, an unneeded conversion RawDocumentBuf -> Document -> RawDocumentBuf is required to use this API.

I noticed that there was this RunCommand::new_raw method, which was feature gated behind "in-use-encryption". I'm not sure the reason for this gate, but it's trivial to remove it and use it with the additional database.raw_run_command method added here.

I only added raw_run_command, but a similar raw_run_cursor_command could also be added which takes RawDocumentBuf as well.

@isabelatkinson
Copy link
Contributor

@beckerinj thank you for this PR! I'll discuss this addition with the team early next week and get back to you on whether we would like to add this to the driver API.

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

I discussed with the team and we agreed that this is a nice addition to our API. I have a suggestion for reducing duplication in the action types; please let me know if you have any questions!

@isabelatkinson isabelatkinson changed the title Add Database raw_run_command RUST-2198 Add run_raw_command method Apr 16, 2025
@beckerinj beckerinj requested a review from a team as a code owner April 16, 2025 21:56
@beckerinj beckerinj requested review from abr-egn and removed request for a team April 16, 2025 21:56
fix docs

don't change formatting
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

Apologies, hit approve before checking CI. It looks like compilation is broken with in-use-encryption enabled.

@beckerinj
Copy link
Contributor Author

I suspected there are some other dependents which need changing under the different flags.

@beckerinj
Copy link
Contributor Author

I believe the latest commit should fix the in-use-encryption issue.

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

one last fix needed based on the CI run, after that this should be good to go

match session.transaction.state {
TransactionState::Starting | TransactionState::InProgress => {
if self.command.contains_key("readConcern") {
if command.get("readConcern").ok().is_some() {
Copy link
Contributor

@isabelatkinson isabelatkinson Apr 17, 2025

Choose a reason for hiding this comment

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

Suggested change
if command.get("readConcern").ok().is_some() {
if command.get("readConcern").is_ok_and(|rc| rc.is_some()) {

get returns an error if the document is malformed, so this as currently written will evaluate to true for any valid document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, made this change 👍

@beckerinj
Copy link
Contributor Author

Apologies my formatter keeps auto formatting the imports, I just undid it again.

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

thanks for the formatting fix, lgtm! @abr-egn this is ready for your review

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Changes LGTM with a doc fix and suggested test.

@beckerinj beckerinj requested a review from kevinAlbs April 25, 2025 01:17
@beckerinj
Copy link
Contributor Author

Cheers, no problem, I've made the requested changes and added the test.

@isabelatkinson isabelatkinson merged commit f258c21 into mongodb:main Apr 25, 2025
15 of 17 checks passed
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