Operation Builder Future

[rylev]

coauthored with @yoshuawuyts

This is a proposal for a new one style of API for operations that eliminates some of the awkwardness of usage without reducing the functionality.

Summary

Operations now return "builders" which implement an into_future method (more of this later) that turns the builder into a future which can be awaited with await and yields a response. These builders provide setter methods for any options as well as a way to set a Context object.

Currently

collection_client
            .create_document(
                my_context,
                &document_to_insert,
                CreateDocumentOptions::new().is_upsert(true),
            )
            .await?;

After this PR

collection_client
            .create_document(&document_to_insert)
            .is_upsert(true) // if the default is desired this line can be removed
            .context(my_context) // if no context is needed, this line can be removed
            .into_future() // more of this below
            .await?;

into_future

Currently, when the code some_value.await is called, Rust desugars this to a direct call to the poll function on Future. However, there is an accepted RFC and implementation that changes this desugaring to call the currently unstable IntoFuture::into_future. When this lands on stable Rust, we will be able to remove the need for calling into_future() as the call to await will call into_future automatically.

The builder pattern

The builder pattern presented here is very common (and is in fact what the Azure SDK for Rust used before the pipeline architecture). Here are examples of it being used in the most popular HTTP crates for Rust:

• hyper
• reqwest
• surf

Thoughts?

cc @JeffreyRichter @heaths

I also just saw @JeffreyRichter's comment on requiring Context. That doesn't change this proposal too much, it would just mean keeping context as the first argument to every operation.

[bmc-msft]

I very much like the change, though into_future feels awkward with a runtime specific language.

Potential alternatives:

• Existing builders in the SDK (like blob_client.put_block_list) use execute.
• hyper uses build in the example you provided, but as an example, the connection builder (src/client/conn.rs) uses handshake.
• surf uses build
• reqwest uses build or send

[MindFlavor]

Excellent job! I really like the new (old?) resulting API.
I also like your take on Context, I would really like to avoid forcing a useless parameter if we can help it. Your way gives the control back to the SDK user.

The only thing I would like to change is the insert function name which I believe is too vague (but it's fine if you don't think it is 😄).

[MindFlavor]

Note on Context: I like it the way it's implemented here 👍. Having a mandatory, possibile empty struct in every function call is ugly: this way the SDK user will only pass the Context if it's meaningful in some way.

[MindFlavor]

