Should we code-generate the Router type?

[david-perez]

Currently, the conversion from RuntimeError into http::Response is in the aws-smithy-http-server runtime crate:

https://github.com/awslabs/smithy-rs/blob/430f4bf8b49f2b1827d9c1657a6d0a5096cad978/rust-runtime/aws-smithy-http-server/src/runtime_error.rs#L59-L97

This conversion is used both by the Router (that converts RuntimeError::UnknownOperation into an 404 HTTP response), and in the code-generated operation handlers' from_request functions (example below):

crate::operation_deser::parse_string_payload_request(req)
    .await
    .map(StringPayloadOperationInputWrapper)
    .map_err(|err| aws_smithy_http_server::runtime_error::RuntimeError {
        protocol: aws_smithy_http_server::protocols::Protocol::RestJson1,
        kind: err.into(),
    })

However, the conversion is protocol-specific: that is, each protocol converts the runtime error into an HTTP response in a different way. This means the conversion cannot live in the runtime: we want to support loading RustCodegenDecorators from the classpath that add support for protocols defined outside the smithy-rs project.

Say we code-generate a protocol-specific function fn runtime_error_to_response(err: RuntimeError) -> http::Response<BoxBody>. There is no problem in having an operation handler's from_request use said function to return the HTTP response. However, the Router is a runtime component, not a code-generated one. In order for it to call the function we would have to pass it to the constructor (which is called by the code-generated impl From<OperationRegistry> for Router):

#[derive(Debug)]
pub struct Router<F, B = Body> {
    routes: Routes<B>,
    runtime_error_fn: F,
}

(note that F does not have a default and needs to go first in the list of generic type parameters). This is a breaking change, users would need to at least type Router<_> in their code.

---

The act of routing the incoming HTTP request itself to an operation is also protocol-specific: each protocol defines its own routing rules. We currently match over all externally-supported protocols, when we only always exercise one branch. So this is another place where we need to be able to inject protocol-specific routing logic into the Router when constructing it, by passing in another closure.

It seems to me like after these breaking changes, there isn't much protocol-agnostic stuff in the Router type. This begs the question: should we entirely code-generate a protocol and service-specific Router in the generated sSDK?

[david-perez]

Note that passing a trait object for the runtime error into response conversion closure:

type RuntimeErrorFn = Box<dyn Fn(RuntimeError) -> http::Response<BoxBody>>;

pub struct Router<B = Body> {
    routes: Routes<B>,
    runtime_error_fn: RuntimeErrorFn,
}

Would not break user code. However, we need RuntimeErrorFn to be Clone, which requires Sized, so it can't be made into a trait object. RuntimeErrorFn needs Clone for Router to be Clone, which is a requirement for IntoMakeService to work:

---- src/operation_registry.rs - operation_registry::OperationRegistry (line 29) stdout ----
error[E0277]: the trait bound `Router: Clone` is not satisfied
  --> src/operation_registry.rs:49:50
   |
22 |    let server = hyper::Server::bind(&bind).serve(app.into_make_service());
   |                                            ----- ^^^^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `Router`
   |                                            |
   |                                            required by a bound introduced by this call
   |
   = note: required because of the requirements on the impl of `for<'a> Service<&'a AddrStream>` for `IntoMakeService<Router>`
   = note: required because of the requirements on the impl of `service::make::MakeServiceRef<AddrStream, Body>` for `IntoMakeService<Router>`

This failing idea is implemented in this davidpz/make-runtime-error-into-response-protocol-agnostic-via-trait-object branch.

[drganjoo]

1. Do we want to use class path based RustCodegenDecorators for version 1.0 or later? and are these known protocols that we are talking about or unknown protocols? If we are talking about unknown protocols, would it be sufficient to just move the router out or do we need to move the request / response into code generated part as well?

2. Are we going to convert the existing code generator into a class path loaded RustCodegenDecorator as well or there is no change in that?

3. By code generating the router, at a later stage, we can also try a trie like structure that is optimised for the URLs that are present in the smithy file. I am not sure how much of a speed gain, if any, that would give us but it might be worth trying out.

4. If we code generate router based on the protocol, can we simplify RuntimeError by removing pub protocol: Protocol from it, and all the match statements that we have inside into_response as well? If yes, that can definitely speed us up.

5. Does this affect how we generate logging commands inside the router? Will the customer be passing an argument at code generation time to have logging commands generated / enabled?

6. Will this scheme help us in skipping the logging commands inside a route if it has sensitive trait on it?

7. In the current scheme, any bug fix and enhancement to the router can easily be rolled out by pushing out a new version of the runtime. But in this new scheme, the customer will have to rerun the code generator to generate the new crate. Let's discuss the recommended deployment strategy in both cases and how future versions are to be rolled out under both.

[david-perez]

Do we want to use class path based RustCodegenDecorators for version 1.0 or later?

We use them now, it's something that we already do.

and are these known protocols that we are talking about or unknown protocols?

Unknown protocols.

If we are talking about unknown protocols, would it be sufficient to just move the router out or do we need to move the request / response into code generated part as well?

Code-generating the Router would be sufficient.

Are we going to convert the existing code generator into a class path loaded RustCodegenDecorator as well or there is no change in that?

If by existing code generator, you mean either the rust-codegen or rust-server-codegen code generators, these are not decorators, but Smithy plugins. They are the entrypoint to code generation.

By code generating the router, at a later stage, we can also try a trie like structure that is optimised for the URLs that are present in the smithy file. I am not sure how much of a speed gain, if any, that would give us but it might be worth trying out.

I've already thought about this. I think performance improvements would only start being noticeable in services with a huge (100?) number of routes. However, the main problem with trie-based routing is that we can't use something off-the-shelf like matchit because of Smithy's routing requirements. It would be very challenging to implement, say, routing based on query string literals using a trie-like data structure, because they can appear anywhere in the request URI after the path.

If we code generate router based on the protocol, can we simplify RuntimeError by removing pub protocol: Protocol from it, and all the match statements that we have inside into_response as well? If yes, that can definitely speed us up.

Yes, the protocol field would go away.

Does this affect how we generate logging commands inside the router? Will the customer be passing an argument at code generation time to have logging commands generated / enabled?

This is unrelated to the fact of whether we code-generate the Router or not. Either way, users should be able to enable logging on their service by registering a layer.

Will this scheme help us in skipping the logging commands inside a route if it has sensitive trait on it?

Yes. At codegen time we have this information in the model and we can thus filter out the logging statements in any component that is code-generated.

In the current scheme, any bug fix and enhancement to the router can easily be rolled out by pushing out a new version of the runtime. But in this new scheme, the customer will have to rerun the code generator to generate the new crate. Let's discuss the recommended deployment strategy in both cases and how future versions are to be rolled out under both.

The assumption is that every time the user deploys, they are running the code generator.

[hlbarber]

Closed by #1666