RUST-2103 Update the rest of actions to document options #1272

abr-egn · 2024-12-20T17:26:35Z

This did involve one more rework of how the macros handle things (sorry!); with that, there's now a family of three related macros:

option_setters: takes the path to an options struct and generates setters for the fields of that struct in the impl block to which it's attached. Can be optionally told to skip setters for some of the fields.
export_doc: generates a notional named "doc item" for use by options_doc from an impl block. Can be optionally told to add entries to the doc item that aren't present in the code of the impl.
options_doc: consumes a "doc item" to add rustdoc listing option setters to a fn. Can be optionally told it's a sync fn to adjust the rustdoc output. (unchanged from last PR)

With this split, I think it's both easier to read and understand (no more multiline kwarg macro invocations) and it much better handles edge cases, of which we have surprisingly many.

Oh, and I also split up the contents of macros/src/lib.rs since that had gotten quite big.

abr-egn · 2024-12-20T17:29:45Z

macros/src/action_impl.rs

Nothing in here changed, just moved to its own file.

abr-egn · 2024-12-20T17:31:45Z

macros/src/lib.rs

-    ($span:expr, $($message:tt)+) => {{
-        return Error::new($span, format!($($message)+)).into_compile_error().into();
-    }};
+    crate::action_impl::action_impl(attrs, input)
 }

 /// Enables rustdoc links to types that link individually to each type
 /// component.
 #[proc_macro_attribute]


This has to be attached to a fn at the toplevel module for whatever reason, hence the wrapper rather than just re-exporting.

abr-egn · 2024-12-20T17:37:58Z

macros/src/option.rs

+    impl_in.to_token_stream().into()
+}
+
+pub(crate) struct OptionSettersArgs {


Previously for the macro_magic custom args I was keeping every token individually and laboriously reconstructing the original in ToTokens, which was a giant hassle with any sort of change. I realized that it's much easier to just keep a clone of the entire input token stream and then only have struct members for the bits actually needed.

abr-egn · 2024-12-20T17:50:18Z

macros/src/rustdoc.rs

deeplink and options_doc are unchanged.

abr-egn · 2024-12-20T17:52:32Z

src/action/aggregate.rs

@@ -30,7 +38,7 @@ impl Database {
    /// returned cursor will be a [`SessionCursor`]. If [`with_type`](Aggregate::with_type) was
    /// called, the returned cursor will be generic over the `T` specified.
    #[deeplink]
-    #[options_doc(aggregate_setters)]
+    #[options_doc(aggregate)]


Because the name is translated into a mangled representation, it's effectively in its own namespace and won't clash with anything. Having it be the same name as the fn it's attached to seemed reasonable, the _setters suffix wasn't doing anything helpful.

abr-egn · 2024-12-20T18:08:23Z

src/action/csfle/encrypt.rs

-        range_options: RangeOptions,
-    );
-}
+#[option_setters(EncryptOptions, skip = [query_type])]


Here's one of the fun corner cases! The Encrypt action is shared across both encrypt and encrypt_expression; we want to expose the query_type option for the former, both in code and documentation, but not the latter. So:

For the generic impl block, the generated setter for query_type is skipped, and it's manually implemented for Encrypt<'_, Value> only.

For the generated documentation for encrypt, the manually written setter for query_type is added to the list

For the generated documentation for encrypt_expr, it isn't

Somewhat tangential, but this led me to find a niche corner case that can lead to bad behavior when using with_options in a call to encrypt_expression:

let options = EncryptOptions::builder() .contention_factor(0) .range_options(range_options) .build(); let result = client_encryption .encrypt_expression(...) .with_options(options) .await?;

The call to encrypt_expression sets the query_type to the correct value on the options internally, but chaining with_options wipes this value, and the following error is returned: EncryptExpression may only be used for range queries.

We should add the following validation to the IntoFuture impl for Encrypt<Expression>:

if query_type is set to Some(<string not equal to "range">), return an error indicating that query_type cannot be set via with_options for calls to encrypt_expression

if query_type is set to None, manually re-set the value to Some("range") as the user inadvertently overrode it by the call to with_options

