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

RFC: Service Builder Improvements #1620

Merged
merged 28 commits into from
Aug 30, 2022
Merged

RFC: Service Builder Improvements #1620

merged 28 commits into from
Aug 30, 2022

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Aug 4, 2022

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far Background section looks good.

design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 5, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 5, 2022
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
Comment on lines +655 to +666
pub fn logging(self, /* args */) -> Operation0<S, Stack<L, LoggingLayer>> {
Operation0 {
inner: self.inner,
layer: Stack::new(self.layer, LoggingLayer::new(/* args */))
}
}

pub fn auth(self, /* args */) -> Operation0<S, Stack<L, AuthLayer>> {
Operation0 {
inner: self.inner,
layer: Stack::new(self.layer, /* Construct auth middleware */)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I hadn't thought of enabling logging/auth per-operation. It's good that we have the option to do so, but I'd just expose these conveniences in the ServiceBuilder to apply them service-wide. What matters is operation-specific arbitrary layers.

}

impl<F> Operation0<ServiceFn<F>, Identity> {
pub fn from_handler(inner: F) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the omission is justified by the sentence above that said you're not going to deal with the whole handlers with and without state issue (or answered by the item in approach B "- from_handler, which takes an async Operation0Input -> Operation0Output."), but: in both this approach and approach B, what are the bounds on F? Is the idea to still require F implement Handler, which we will implement in codegen like we do now, for functions with and without state?

Copy link
Contributor Author

@hlbarber hlbarber Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we should continue with Handler pattern except the bounds change now and the into_service on Handler should not convert directly to a Service<http::Request, Response = http::Response>.

The into_service here takes the Handler and does a smaller step to Service<{Operation}Input, Response = {Operation}Output>. This step is agnostic to the protocol and specifics of the operation.

https://github.com/hlbarber/service-builder/pull/1/files#diff-67196527c762aa94fd3d640b4495ff46ab8e9a0760f426b9314b0e00b3f0846b

The eventual conversion to from Service<{Operation}Input, Response = {Operation}Output> to Service<http::Request, Response = http::Response> is done later:
https://github.com/hlbarber/service-builder/pull/1/files#diff-3492898f38fbf939a6bd57aa646735db0293b43804edb28848ef95a88b77df77

design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
Copy link
Contributor

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting into this!

I favour approach C as well in terms of middleware constructors.

design/src/rfcs/rfc0020_service_builder.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Aug 19, 2022
@hlbarber hlbarber marked this pull request as ready for review August 19, 2022 13:32
@hlbarber hlbarber requested a review from a team as a code owner August 19, 2022 13:32
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy that we're doing this huge improvement.

Overall the document's content is very well written.

Consider running it through mdlint or prettier before merging to aid reading and maintaining the source. Consider hard-wrapping lines too.


I also take it that these remaining 3 ideas we discussed are being punted to another RFC (which is for the best, since this one is long):

  1. Type-safe state extraction.
  2. Allowing a build_unchecked() on the service builder for the case where the user does not want to specify a handler for all operations (think the case of a multi-Lambda service).
  3. Exploration of the ServiceBuilderExt idea to allow us and third-parties to vend curated stacks of middleware.

In fact, I don't think any of these warrant an RFC. Just the implementation PR, if they turn out to be possible, would be fine.

}
```

The customer will now get a compile time error rather than a runtime error when they fail to specify a handler.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking whether we could do the same thing to structure shape builders? Constraint trait validation still needs to happen at runtime, and it might be a lot of work to pass around the builders with the correct type params among the several deserializing functions, but in theory I don't see why we could not detect at compile-time whether the user set all the required fields... What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think required would definitely be a good candidate for compile time checking.

The decision is not as as clear cut as the service builder case though:

  • We already have generics on the service builder which allows us to type check.
  • "missing handler" is the entire failure criteria so we eliminate fallibility completely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about it, I think it's not worth introducing API asymmetry for required with respect to the other constraint traits. Maybe it'd be weird to have one constraint trait checked at compile time and the others at runtime. And modifying the deserializers to make them aware of when they've set a value would be a hassle.


Two downsides of this are:

- `RuntimeError` enumerates all possible errors across all existing protocols, so is larger than modelling the errors for a specific protocol.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big downside I overlooked when first implementing RuntimeError, but I went with it because I think all AWS protocol errors can be more or less massaged into one of the 5 variants, and the existing service framework works in a similar way. smithy-ts server SDK also went down a similar route.

But I always wondered if/when we support wildly different non-AWS protocols, whether that enum would just grow unwieldy. I'm glad we're getting rid of this.

design/src/rfcs/rfc0020_service_builder.md Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber
Copy link
Contributor Author

I also take it that these remaining 3 ideas we discussed are being punted to another RFC (which is for the best, since this one is long):

  1. Type-safe state extraction.
  2. Allowing a build_unchecked() on the service builder for the case where the user does not want to specify a handler for all operations (think the case of a multi-Lambda service).
  3. Exploration of the ServiceBuilderExt idea to allow us and third-parties to vend curated stacks of middleware.

In fact, I don't think any of these warrant an RFC. Just the implementation PR, if they turn out to be possible, would be fine.

Just to add some extra context/thoughts to these:

  1. We now have a great case study on how to do this via Add type safe state extractor tokio-rs/axum#1155. I think it's best that we complete the RFC implementation "extractor-less" and then put up a PR to add the type extractor machinery separately. The implementation seems straight forward, but there's a question of whether it's worth adding an extra set of generics for a marginal(?) improvement to ergonomics.
  2. I'm confident we can get something like this added.
  3. Similar to (1). In this case we might be brushing against the lack of GATs and other nightly features.

@hlbarber hlbarber merged commit bc9c9f3 into main Aug 30, 2022
@hlbarber hlbarber deleted the harryb/builder-rfc branch August 30, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants