-
Notifications
You must be signed in to change notification settings - Fork 185
RUST-2157 Complete set of Atlas Search helpers #1462
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
Conversation
etc/gen_atlas_search/src/main.rs
Outdated
#[doc = #desc] | ||
#[doc = ""] | ||
#[doc = #link] | ||
pub fn #constr_ident(#required_args) -> AtlasSearch<#name_ident> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled the constructor functions out of the AtlasSearch
struct because it got very noisy when using helpers like compound
that can have lots of sub-clauses; this way users can either have the names prefixed or not depending on their choice of import.
(Couldn't do use AtlasSearch::*
, sadly, the compiler complains that AtlasSearch
isn't a namespace, despite the same syntax working for enum variants.)
_t: PhantomData<T>, | ||
} | ||
|
||
impl<T> From<AtlasSearch<T>> for Document { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swapped this for stage
because the document constructed is very specific to the context of an aggregation pipeline rather than being a general conversion (and it's just one more character).
fn to_bson(self) -> Bson { | ||
Bson::String(self.to_owned()) | ||
/// An Atlas Search operator parameter that can accept multiple types. | ||
pub trait Parameter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup with the Parameter
trait means both that the specific child traits are sealed (it wouldn't make sense for users to add new impls for those, they're just ways to define subsets of bson values) and that the conversion to a value for inclusion in the pipeline doc doesn't have to be defined for each one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this is very convenient!
src/atlas_search.rs
Outdated
/// An Atlas Search operator parameter that is itself a search operator. | ||
pub trait SearchOperator: private::Parameter {} | ||
impl<T> SearchOperator for AtlasSearch<T> {} | ||
impl SearchOperator for Document {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is future-proofing, so users can always fall back to directly using documents if our builders don't have the syntax they need for a subclause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here and to aggregate.rs are from when I was considering a new action as the entry point; I ended up not doing that but these seemed reasonable microimprovements to keep.
} | ||
} | ||
/// A pending `$search` aggregation stage. | ||
pub struct AtlasSearch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to exist as a place to hang options for the $search
stage rather than the individual operator. This does mean you get a little awkward when setting options (search(operator(param)).option(value).stage()
) but that can be sidestepped with the operator's stage()
when no options need to be set.
Alternatives considered:
operator(param).search().option(value).stage()
syntax instead; subjectively it stutters, and it also doesn't reflect the underlying model as well.- New
search
action distinct fromaggregate
. Would require duplicating all ofaggregate
's options and would then conflate options for the stage with options for the operation, which doesn't seem great and could run into name clashes. - Include
search
option setters in the genericSearchOperator<T>
impl; allows nonsensical things like trying to set a different index on a nested operator than the toplevel one, and has the same name clash issue as the action does.
2f78823
to
a42040e
Compare
a42040e
to
2605f47
Compare
src/atlas_search.rs
Outdated
|
||
/// Document that specifies the tracking option to retrieve analytics information on the search | ||
/// terms. | ||
pub fn tracking(mut self, value: Document) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is scheduled for removal later this year: https://www.mongodb.com/docs/atlas/atlas-search/aggregation-stages/search/#fields
I suggest we omit it entirely from our API since it'll stop working fairly soon
src/atlas_search.rs
Outdated
} | ||
impl Facet<Date> { | ||
#[allow(missing_docs)] | ||
pub fn default(mut self, bucket: impl AsRef<str>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way on this, but I was initially confused here by the naming collision with Default::default
- thoughts on naming this something like default_bucket
instead?
fn to_bson(self) -> Bson { | ||
Bson::Array(self.iter().map(|s| Bson::String(s.clone())).collect()) | ||
/// A facet definition. | ||
pub struct Facet<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding a link to the facet docs for more details: https://www.mongodb.com/docs/atlas/atlas-search/facet/
Also, can you add docs for the options setters? They can probably just be copied from what's in the docs page
src/atlas_search.rs
Outdated
} | ||
|
||
/// Convert to an aggregation stage document. | ||
pub fn stage(self) -> Document { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's more verbose, but I would prefer into_stage
here to make it more clear that this is the final thing that needs to happen when constructing a search stage; otherwise this reads more as another constructor method
fn to_bson(self) -> Bson { | ||
Bson::String(self.to_owned()) | ||
/// An Atlas Search operator parameter that can accept multiple types. | ||
pub trait Parameter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this is very convenient!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
RUST-2157
This builds on #1451 and implements the expanded machinery to generate the rest of the helpers. I'd say most of them needed some kind of addition beyond just "include the yaml file".