Open garypen opened 2 years ago
I don't really understand what you mean with try_layer
. I see two solutions here:
bool
instead of `Result<(), _>CorsBuilder
that has fallible finishing functions like fn layer(self) -> Result<CorsLayer, _>
and fn service(self) -> Result<Cors, _>
Returning Result
makes most sense when you expect users to handle the errors somehow. ensure_usable_cors_rules
checks for invalid configurations which I'm not sure how you'd handle. Maybe you can elaborate on your use case and how you expect to handle the errors?
Something like try_layer
wont work since tower::Layer
trait doesn't have such a method. It also wouldn't be compatible with tower::ServiceBuilder
unless we make fallible as well, which is a big change. If we do decide to support this I say we provide a separate builder.
I'll try to clarify why this is potentially useful:
@jplatte What I mean bytry_layer()
is to avoid changing the existing layer()
interface, to help maintain backwards compatibility for existing API consumers. There may be better ways to achieve this, but that's the motivation.
@davidpdrsn The nice thing about returning a Result
here is that I like the existing error messages and so, I want to maintain those (hence, bool
not as attractive) and in my use-case I can handle the errors. In our product, we maintain a configuration state machine. Before we transition to the new configuration, ensure that the incoming configuration is correct. If not, we reject the proposed configuration (with helpful error messages) and continue with the current configuration. Here's a link to a PR on our product which may be helpful if you want more detail: https://github.com/apollographql/router/pull/1197
I could have a go at trying to implement something for a draft PR if this is something that you think would be useful in tower-http
. If you think it's not generally applicable, then I can live with the solution I've put together in the PR above.
Alright, I guess Result
makes sense if you want to pass on the error message. I could totally see having .validate()
on the layer and service, which returns Result<(), CorsConfigurationError>
, with the existing ensure_usable_cors_rules
calls being replaced by something like
if let Err(e) = self.validate() {
panic!("{}", e);
}
I think I'm gonna experiment with some sort of builder that doesn't panic.
Feature Request
Motivation
We are using tower-http in our project and we allow users to configure CORS. If there is a mistake which we can't detect in the composition of those rules, then this can result in a runtime panic in the
ensure_usable_cors_rules()
fn. This is not a good experience and we are working around this at the moment by validating our CORS rules before we pass them into tower-http.Proposal
Could we modify
ensure_usable_cors_rules()
so that rather thanassert
ing rules are usable, returns a result and expose this function so that we can validate before attempting to create a CorsLayer?:Alternatives
Perhaps have a
try_layer()
on Cors (or CorsLayer?) which is a fallibleCorsLayer
creator? If we had this, we could keep the original assert basedensure_usable_cors_rules()
and add a new private fallible ensurer for consumption intry_layer
. I feel like this would be the cleanest solution.