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-1667 Add search index management helpers #989

Merged
merged 21 commits into from
Nov 14, 2023

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Nov 8, 2023

RUST-1667

Nothing particularly unusual. New commands, new wrappers for them, new unified tests. The only thing of note is that the commands are proxied server-side to a different process, so stuff like write concerns and sessions don't apply.

@abr-egn abr-egn marked this pull request as ready for review November 9, 2023 15:29
src/search_index.rs Show resolved Hide resolved
name: impl Into<Option<&str>>,
aggregation_options: impl Into<Option<AggregateOptions>>,
_list_index_options: impl Into<Option<ListSearchIndexOptions>>,
) -> Result<Cursor<Document>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to return a concrete type in this cursor like we do for the regular list_indexes method -- do the documents returned here match the shape of SearchIndexModel?

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 spec doesn't actually say, it just gives the return as Document. Looking at other drivers, C++ and Go don't even have support for a type-parameterized cursor; Java provides both a Cursor<Document> and a Cursor<T> but no specific concrete type. Given that, I'd like to leave it at the more general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An update here - the document returned isn't at all in the shape of SearchIndexModel, it's its own thing:

    Document({
        "id": String(
            "6553a4ec949b624697d5d1f8",
        ),
        "name": String(
            "test-search-index",
        ),
        "status": String(
            "PENDING",
        ),
        "queryable": Boolean(
            false,
        ),
        "latestDefinitionVersion": Document({
            "version": Int64(
                0,
            ),
            "createdAt": DateTime(
                2023-11-14 16:48:44.772 +00:00:00,
            ),
        }),
        "latestDefinition": Document({
            "mappings": Document({
                "dynamic": Boolean(
                    false,
                ),
            }),
        }),
        "statusDetail": Array([]),
    }),

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I was going to say that I would prefer consistency with our existing API but that's not much of a concern if the return types are so different. Sticking with Document SGTM.

/// returned.
pub async fn list_search_indexes(
&self,
name: impl Into<Option<&str>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bummer that nested impls aren't allowed, because I think this type would ideally be something like impl Into<Option<impl AsRef<str>>>. With this type definition, passing in the name field of a SearchIndexModel will require model.name.as_deref() as Option<String> don't pass the impl test. An outright String requires s.as_str().

I wonder if it would be more ergonomic to make this type Option<impl AsRef<str>>. That would allow passing model.name directly, but users would need to wrap a non-optional String or &str in Some. I'm not sure which use case is better to optimize for, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went down a rabbit hole here - I originally tried having both types be full parameters with where clauses, but that doesn't work either because it requires annotations for the None case. It seemed like providing a default for the inner type would be the way to go, but that's not supported on functions because the semantics are actually less obvious than it seems.

I'd lean towards keeping the Into<Option<..>> for consistency with the rest of the API (including the other parameters of this fn).

@@ -183,6 +183,16 @@ impl<T> Collection<T> {
}
}

pub(crate) fn clone_unconcerned(&self) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? IIRC these options are propagated from the client when a db/coll is instantiated and then resolved with the resolve_options! macro, so I think they will only get applied if they're explicitly folded into the options provided to a method (which we're not doing in these helpers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed for list_search_indexes, which is implemented as an aggregate call.

@@ -0,0 +1,151 @@
use bson::{to_bson, Bson, Document};
Copy link
Contributor

Choose a reason for hiding this comment

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

good call on moving these to their own file, operation.rs is huge and could probably use some modularization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed RUST-1800 for this.

name: impl Into<Option<&str>>,
aggregation_options: impl Into<Option<AggregateOptions>>,
_list_index_options: impl Into<Option<ListSearchIndexOptions>>,
) -> Result<Cursor<Document>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I was going to say that I would prefer consistency with our existing API but that's not much of a concern if the return types are so different. Sticking with Document SGTM.

@abr-egn abr-egn merged commit c4de516 into mongodb:main Nov 14, 2023
13 of 15 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.

2 participants