I can make a follow-up PR to fix this; we're also missing a TypedBuilder impl for EncryptOptions so I'll add that too.

Nice catch! Follow up PR makes sense, thank you :)

abr-egn · 2024-12-20T18:12:56Z

src/action/shutdown.rs

@@ -130,6 +134,7 @@ pub struct Shutdown {
    pub(crate) immediate: bool,
 }

+#[export_doc(shutdown)]


Another corner case: we want to generate documentation for the options for shutdown, but there's no corresponding options object.

isabelatkinson

the updated docs look great!

isabelatkinson · 2024-12-31T21:47:17Z

src/action/csfle/encrypt.rs

-        range_options: RangeOptions,
-    );
-}
+#[option_setters(EncryptOptions, skip = [query_type])]


Somewhat tangential, but this led me to find a niche corner case that can lead to bad behavior when using with_options in a call to encrypt_expression:

let options = EncryptOptions::builder() .contention_factor(0) .range_options(range_options) .build(); let result = client_encryption .encrypt_expression(...) .with_options(options) .await?;

The call to encrypt_expression sets the query_type to the correct value on the options internally, but chaining with_options wipes this value, and the following error is returned: EncryptExpression may only be used for range queries.

We should add the following validation to the IntoFuture impl for Encrypt<Expression>:

if query_type is set to Some(<string not equal to "range">), return an error indicating that query_type cannot be set via with_options for calls to encrypt_expression

if query_type is set to None, manually re-set the value to Some("range") as the user inadvertently overrode it by the call to with_options

I can make a follow-up PR to fix this; we're also missing a TypedBuilder impl for EncryptOptions so I'll add that too.

abr-egn added 30 commits December 11, 2024 09:58

create_index

07bcc50

create_data_key

cb77079

wip: rework option_setter2 arg parsing

cafdf97

clean up parsing

414242c

split up

810d99c

untested skip

f935c50

encrypt (but its not right)

54f44cc

split macros

57b038b

fix encrypt doc

42b4e2b

create_encrypted_collection

b3ad8d9

delete

2361b3a

distinct

5c282c5

drop_index

c139bbb

drop

895a39d

find_and_modify

aba8401

gridfs

71b77b3

insert_mant

e6a5bb9

insert_one

a44b6c9

list_collections

b52b87f

list_databases

47e245e

list_indexes

e6c28e5

drop _setters suffix

63cf7d4

replace_one

d1a9d6a

run_command

6b6e238

search_index

7cb7c7b

session

08decda

shutdown

f7313df

transaction

fcdabb3

update

440d33f

watch

438bd2f

abr-egn added 3 commits December 20, 2024 11:57

drop old option_setters

1596abe

rename

4c994ff

fix imports

7ed6a86

abr-egn commented Dec 20, 2024

View reviewed changes

abr-egn marked this pull request as ready for review December 30, 2024 14:55

abr-egn requested a review from isabelatkinson December 30, 2024 15:07

isabelatkinson approved these changes Dec 31, 2024

View reviewed changes

abr-egn merged commit 2815edc into mongodb:main Jan 2, 2025
17 checks passed

isabelatkinson mentioned this pull request Jan 3, 2025

Disallow setting query_type for encrypt_expression #1275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-2103 Update the rest of actions to document options #1272

RUST-2103 Update the rest of actions to document options #1272

abr-egn commented Dec 20, 2024 •

edited

Loading

abr-egn Dec 20, 2024

abr-egn Dec 20, 2024

abr-egn Dec 20, 2024

abr-egn Dec 20, 2024

abr-egn Dec 20, 2024

abr-egn Dec 20, 2024

isabelatkinson Dec 31, 2024

abr-egn Jan 2, 2025

abr-egn Dec 20, 2024

isabelatkinson left a comment

isabelatkinson Dec 31, 2024

RUST-2103 Update the rest of actions to document options #1272

RUST-2103 Update the rest of actions to document options #1272

Conversation

abr-egn commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelatkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Dec 20, 2024 •

edited

Loading