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

RUST-1588: Add RunCursorCommand #912

Merged
merged 15 commits into from
Jul 19, 2023
Merged

Conversation

drshika
Copy link
Contributor

@drshika drshika commented Jul 10, 2023

added operation RunCursorCommand (spec: https://github.com/mongodb/specifications/blob/c09f979ad296400552a98c9b784197ec648c2096/source/run-command/run-command.rst#runcursorcommand)

  • implemented Operation which uses default RunCommand behavior for every method but handle_response

@drshika drshika marked this pull request as ready for review July 10, 2023 18:31
@drshika drshika requested a review from abr-egn July 11, 2023 13:16
@abr-egn abr-egn changed the title RUST-1636: Add RunCursorCommand RUST-1588: Add RunCursorCommand Jul 11, 2023
src/db/options.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db/options.rs Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db/options.rs Outdated Show resolved Hide resolved
src/db/options.rs Outdated Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
@abr-egn abr-egn requested a review from isabelatkinson July 14, 2023 18:01
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM! Tagged Isabel in.

src/db/options.rs Outdated Show resolved Hide resolved
src/db/options.rs Outdated Show resolved Hide resolved
src/db/options.rs Outdated Show resolved Hide resolved
src/db/options.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
@drshika drshika requested a review from isabelatkinson July 17, 2023 20:53
src/db/options.rs Outdated Show resolved Hide resolved
src/operation/run_cursor_command.rs Outdated Show resolved Hide resolved
@drshika drshika requested a review from isabelatkinson July 18, 2023 15:28
src/db/options.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
drshika and others added 3 commits July 18, 2023 12:41
Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>
Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>
@drshika drshika requested a review from isabelatkinson July 19, 2023 16:44
src/db.rs Outdated
pub async fn run_cursor_command(
&self,
command: Document,
options: RunCursorCommandOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

The option type still needs to be updated here

src/db.rs Outdated
Comment on lines 502 to 505
let selection_criteria = match options.clone() {
Some(options) => options.selection_criteria,
None => None,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than cloning the entire options struct, it would be more efficient only to clone the selection_criteria. You should be able to do this by using a reference to the options, which can be done either like this:

let selection_criteria = match options {
    Some(ref options) => options.selection_criteria.clone(),
    None => None,
};

or this:

let selection_criteria = match &options {
    Some(options) => options.selection_criteria.clone(),
    None => None,
}

(I slightly prefer the first one for no good reason; either is totally fine.)

You can also use and_then to achieve the same thing:

let selection_criteria = options
    .as_ref()
    .and_then(|options| options.selection_criteria.clone());

We use as_ref to get a reference to the struct inside the Option rather than an owned version of it so that we can continue to use it later. and_then does something called a flat map (you can read more about it here), which is useful when you want to avoid getting data that's wrapped in multiple layers of Options/Results.

All three of these options would work here, but it's generally considered best practice to use and_then rather than an explicit match in situations like these.

src/db.rs Outdated
options: impl Into<Option<RunCursorCommandOptions>>,
session: &mut ClientSession,
) -> Result<SessionCursor<Document>> {
let mut options: Option<RunCursorCommandOptions> = options.into().clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

cloning shouldn't be necessary here

src/db.rs Outdated
Comment on lines 506 to 509
let option = match options.clone() {
Some(options) => options,
None => RunCursorCommandOptions::default(),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than creating an empty options struct when the user hasn't provided any options, we can just make the options field on RunCursorCommand an Option<RunCursorCommandOptions>. This will save us an allocation when options is None. (You'll probably need to do similar calls to and_then in handle_response as I described above to extract fields from the options).

@drshika drshika requested a review from isabelatkinson July 19, 2023 18:59
@isabelatkinson
Copy link
Contributor

I think the changes still need to be pushed :) it looks like there are also some compilation errors with CSFLE on the CI run

@drshika
Copy link
Contributor Author

drshika commented Jul 19, 2023

@isabelatkinson changes should be in here: 54185f8

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 tiny suggestion but otherwise lgtm! no need for a re-review

src/db.rs Outdated Show resolved Hide resolved
@isabelatkinson
Copy link
Contributor

Ah it actually looks like there are some clippy and cargo-deny failures, those will need to be fixed up as well.

Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>
@drshika drshika merged commit c242539 into mongodb:main Jul 19, 2023
@drshika drshika deleted the RUST-1636/run-command branch July 19, 2023 21:29
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