Builder ergonomics

[CBenoit]

Hi! 👋

First, let me thank you for the great crate. It was a breeze to add initial support for ngrok in our product, Devolutions Gateway.

However, I noticed all the builders are "consuming". This means, we can’t write straightforward code such as:

let mut builder = Session::builder()
    .authtoken(…)
    .tcp_endpoint()
    .remote_addr(…)
    .forwards_to(…);

if let Some(metadata) = metadata {
    builder.metadata(metadata);
}

for deny_cidr in deny_cidrs {
    builder.deny_cidr(deny_cidr);
}

let tunnel = builder.listen().await?;

This is really important when the configuration must be dynamic. See how I had to resort to macro tricks to get something somewhat "clean": Devolutions/devolutions-gateway#442

Here is an excerpt:

macro_rules! builder_call_opt {
    ($builder:ident . $method:ident ( $ngrok_option:expr ) ) => {{
        if let Some(option) = $ngrok_option {
            $builder.$method(option)
        } else {
            $builder
        }
    }};
}

macro_rules! builder_call_vec {
    ($builder:ident . $method:ident ( $ngrok_option:expr ) ) => {{
        let mut builder = $builder;
        let mut iter = $ngrok_option;
        loop {
            builder = match iter.next() {
                Some(item) => builder.$method(item),
                None => break builder,
            };
        }
    }};
    ($ngrok_option:expr, $builder:ident . $method:ident ( $( $( & )? $field:ident ),+ ) ) => {{
        let mut builder = $builder;
        let mut iter = $ngrok_option.iter();
        loop {
            builder = match iter.next() {
                Some(item) => builder.$method($( & item . $field ),+),
                None => break builder,
            };
        }
    }};
}

macro_rules! builder_call_flag {
    ($builder:ident . $method:ident ( $ngrok_option:expr ) ) => {{
        match $ngrok_option {
            Some(option) if option => $builder.$method(),
            _ => $builder,
        }
    }};
}

let builder = Session::builder().authtoken(&conf.authtoken);
let builder = builder_call_opt!(builder.heartbeat_interval(conf.heartbeat_interval));
let builder = builder_call_opt!(builder.heartbeat_tolerance(conf.heartbeat_tolerance));
let builder = builder_call_opt!(builder.metadata(&conf.metadata));
let builder = builder_call_opt!(builder.server_addr(&conf.server_addr));

let session = builder.connect().await?;

let builder = session.http_endpoint().domain(&http_conf.domain).forwards_to(hostname);
let builder = builder_call_opt!(builder.metadata(&http_conf.metadata));
let builder = builder_call_vec!(http_conf.basic_auth, builder.basic_auth(username, password));
let builder = builder_call_opt!(builder.circuit_breaker(http_conf.circuit_breaker));
let builder = builder_call_flag!(builder.compression(http_conf.compression));
let builder = builder_call_vec!(builder.allow_cidr(http_conf.allow_cidrs.iter()));
let builder = builder_call_vec!(builder.deny_cidr(http_conf.deny_cidrs.iter()));
let builder = builder_call_opt!(builder.proxy_proto(http_conf.proxy_proto.map(ProxyProto::from)));
let builder = builder_call_vec!(builder.scheme(
    http_conf
        .schemes
        .iter()
        .map(|s| Scheme::from_str(s).unwrap_or(Scheme::HTTPS))
));

let tunnel = builder.listen().await?;

This is especially bad for collections (basic_auth, allow_cidr, deny_cidr, …).

For reference, I had a similar situation in our IronRDP WASM module: SessionBuilder. In order to interface with JavaScript in a clean way, the builder methods must not take ownership of self. Using a RefCell internally is often the best solution. (An additional requirement, that ngrok-rs can ignore, was for the builder to be cheap to clone, hence the Rc.)

Rust official Style Guidelines book is also a good read: https://doc.rust-lang.org/1.0.0/style/ownership/builders.html

[jrobsonchase]

This was a consideration when initially designing the builders, so the mutating vs consuming builder question isn't a new one. I believe at some point, there was an ownership concern that required the final connect/listen call to consume the builder itself, so it couldn't be called on a chain of builder methods that take/return mutable references. That's not the case today, but I still feel like it's mostly a matter of preference, since builder = builder.some_option(); is nearly as ergonomic as builder.some_option();, and you don't have to pre-bind it before the initial "base" options.

In your example, if the builder methods took/returned mutable references:

// This can't work - builder is a reference that outlives the data it points to.
// let mut session_builder = Session::builder()
//     .authtoken(…);
// if let Some(session_meta) = session_metadata {
//     session_builder.session_metadata(session_meta);
// }
// 
// let endpoint_builder = session_builder
//     .connect()
//     .await?
//     .tcp_endpoint()
//     .remote_addr(…)
//     .forwards_to(…);

// Instead, it has to be:
let mut session_builder = Session::builder(); // initial bind
session_builder.authtoken(...); // initial options, separate statement