I think we should come up with a more explicit function name. insert does not make clear that we are changing the Context. Maybe something like override_context (supposing to merge #508 as well)?

Side note: What about having a trait for this function? It won't change much but it would signal unequivocally the meaning of this.
What do you think?

[yoshuawuyts]

What about having a trait for this function? It won't change much but it would signal unequivocally the meaning of this.

We discussed this, and adding a trait means that users of the method would need to import it into scope before being able to use it. That's not necessarily a bad idea, but before we decide to do that we should probably understand better how often the context interface is used.

We settled on keeping it as an inherent function for this proposal, but leaving the option open to re-evaluate once we have a better grip on usage.

[MindFlavor]

Nice 💘!

[yoshuawuyts]

• surf uses build
• reqwest uses build or send

In both reqwest and surf, the method equivalent to what we're doing here would be send. The build method synchronously constructs a Request type, which serves a slightly different purpose ¹. For surf I originally picked send as the API name because that was reqwest called their method, and we weren't aware yet that IntoFuture would be part of async/.await semantics. My post announcing surf predates my post on async builders by almost three months.

With the knowledge we have today, if I could go back and design it again I'd instead would've named the method into_future to be forward-compatible with the stabilization of IntoFuture::into_future. The plan is that nobody should ever need to call into_future by hand; but if they really want to they still can. Which if things go well, might be as soon as Rust 1.58.

Footnotes

1. This is nuanced and probably confusing since Request also implements Future. But at least for surf this is only because of a lack of IntoFuture on stable. Once that lands we plan to replace the Future impl on Request with IntoFuture instead. I'm sharing this here because the context may not necessarily be clear from the API docs. ↩

[cataggar]

Much better. To confirm, all required parameters will be in the function signature,. All optional parameters are setter methods. Correct?

[yoshuawuyts]

[...] all required parameters will be in the function signature,. All optional parameters are setter methods. Correct?

That's indeed the idea.

[rylev]

That is correct.

[heaths]

This is close to what we do across other languages, but idiomatically there are some differences. For example, in .NET, once we hit ~6 (it's not a hard limit) required parameters, we do a {MethodName}Options class. In fact, that naming convention is consistent across languages when used. For Python, they use a combination of named params and kwargs. Java and JavaScript are similar in nature to .NET, though JS uses options bags (typed in TS, but of course doesn't really matter in pure JS).

We should follow suit here. So any optional parameters should maybe be in a {MethodName}Options class for methods. For clients, it's typically {ClientName}Options, but Java uses a builder pattern that basically follows what @cataggar mentioned above.

[cataggar]

serde_json::to_string is always a red flag. Recommend using to_json. Currently in core::http_client.

[cataggar]

That was existing code, so we can put this in a new issue. No need to hold up this awesomeness.

[cataggar]

I would appreciate reviewing how this can be integrated into the generated services in #520.

[heaths]

The context above is no longer needed, correct?

[JeffreyRichter]

I'm surprised to see this PR. In the early days of this effort, the builder pattern for operations was used and then we went away from to the pattern shown "currently". But, now it seems that you're going back to what the original pattern was.

Personally, I'm not a fan of the builder pattern as it introduces a lot of uncertainty in the code.
For example, let's say your code above is modified like this (pseudo-code, not necessarily RUST):

a = collection_client.create_document(&document_to_insert)
b = a.is_upsert(true) // if the default is desired this line can be removed
c = b.context(my_context) // if no context is needed, this line can be removed
Foo(c)        // Call Foo passing c here

Foo(c) {
   // Foo doesn't know what state c is in now: Is upsert true? is there already a context?
   d = c.is_upsert(true)   // What does this do?
   e = d.context(my_context) // What does this do?
    e.into_future().await?;
    e.into_future().await?   // Can this be done twice?
    
    // Can we call into_future once and then await it twice?
   f = e.into_future()
   f.await?
   f.await?

// I don't know how customers can rationalize about the state of "the operation object" and if
// something goes wrong with e service call, how can the customer determine the state and, more
// importantly, which piece of code (Foo or one of Foo's callers) set that state?

Also, it's not obvious that this does:

   d = c.is_upsert(true)
   e = d.is_upsert(false)

Does first caller win? Does last caller win? In this case, last caller probably wins but for some properties (like context) it's not obvious.

My memory of our early discussions was that we would not use the builder pattern for client operations but that we would use it for operation optional parameters. I didn't think the builder pattern for this was good either; I prefered just a simple structure with fields in it. But, in Rust, there is a problem with this: when you add new fields in a new version all callers are broken because RUST requires setting fo all the fields. So, adding fields was not backward compatible in Rust. To "fix" this, we agreed to using the builder pattern for option structures so only the fields set by methods would be changed. Any new fields would have new methods and the calling code just wasn't calling these methods which is fine and backward compatible.

[yoshuawuyts]

Hey @JeffreyRichter, thanks for asking! You're indeed right that we're going back to a design that more closely resembles what we started out with! It seems you have some questions about this direction. I want to make sure you feel they are addressed, so I've attempted to summarize and answer them individually. Let me know whether this makes sense, and if you have any other questions about the design!

What happens if a setter method on a builder is called twice?

The builder pattern is fairly common in Rust code; both in the ecosystem, and in the standard library. There are a few variations on the builder pattern, but in all cases the resulting outcome of calling the same method twice in a row is that the last call wins.

let mut builder = client.create_document(&doc)
    .is_upsert(true)
    .is_upsert(false);

// the value of `is_upsert` was set to `false`.

In addition to that, we can expect the Rust optimizer to detect the sequential calls to is_upsert, and entirely optimize out the intermediate call to is_upsert(true). So in terms of outcome we can think of is_upsert(true) as if it was never called.

If we use a builder pattern, how can we know whether Context has been set?

I've translated the Rust pseudocode above into the following example:

let builder = client.create_document(&doc);
foo(builder);

fn foo(builder: CreateDocumentBuilder) {
    // How can we know whether a `Context`
    // is available here?
}

In the implementation of the PR, we guarantee that a Context object has always been instantiated. So to directly answer the question asked: Context will always be available in foo's body. And because instantiating an empty Context is zero cost, we aren't penalized for using this pattern.

This brings us to the question: "What is it that we want to do with the Context in function foo's body?". We touched on this in yesterday's meeting (I think this might have been before you joined?). We're thinking of providing two interfaces for interacting with the context through builders ¹:

• Builder::context: to whole-sale override the context with a different context.
• Builder::insert: to insert/override a single item into the Context's type map.

Builder::insert follows the same precedence logic as all other methods on Builder: we mutate a shared state, and the last call to insert a value wins.

If we just want to override a single value (e.g. change the retry policy) we can just do so. But if we want to whole-sale replace the existing context with something else we've created, we can do so too.

How can we override values set in Context in bulk?

There is one type of operation that we haven't covered yet: what if we want to bulk-insert values from within function foo's body. Ryan and I talked about this scenario while pairing, and decided against covering it for now. But if this is something we'd like to support we could easily do so through an impl Extend<Context> for CreateDocumentBuilder {}. This would enable us to write:

async fn main() {
    let mut builder = client.create_document(&doc)
        .insert(Foo::new())
        .insert(Bar::new())
        .insert(Baz::new());
    foo(builder).await;
}

fn overrides() -> Context {
    let mut cx = Context::new();
    cx.insert(Bar::new());
    cx.insert(Baz::new());
}

async fn foo(mut builder: CreateDocumentBuilder) {
    // Override the context values and make the request.
    let res = builder.extend(overrides()).await;
}

We can easily add this, but we didn't as part of the initial design because we weren't sure whether this addressed any practical customer needs.

What happens if we call into_future twice?

IntoFuture::into_future takes self by-value, meaning if we attempt to call it twice we get a compile error:

error[E0382]: use of moved value: `foo`
  --> src/main.rs:4:5
   |
2  |     let foo = Foo {};
   |         --- move occurs because `foo` has type `Foo`, which does not implement the `Copy` trait
3  |     foo.into_future();
   |         ------------- `foo` moved due to this method call
4  |     foo.into_future();
   |     ^^^ value used here after move

There is a related question here: "What if we want to construct a builder once, and repeatedly use that to create requests?". Since the builder pattern in Rust generally doesn't have any runtime overhead, simply moving the constructor to a function would suffice. But in addition to that it's good practice to implement Clone on all types that can support it, which would enable customers to write the following:

e.clone().into_future().await?;
e.into_future().await?;

What happens if we call .await twice?

.await in Rust takes futures by-value, and calling .await twice on a future results in a compile error:

error[E0382]: use of moved value: `fut`
 --> src/lib.rs:5:5
  |
3 |     let mut fut = foo.into_future();
  |         ------- move occurs because `fut` has type `std::future::Ready<()>`, which does not implement the `Copy` trait
4 |     fut.await;
  |     --------- value moved here
5 |     fut.await;
  |     ^^^ value used here after move

Generally I prefer not to implement Clone on futures directly, since let fut = async {}; returns impl Future and doesn't implement Clone. That keeps some degree of consistency between both manually authored and async {}-generated futures.

error[E0599]: no method named `clone` found for opaque type `impl Future` in the current scope
 --> src/main.rs:3:9
  |
3 |     fut.clone();
  |         ^^^^^ method not found in `impl Future`

Footnotes

1. Method names are subject to change, but that's a separate conversation. ↩

[JeffreyRichter]

I read all of this, I'd like just focus on the builder pattern aspect of this for now; we can address the context parts later. But, I will just say that contexts are supposed to be immutable and so the discussion on how to mutate them seems not appliable.

I do see what you're saying and I looked at the fs crate example you linked to. I also saw that once you get a file, there are a bunch of plain methods on file, see https://doc.rust-lang.org/std/io/trait.Write.html.

So, I think we can safely say that plain methods and builder pattern methods are both idiomatic to Rust.

What benefit is there for using the builder pattern proposed here as opposed to:
client.Method(mandatoryArg1, mandatoryArg2, *optionalArgs) where optionalArgs is constructed via a builder pattern?

All the other Azure SDKs use the line above so Rust needs to have a great reason to deviate from this.

[cataggar]

What benefit is there for using the builder pattern proposed here as opposed to:
client.Method(mandatoryArg1, mandatoryArg2, *optionalArgs) where optionalArgs is constructed via a builder pattern?

A concrete example of passing in optional arguments, using the azure_svc_batch example is:

azure-sdk-for-rust/services/svc/batch/examples/list_pools.rs

Line 24 in 7fd4c30

let pools = pool::list(config, None, None, None, None, None, None, None, None).await?;

When code is generated based off of this & #520, it will look like:

let pools = batch_client.pool().list().into_future().await?;

And even better when into_future() is no longer necessary:

let pools = batch_client.pool().list().await?;

I think that is a better user experience.

[JeffreyRichter]

I'm having trouble mapping the pool example to what I'm seeing to what I expect to see.
We've been talking about issue service operations but the example you link to is about client creation.

Creating a client requires 2 things: an endpoint and credentials and an optional 3rd thing: options.
So, maybe this is a good pattern direction for client creation:

 let client = &SomeClient::new(endpoint, credential)  // "new" up the client, optionally set options, and then build
        .optionA(...)   // If customer desires
        .optionB(...)   // If customer desires
        .build();

NOTE: Once built, the client should be immutable.

And maybe this is a good pattern for client operations:

 let operation = &client.SomeOperation(mandatory args)  // news up the "operation" with mandatory arguments, etc.
        .optionA(...)   // If customer desires
        .optionB(...)   // If customer desires
        .await?;

I thnk I can get on board with these patterns

[heaths]

A concrete example of passing in optional arguments, using the azure_svc_batch example is:

I think what @JeffreyRichter is getting it is something more like:

let opts = PoolListOptionsBuilder::new()
  .foo("foo")
  .bar(1)
  .build();
let result = client.pool().list(&opts).into_future().await?;

This is similar to what .NET and Java do, while JavaScript just uses bags e.g., {foo: "foo", bar: 1} and Python uses kwargs e.g., foo="foo", bar=1. Without overloads or default parameters (like C#), though, at the very least someone would need to pass None to an Option<PoolListOptionsBuilder> parameter which seems rather tedious.

The client should definitely be immutable and, given that, wouldn't that make the method return some builder (in concept) to which you can either set optional parameters or call its final invocation - which would be into_future()?

[MindFlavor]

I think what @JeffreyRichter is getting it is something more like:

let opts = PoolListOptionsBuilder::new()
  .foo("foo")
  .bar(1)
  .build();
let result = client.pool().list(&opts).into_future().await?;

This is similar to what .NET and Java do, while JavaScript just uses bags e.g., {foo: "foo", bar: 1} and Python uses kwargs e.g., foo="foo", bar=1. Without overloads or default parameters (like C#), though, at the very least someone would need to pass None to an Option<PoolListOptionsBuilder> parameter which seems rather tedious.

The client should definitely be immutable and, given that, wouldn't that make the method return some builder (in concept) to which you can either set optional parameters or call its final invocation - which would be into_future()?

Yes I think you underlined the important parts. The builder pattern still has a PoolListOptions, but it's "internal" in the sense an SDK user won't instantiate it directly, they receive it from the list() call. This removes the need to pass None manually. See:

// before
let opts = PoolListOptionsBuilder::new()
  .foo("foo")
  .bar(1)
  .build();
let result = client.pool().list(&opts)
  .into_future()
  .await?;

// after
let result = client.pool().list() // <-- this will return a `PoolListOptionsBuilder`!
  .foo("foo")
  .bar(1)
  .await?

I want to add another important reason to support the proposed approach: currying¹.

There is no need to "consume" (ie await) the operation right away. Since we are building the SDK with owned structs only (as opposed as I erroneously did before by using references everywhere) this is perfectly fine (handrolled code just to give you an idea):

fn list_pool_with_one_bar(client: Client) -> PoolListOptionsBuilder {
  client.pool().list()
    .bar(1)
}

fn main() {
   let client = new Client(/* <snip> */)?;
   let result = list_pool_with_one_bar(client)
    .foo("foo")
    .await?; // <- we can still override any option here if we want! 
}

In other words the builder pattern allows our SDK to support currying. This is another big win compared to the client.Method(mandatoryArg1, mandatoryArg2, *optionalArgs) approach. This technique is very common in functional languages (which Rust partially is), not so much in imperative ones (ie for example how Go treats errors effects make this impossibile).

Footnotes

1. https://en.wikipedia.org/wiki/Currying ↩

[JeffreyRichter]

This is a great conversation to have because we are coming up with a pattern that we will have to use everywhere. So, our decision here is of very significant importance. I'd prefer to couch this in generic pattern terms which would eventually become a formal guideline that all Rust client libraries MUST follow. So, we also have to be sure that this will work everywhere.

Once a customer has a client to a service, they invoke a service operation using a pattern like this:

let operation_result = &client.Operation(mandatory args) // news up the "operation" with mandatory arguments, etc.
   .optionA(...) // If customer desires
   .optionB(...) // If customer desires
   .await?;

We can debate later if Context is a mandatory arg or not.
I think I can be OK with this as a pattern guideline. Is everyone else OK with this?
Also, naming is very important. I do not think that the "Operation" method name should contain "Options" and I'm less sure about whether "Builder" should be there. I lean towards not because every Client method would end with Builder and this is just needless repetition. But, if ending builders with "Builder" is idiomatic, then we can add it. But I think the file stream examples refererrd to early in this PR did not have a "Builder" suffix suggesting that this is not idiomatic.

[bmc-msft]

I find the model of Operation(manditory, manditory).optionA(...).optionB(...).await? style pattern to be ergonomic and would be happy to see this be the default.

[cataggar]

I'm trying to figure out the best way to pass parameters in #520 #527 and as_ref().to_owned() may be an anti-pattern. Should impl Into<String> be used instead. Could the Builders avoid some cloning with Cow?

[yoshuawuyts]

Should impl Into<String> be used instead.

I think your suggestion to use impl Into<String> is a good one; it evades an unnecessary allocation when an owned string is passed directly. This should make for an easy improvement!

---

Could the Builders avoid some cloning with Cow?

Potentially we could; for example using impl Into<Cow<'static, str>> as the API bound could potentially save us from having to perform an allocation when passing string slices as well. But I think if we go down this path we should have guidelines in place. As we know from experience it's really easy to accidentally push lifetimes into customer-facing APIs, which ends up having real consequences for user experience. And Cow can make it easy to fall into that trap.

In general I'd prefer it if we took a gradual approach to this. The change to using Into<String> over AsRef<str> should be an easy one we can do straight away. And once we're comfortable with the general shape of the APIs, we can look at optimizing things further by seeing if we can make the switch over to the slightly more complicated Into<Cow<'static, str>>. How does that sound?

[yoshuawuyts]

Found a few gotchas in our implementation. Should be easy to fix!

[yoshuawuyts]

This type probably should be pub:

Suggested change

	type CreateDatabase = futures::future::BoxFuture<'static, crate::Result<CreateDatabaseResponse>>;
	pub type CreateDatabase = futures::future::BoxFuture<'static, crate::Result<CreateDatabaseResponse>>;

[yoshuawuyts]

For the context method to work in a chaining fashion, this type should take self by-reference:

Suggested change

	pub fn into_future(self) -> CreateDatabase {
	pub fn into_future(&self) -> CreateDatabase {

This may require additional calls to clone in the body of Box::pin.

[yoshuawuyts]

I think I can be OK with this as a pattern guideline. Is everyone else OK with this?

Yes, I'm on board with this. From reading the comments it sounds like we should have consensus on this?

---

Also, naming is very important. I do not think that the "Operation" method name should contain "Options" and I'm less sure about whether "Builder" should be there [...]

I believe we should have consensus that the method name should be Client::operation and not Client::operation_builder or Client::operation_options. But to avoid any confusion here's an annotated example:

Usage Example

This is the same example as used previously, but featuring example types instead of placeholders.

let res = some_client
    .operation("mandatory arg1", "mandatory arg2")
    .option_a(true) // If customer desires
    .option_b(true) // If customer desires
    .await?;

Types Overview

• Client::operation: method on Client to perform a specific operation, takes required parameters.
• OperationBuilder: the builder type returned by Client::operation. Contains builder methods to set optional parameters, and implements IntoFuture so it can be awaited to get the Operation future.
• Operation: a Future which when .awaited resolves into a Result of OperationResponse.
• OperationResponse: the result of a successful operation.

Annotated Interface Overview

/// Our crate's error type.
struct Error;
/// A specialized `Result` alias for our crate.
type Result<T> = std::result::Result<T, Error>;

/// An SDK client. For example `CosmosClient`.
/// For brevity we're omitting all constructor methods for the client.
pub struct SomeClient;
impl SomeClient {
    /// Perform an operation using the SDK client.
    // In actual implementations we should probably take a variation on
    // `A: Into<String>` rather than `&str` directly.
    pub fn operation(&self, arg1: &str, arg2: &str) -> OperationBuilder;
}

/// Construct an operation.
pub struct OperationBuilder;
impl OperationBuilder {
    /// Internal constructor function. Takes `Client` by-value.
    fn new(client: Client, option_a: bool, option_b: bool) -> Self;

    /// Set the value of `option_a`. This defaults to `false.`
    pub fn option_a(&mut self, option_a: bool) -> &mut Self;

    /// Set the value of `option_b`. This defaults to `false.`
    pub fn option_b(&mut self, option_b: bool) -> &mut Self;

    /// Finish constructing the builder and submit the request.
    // Before `IntoFuture` stabilizes we should use
    // an inherent method that needs to manually be called.
    pub fn into_future(&self) -> Operation;
}

// When `Intofuture` stabilizes we should use the trait instead
// which will automatically be called as part of `.await` desugaring.
impl IntoFuture for OperationBuilder {
    type Future = Operation;
    type Output = crate::Result<OperationResponse>;
    fn into_future(self) -> Self::Future;
}

// `IntoFuture` will also need to be implemented for references, so that
// we can call `.await` directly at the end of our builders. This is
// because our builders return `&mut Self` rather than `Self`, which means
// needing to implement the trait twice.
impl IntoFuture for &OperationBuilder {
    type Future = Operation;
    type Output = crate::Result<OperationResponse>;
    fn into_future(self) -> Self::Future;
}

/// The future returned from `OperationBuilder`.
// We use a type alias rather than a concrete type so that we don't have to bother
// with manually forwarding `Future` impls. Once "Type Alias Impl Trait" lands in the
// language, we should be able to do away with the `Box` wrapper and use
// `impl Future` instead.
pub type Operation = Pin<Box<dyn Future<Output = crate::Result<OperationResponse> + Send + 'static>>;

/// The value returned by a successful operation.
pub struct OperationResponse;

[JeffreyRichter]

This looks really good to me! It's also critical that a customer be able to examine the state of the builder when debugging sand possibly at runtime. I saw a loop earlier that showed how to see state at runtime. I assume a debugger will also show the state, right?

[yoshuawuyts]

This looks really good to me!

Yay, I'm glad!

It's also critical that a customer be able to examine the state of the builder when debugging sand possibly at runtime.

Yes, strong agree. For brevity I omitted derives, but we should indeed ensure we derive Debug on all types so values can be inspected at runtime. And debuggers should already be able to introspect the state of the builders without issues.

[yoshuawuyts]

It seems we're all in agreement this design is the way forward. #527 was merged two weeks ago, and we should make sure that our other SDKs follow along. Pressing merge!

[thovoll]

This means #290 should be updated on how to migrate a crate to the (updated) new architecture.