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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/coll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ impl<T> Clone for Collection<T> {
}
}

#[derive(Debug)]
#[derive(Debug, Clone)]
struct CollectionInner {
client: Client,
db: Database,
Expand Down Expand Up @@ -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.

let mut new_inner = CollectionInner::clone(&self.inner);
new_inner.write_concern = None;
new_inner.read_concern = None;
Self {
inner: Arc::new(new_inner),
_phantom: Default::default(),
}
}

/// Get the `Client` that this collection descended from.
pub fn client(&self) -> &Client {
&self.inner.client
Expand Down
3 changes: 2 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ mod index;
mod operation;
pub mod results;
pub(crate) mod runtime;
mod search_index;
mod sdam;
mod selection_criteria;
mod serde_util;
Expand All @@ -362,7 +363,7 @@ pub use crate::{
gridfs::{GridFsBucket, GridFsDownloadStream, GridFsUploadStream},
};

pub use {client::session::ClusterTime, coll::Namespace, index::IndexModel, sdam::public::*};
pub use {client::session::ClusterTime, coll::Namespace, index::IndexModel, sdam::public::*, search_index::SearchIndexModel};

#[cfg(all(feature = "tokio-runtime", feature = "sync",))]
compile_error!(
Expand Down
2 changes: 2 additions & 0 deletions src/operation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ mod list_indexes;
mod raw_output;
mod run_command;
mod run_cursor_command;
mod search_index;
mod update;

#[cfg(test)]
Expand Down Expand Up @@ -73,6 +74,7 @@ pub(crate) use list_indexes::ListIndexes;
pub(crate) use raw_output::RawOutput;
pub(crate) use run_command::RunCommand;
pub(crate) use run_cursor_command::RunCursorCommand;
pub(crate) use search_index::{CreateSearchIndexes, DropSearchIndex, UpdateSearchIndex};
pub(crate) use update::{Update, UpdateOrReplace};

const SERVER_4_2_0_WIRE_VERSION: i32 = 8;
Expand Down
179 changes: 179 additions & 0 deletions src/operation/search_index.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
use bson::{doc, Document};
use serde::Deserialize;

use crate::{cmap::Command, error::Result, Namespace, SearchIndexModel};

use super::OperationWithDefaults;

#[derive(Debug)]
pub(crate) struct CreateSearchIndexes {
ns: Namespace,
indexes: Vec<SearchIndexModel>,
}

impl CreateSearchIndexes {
pub(crate) fn new(ns: Namespace, indexes: Vec<SearchIndexModel>) -> Self {
Self { ns, indexes }
}
}

impl OperationWithDefaults for CreateSearchIndexes {
type O = Vec<String>;
type Command = Document;
const NAME: &'static str = "createSearchIndexes";

fn build(&mut self, _description: &crate::cmap::StreamDescription) -> Result<Command> {
Ok(Command::new(
Self::NAME.to_string(),
self.ns.db.clone(),
doc! {
Self::NAME: self.ns.coll.clone(),
"indexes": bson::to_bson(&self.indexes)?,
},
))
}

fn handle_response(
&self,
response: crate::cmap::RawCommandResponse,
_description: &crate::cmap::StreamDescription,
) -> Result<Self::O> {
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct Response {
indexes_created: Vec<CreatedIndex>,
}

#[derive(Debug, Deserialize)]
struct CreatedIndex {
#[allow(unused)]
id: String,
name: String,
}

let response: Response = response.body()?;
Ok(response
.indexes_created
.into_iter()
.map(|ci| ci.name)
.collect())
}

fn supports_sessions(&self) -> bool {
false
}

fn supports_read_concern(&self, _description: &crate::cmap::StreamDescription) -> bool {
false
}
}

#[derive(Debug)]
pub(crate) struct UpdateSearchIndex {
ns: Namespace,
name: String,
definition: Document,
}

impl UpdateSearchIndex {
pub(crate) fn new(ns: Namespace, name: String, definition: Document) -> Self {
Self {
ns,
name,
definition,
}
}
}

impl OperationWithDefaults for UpdateSearchIndex {
type O = ();
type Command = Document;
const NAME: &'static str = "updateSearchIndex";

fn build(
&mut self,
_description: &crate::cmap::StreamDescription,
) -> crate::error::Result<crate::cmap::Command<Self::Command>> {
Ok(Command::new(
Self::NAME.to_string(),
self.ns.db.clone(),
doc! {
Self::NAME: self.ns.coll.clone(),
"name": &self.name,
"definition": &self.definition,
},
))
}

fn handle_response(
&self,
response: crate::cmap::RawCommandResponse,
_description: &crate::cmap::StreamDescription,
) -> crate::error::Result<Self::O> {
response.body()
}

fn supports_sessions(&self) -> bool {
false
}

fn supports_read_concern(&self, _description: &crate::cmap::StreamDescription) -> bool {
false
}
}

#[derive(Debug)]
pub(crate) struct DropSearchIndex {
ns: Namespace,
name: String,
}

impl DropSearchIndex {
pub(crate) fn new(ns: Namespace, name: String) -> Self {
Self { ns, name }
}
}

impl OperationWithDefaults for DropSearchIndex {
type O = ();
type Command = Document;
const NAME: &'static str = "dropSearchIndex";

fn build(
&mut self,
_description: &crate::cmap::StreamDescription,
) -> Result<Command<Self::Command>> {
Ok(Command::new(
Self::NAME.to_string(),
self.ns.db.clone(),
doc! {
Self::NAME: self.ns.coll.clone(),
"name": &self.name,
},
))
}

fn handle_response(
&self,
response: crate::cmap::RawCommandResponse,
_description: &crate::cmap::StreamDescription,
) -> Result<Self::O> {
response.body()
}

fn handle_error(&self, error: crate::error::Error) -> Result<Self::O> {
if error.is_ns_not_found() {
Ok(())
} else {
Err(error)
}
}

fn supports_sessions(&self) -> bool {
false
}

fn supports_read_concern(&self, _description: &crate::cmap::StreamDescription) -> bool {
false
}
}
1 change: 1 addition & 0 deletions src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub use crate::{
db::options::*,
gridfs::options::*,
index::options::*,
search_index::options::*,
selection_criteria::*,
};

Expand Down
135 changes: 135 additions & 0 deletions src/search_index.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
use self::options::*;
use crate::{
bson::Document,
coll::options::AggregateOptions,
error::{Error, Result},
operation::{CreateSearchIndexes, DropSearchIndex, UpdateSearchIndex},
Collection,
Cursor,
};

use bson::doc;
use serde::{Deserialize, Serialize};
use typed_builder::TypedBuilder;

impl<T> Collection<T> {
/// Convenience method for creating a single search index.
pub async fn create_search_index(
&self,
model: SearchIndexModel,
options: impl Into<Option<CreateSearchIndexOptions>>,
) -> Result<String> {
let mut names = self.create_search_indexes(Some(model), options).await?;
match names.len() {
1 => Ok(names.pop().unwrap()),
n => Err(Error::internal(format!("expected 1 index name, got {}", n))),
}
}

/// Creates multiple search indexes on the collection.
pub async fn create_search_indexes(
&self,
models: impl IntoIterator<Item = SearchIndexModel>,
_options: impl Into<Option<CreateSearchIndexOptions>>,
) -> Result<Vec<String>> {
let op = CreateSearchIndexes::new(self.namespace(), models.into_iter().collect());
self.client().execute_operation(op, None).await
}

/// Updates the search index with the given name to use the provided definition.
pub async fn update_search_index(
&self,
name: impl AsRef<str>,
definition: Document,
_options: impl Into<Option<UpdateSearchIndexOptions>>,
) -> Result<()> {
let op = UpdateSearchIndex::new(
self.namespace(),
name.as_ref().to_string(),
definition.clone(),
);
self.client().execute_operation(op, None).await
}

/// Drops the search index with the given name.
pub async fn drop_search_index(
&self,
name: impl AsRef<str>,
_options: impl Into<Option<DropSearchIndexOptions>>,
) -> Result<()> {
let op = DropSearchIndex::new(self.namespace(), name.as_ref().to_string());
self.client().execute_operation(op, None).await
}

/// Gets index information for one or more search indexes in the collection.
///
/// If name is not specified, information for all indexes on the specified collection will be
/// 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).

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.

let mut inner = doc! {};
if let Some(name) = name.into() {
inner.insert("name", name.to_string());
}
self.clone_unconcerned()
.aggregate(
vec![doc! {
"$listSearchIndexes": inner,
}],
aggregation_options,
)
.await
}
}

/// Specifies the options for a search index.
#[derive(Debug, Clone, Default, TypedBuilder, Serialize, Deserialize)]
#[builder(field_defaults(default, setter(into)))]
#[non_exhaustive]
isabelatkinson marked this conversation as resolved.
Show resolved Hide resolved
pub struct SearchIndexModel {
/// The definition for this index.
pub definition: Document,

/// The name for this index, if present.
#[serde(skip_serializing_if = "Option::is_none")]
pub name: Option<String>,
}

pub mod options {
#[cfg(docsrs)]
use crate::Collection;
use serde::Deserialize;
use typed_builder::TypedBuilder;

/// Options for [Collection::create_search_index]. Present to allow additional options to be
/// added in the future as a non-breaking change.
#[derive(Clone, Debug, Default, TypedBuilder, Deserialize)]
#[builder(field_defaults(default, setter(into)))]
#[non_exhaustive]
pub struct CreateSearchIndexOptions {}

/// Options for [Collection::update_search_index]. Present to allow additional options to be
/// added in the future as a non-breaking change.
#[derive(Clone, Debug, Default, TypedBuilder, Deserialize)]
#[builder(field_defaults(default, setter(into)))]
#[non_exhaustive]
pub struct UpdateSearchIndexOptions {}

/// Options for [Collection::list_search_indexes]. Present to allow additional options to be
/// added in the future as a non-breaking change.
#[derive(Clone, Debug, Default, TypedBuilder, Deserialize)]
#[builder(field_defaults(default, setter(into)))]
#[non_exhaustive]
pub struct ListSearchIndexOptions {}

/// Options for [Collection::drop_search_index]. Present to allow additional options to be
/// added in the future as a non-breaking change.
#[derive(Clone, Debug, Default, TypedBuilder, Deserialize)]
#[builder(field_defaults(default, setter(into)))]
#[non_exhaustive]
pub struct DropSearchIndexOptions {}
}
1 change: 1 addition & 0 deletions src/test/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ mod crud;
mod crud_v1;
mod faas;
mod gridfs;
mod index_management;
#[cfg(all(not(feature = "sync"), not(feature = "tokio-sync")))]
mod initial_dns_seedlist_discovery;
mod load_balancers;
Expand Down
7 changes: 7 additions & 0 deletions src/test/spec/index_management.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use crate::test::spec::unified_runner::run_unified_tests;

#[cfg_attr(feature = "tokio-runtime", tokio::test)]
#[cfg_attr(feature = "async-std-runtime", async_std::test)]
async fn run() {
run_unified_tests(&["index-management"]).await;
}
Loading