if let Some(session_metadata) = session_metadata {
    session_builder.metadata(session_metadata);
}

let mut endpoint_builder = builder
    .authtoken(…)
    .connect()
    .await?
    .tcp_endpoint(); // initial bind

endpoint_builder
    .remote_addr(…)
    .forwards_to(…); // initial options, another extra statement

if let Some(metadata) = metadata {
    endpoint_builder.metadata(metadata);
}

for deny_cidr in deny_cidrs {
    endpoint_builder.deny_cidr(deny_cidr);
}

let tunnel = endpoint_builder.listen().await?;

whereas with the owning-builder approach:

let mut session_builder = Session::builder()
    .authtoken(...); // options return the builder, so you can call them at bind-time
if let Some(session_metadata) = session_metadata {
    // But you do need to reassign it if you want to "mutate" your builder
    session_builder = session_builder.metadata(session_metadata);
}

let mut endpoint_builder = builder
    .authtoken(…)
    .connect()
    .await?
    .tcp_endpoint() // Again, no extra statement needed here - options are in-line
    .remote_addr(…)
    .forwards_to(…);

if let Some(metadata) = metadata {
    endpoint_builder = endpoint_builder.metadata(metadata);
}

for deny_cidr in deny_cidrs {
    endpoint_builder = endpoint_builder.deny_cidr(deny_cidr);
}

let tunnel = endpoint_builder.listen().await?;

I tend toward preferring expression-oriented/functional APIs, which I think is why the forced separation between binding and initial options for mutating builders has always felt kludgey to me. I'm willing to be convinced, but it's a pretty big (if a bit rote) change to make. I'll give you that the hoops you have to jump through to mutate it in another struct behind &mut are particularly bad:

struct Foo {
    builder: Option<ngrok::config::TcpTunnelBuilder>,
}

impl Foo {
    fn set_meta(&mut self, meta: impl Into<String>) {
        self.builder = self.builder.take().map(|builder| builder.metadata(meta));
    }
}

Your macros are solving a different problem, or rather a couple of different problems:

1. Mapping your configuration structures to builder calls
   Some of this we could probably handle better, like offering methods that accept iterators for methods that are likely to be called on lists of things. The Go API is kinda nice here with the args ...Type syntax, which isn't as "pretty" in rust. Others, like the Optional calls feel like they're correct from the ngrok-rust standpoint. Builder methods accepting Option arguments seem redundant to me. They're already "optional" by virtue of being called or not called.
   An alternative to your macros (or at least their bodies) could be:

// Abuse the IntoIter implementation for Option as well
builder = thing
    .into_iter()
    .fold(builder, |builder, value| builder.method(value));

2. (I suspect) dealing with builders with methods that look the same, but are still different concrete types and not abstracted using traits.
   Since you only have an HTTP example here, I can't be sure if this is actually a concern of yours, but it's a pet peeve of mine. Like all of the endpoint builders have the allow/deny_cidr methods, but you can't pass any old builder to a generic apply_ip_restrictions function and let it call the allow/deny methods. This feels like the biggest ergonomics problem to me.

[CBenoit]

You are bringing up good points. By declaring a mutable binding and using fold I’m actually able to get a decent result albeit with a lot of builder = assignments. I admit I would prefer not having to re-assign each time, but as you said, it’s likely a matter of preference and I happen to be willing to be convinced as well, and will not try to convince you instead here 😄 (I actually do like expression-oriented/functional code myself, but somehow didn’t feel the same for the builder pattern, probably because of the numerous builder =).

(I suspect) dealing with builders with methods that look the same, but are still different concrete types and not abstracted using traits.
Since you only have an HTTP example here, I can't be sure if this is actually a concern of yours, but it's a pet peeve of mine. Like all of the endpoint builders have the allow/deny_cidr methods, but you can't pass any old builder to a generic apply_ip_restrictions function and let it call the allow/deny methods. This feels like the biggest ergonomics problem to me.

We actually support both TCP and HTTP tunnels in the original code but simplified the example in my original post:
https://github.com/Devolutions/devolutions-gateway/blob/08520cdbbb8e46732ef2836cd780edbfc4ca0bd2/devolutions-gateway/src/ngrok.rs#L84
That being said, I’m kind of okay with duplicating some parts, especially when it’s "looking the same", but "actually different". I guess this could be improved in the case of allow/deny_cidr. If API author is allowing me to abstract over it (because it’s actually okay), I would definitely take that route though. Looking forward to it!

Feel free to close the issue, or otherwise use it to track this potential change.

[jrobsonchase]

Good news - changed my mind on this one! Realized that owning builders are really bad when you want to have fallible builder methods, since there's no hope of recovery without plumbing the builder back through every Err result.

So now, all of the builders are mutable and some of the errors have been moved from connect-time up to configure-time 🎉

[CBenoit]

Thank you for the update!

So now, all of the builders are mutable and some of the errors have been moved from connect-time up to configure-time 🎉

That’s actually nice! 😄