Skip to content
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

Replace FluentBuilders with FluentTypedFuture #1086

Closed
2 tasks done
jinyoung-cs opened this issue Mar 4, 2024 · 4 comments
Closed
2 tasks done

Replace FluentBuilders with FluentTypedFuture #1086

jinyoung-cs opened this issue Mar 4, 2024 · 4 comments
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@jinyoung-cs
Copy link

jinyoung-cs commented Mar 4, 2024

Describe the feature

FluentBuilders are a mechanism used to construct requests prior to being sent. FluentBuilders have two downfalls:

  • Request construction is fallible and is represented by ConstructionFailure
  • Request must be send which is basically just boilerplate.

Use Case

Replacing FluentBuilders with compile-time type checks guarentee that required fields are fulfill upon request. We can await these FluentTypedFutures directly instead of using send. It is basically user ergonomics and safety.

Proposed Solution

I'll use aws_sdk_s3::Client::create_bucket for example. If the create-bucket operation does not have bucket set, construction will fail at runtime.

There is a popular alternative to fallible builder pattern called typed builders (See: https://crates.io/crates/typed-builder). We can remove an entire class of runtime errors with such a pattern.

Create bucket builder may look something like this:

struct Invalid;

struct CreateBucketFuture<Bucket> {
    // required
    bucket: Bucket,
    // non-required
    acl: Option<BucketCannedAcl>,
    ...
}

impl CreateBucketFuture<Bucket> {
    fn bucket(self, bucket: impl Into<String>) -> CreateBucketFuture<String> {
        let CreateBucketFuture { acl, .. } = self;
        CreateBucketFuture {
            bucket: bucket.into(),
            acl,
            ...
        }
    }

    ...
}

// note it is only implemented for `String` not `Invalid`
impl IntoFuture for CreateBucketFuture<String> {
    type Output = Result<CreateBucketOutput, SdkErrorNoConstructionError<CreateBucketError, HttpResponse>>;
    // impl Trait in type alias requires https://github.com/rust-lang/rust/issues/63063, but can be a drop in later, without breaking*
    type IntoFuture = Pin<Box<dyn Future<Output = Self::Output>>>;

    fn into_future(self) -> Self::IntoFuture {
        Box::pin(async { self.send() })
    }
}

impl Client {
    fn create_bucket(&self) -> CreateBucketFuture<Invalid> { // we could type alias here as `Invalid` might be jarring in docs
        CreateBucketFuture {
            ...
        }
    }
}

async fn test(client: Client) {
    client.create_bucket()
        .bucket("123") // guarenteed to construct
        .await // send it off without the `send`
}

playground

Other Information

There are no major issues with the future aspect of this change. It can be implemented today for existing builder types with backwards and forward compatibility.

There are three major issues with the typed aspect of this change:

  1. This cannot directly replace FluentBuilders as existing downstream consumers who already have incorrect construction will encounter compile errors; therefore, this is a breaking change. This would especially be a problem for crates which might be exercising ConstructionFailures in tests, though those tests could be removed entirely.
  2. Although FluentBuilders adopt an owned builder pattern, taking in self as opposed to &mut self, FluentTypedFuture would be more difficult to use in branching code. This is particularly true in branching code with multiple required fields e.g. aws_sdk_s3::Client::copy_object (bucket, copy_source, and key are required)
// bikeshedding alternative name with _foo
let request = client.copy_object_foo();
let set_bucket_first: bool = todo!();
let bucket_id: String = todo!();
let should_copy_source: bool = todo!();
let key: String = todo!();

// ! Compile error, the type returned by the first branch is different than the type returned by the second branch
// When a user sets required fields, they are changing the underlying type. Type mutations / field setting must be uniform across branches
let request = if set_bucket_first {
    request.bucket(bucket_id)
} else {
    request
};

let request = request
    .copy_source(should_copy_source)
    .key(key);

// Same issue here
let request = if !set_bucket_first {
    request
} else {
    request.bucket(bucket_id)
};

The counterargument to this point is that these are owned builder patterns, and the fields should be prepared beforehand. This is generally true in practice as well, but nevertheless, it is a breaking behavioral change.

  1. Compile times would increase. I have heard EC2 compile times are already egregious, and typed builders could theoretically create a unique type, per sum of k combinations of required fields, per operation.
  • copy_object for example could have 23 = 8 builders. (Sum of every combination of required fields is the sum of binomial coefficients, i.e. Pascal's Triangle = 2required fields). This is the pathological case.
  • In practice this would be closer to n where n represents the number of required fields.
  1. The source code in the generated clients would get much larger. Doc comments would need some massaging to look right without having Invalid misguiding users.

It's hard to say how much this would impact binary and compile times, but thought it worth mentioning.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

A note for the community

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue, please leave a comment
@jinyoung-cs jinyoung-cs added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 4, 2024
@jinyoung-cs
Copy link
Author

oops just realized this is kind of a duplicate of something I already posted here: #1027

@rcoh
Copy link
Contributor

rcoh commented Mar 5, 2024

Yeah I don't think the typed builder pattern is realistic. I think the most realistic option there is going at it from a custom linting perspective.

The IntoFuture implementation, though, I think is a reasonable thing to build.

@mchoicpe-amazon
Copy link
Contributor

I see. I thought it worth mentioning, but yeah. I'll close this out since #1027 already captures the future implementation.

@jinyoung-cs jinyoung-cs closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2024
Copy link

github-actions bot commented Mar 5, 2024

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

3 participants