RFC: Service Builder Improvements #1620
Conversation
Co-authored-by: david-perez <d@vidp.dev>
| 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 */) | ||
| } |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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
Co-authored-by: david-perez <d@vidp.dev>
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
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):
- Type-safe state extraction.
- 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). - Exploration of the
ServiceBuilderExtidea 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Just to add some extra context/thoughts to these:
|
Rendered
Toy Implementation