RUST-1512 Convert the remainder of Collection methods to use actions
#1046
Conversation
| @@ -69,6 +69,7 @@ in-use-encryption-unstable = ["mongocrypt", "rayon", "num_cpus"] | |||
| tracing-unstable = ["tracing", "log"] | |||
|
|
|||
| [dependencies] | |||
| action_macro = { path = "action_macro" } | |||
There was a problem hiding this comment.
Proc macros have to be in different crates than where they're used.
| sync_wrap, | ||
| } = parse_macro_input!(input as ActionImpl); | ||
|
|
||
| let mut unbounded_generics = generics.clone(); |
There was a problem hiding this comment.
This is the part that caused the conversion. With declarative macros, there's no way to say "match on a standard generics clause", let alone "give me a copy of that with the constraints stripped". Both of those are trivial for a proc macro.
This was needed because the remaining actions had various constraints on the generics that previous ones didn't (Serialize, etc.) and attempting to handle that in declarative mode rapidly got very ugly.
As a side benefit, this also removed the need for the various nested macro hacks to deal with optional parameters.
| @@ -86,12 +112,13 @@ macro_rules! option_setters { | |||
| } | |||
| use option_setters; | |||
|
|
|||
| pub(crate) mod private { | |||
There was a problem hiding this comment.
I added this to give us freedom to add methods to Action in the future; otherwise a user might implement it and then adding a method would be breaking.
| /// The type of the value produced by execution. | ||
| type Output; | ||
|
|
||
| pub trait Action: private::Sealed + IntoFuture { |
There was a problem hiding this comment.
Action was already de facto a subtrait of IntoFuture; declaring it means that we don't have to duplicate the Output associated type and makes it more explicit that it should be executed via .await.
| @@ -116,7 +116,7 @@ impl<'a, Session> Aggregate<'a, Session> { | |||
| } | |||
|
|
|||
| impl<'a> Aggregate<'a, ImplicitSession> { | |||
| /// Runs the operation using the provided session. | |||
| /// Use the provided session when running the operation. | |||
There was a problem hiding this comment.
The previous wording could have led readers to think that calling .session would immediately execute the action.
| } | ||
|
|
||
| impl<'a, T: Send + Sync, Mode, Session> Find<'a, T, Mode, Session> { | ||
| option_setters!(options: FindOptions; |
There was a problem hiding this comment.
Suggest keeping around FindOneOptions in the public API -- with this configuration a user could presumably do something like:
let find_options = FindOptions::builder().limit(10).cursor_type(CursorType::Tailable).build();
// configured limit is silently overriden, cursor type is effectively ignored
let doc = coll.find_one(doc! {}).with_options(find_options).await;
It's probably unlikely that a user would try to set these options for a find_one, but I think the separate types make it more clear which options are actually relevant.
There was a problem hiding this comment.
Makes sense. Since they no longer share options or output type, I just split FindOne out into its own action struct.
| UpdateModifications(UpdateModifications), | ||
| Replacement(&'a T), |
There was a problem hiding this comment.
I think we should leave this as-is. Keeping a reference to the user's input here means that we only need to allocate the byte buffer for the replacement document once, i.e. on line 34 below. Storing a RawDocumentBuf requires allocating the byte buffer once when the ReplaceOne action is constructed and then again when it's cloned on line 34 in the new diff. (This was a perf ticket a little while ago that I worked on, see RUST-877.)
There was a problem hiding this comment.
Derp, I missed that this was reverting some of your performance work, sorry!
That said, I like the other effects of using a RawDocumentBuf here instead of a &T - it makes types simpler without the lifetime and parameter, and means the human_readable_serialization option doesn't need to be propagated. I believe with mongodb/bson-rust#463 we get the best of both worlds; it avoids the intermediate buffer without having to carry a ref around.
abr-egn
force-pushed
the
RUST-1512/collection-final
branch
from
5b8dcd7 to
d432400
Compare
RUST-1512
This also necessitated converting
action_impl!into a proc_macro. I think this is an overall positive - while the code for it is now more verbose, it's also now fairly simple normal Rust code.