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

Add configuration (for extractors and middleware) #5

Closed
aturon opened this issue Nov 8, 2018 · 13 comments · Fixed by #109
Closed

Add configuration (for extractors and middleware) #5

aturon opened this issue Nov 8, 2018 · 13 comments · Fixed by #109
Labels
design Open design question
Milestone

Comments

@aturon
Copy link
Collaborator

aturon commented Nov 8, 2018

Most extractors and middleware should be configurable. But that presents some challenges:

  • Extractors are given as argument types to endpoints, so there's no obvious value to attach configuration to.
  • Middleware configuration may want to be tweaked in route-specific ways.

One approach for solving these problems is to use a typemap for configuration, allowing extractors and middleware to provide their knobs as public types that can be installed into the map. This map would then be built as part of constructing the router:

let mut app = App::new();

// configure json extractors to use a 4k maximum payload by default
app.config(json::MaxPayload(4096));

// set up the `foo` endpoint and override its configuration
app.at("foo")
    .put(foo_endpoint)
    .config(json::MaxPayload(1024));

// for all routes starting at `bar`...
app.at("bar").nest(|router| {
    // configure the json extractor to return a 400 on parsing failure
    router.config(json::OnError(StatusCode::BAD_REQUEST));

    // ...
})

(Note: the nest example uses the feature proposed here).

Note that this functionality could be implemented using middleware and the extensions part of the request, but that would mean the configuration is constructed fresh during request handling, rather than when the app is constructed.

Let's reach consensus on a design, and then implement this!

@aturon aturon added the design Open design question label Nov 8, 2018
@bIgBV
Copy link
Contributor

bIgBV commented Nov 19, 2018

So as I'm working on #8 I figured I would add my own thoughts around this. When it configuration, different web frameworks have different ways of going about it. The two chief ways I can think of are:

  1. Parsing from a config file/environment
  2. Programatic control.

There are pros and cons to both of them, but I think the most flexible solution would be to have a set of defaults, which are overridden by values parsed from the environment and these can be further overridden by programatic configuration.

I think this is a good idea because for someone who just wants to set up a quick service, they will have sane defaults to work with, while setting up configuration to be read from a file/environment will help make deployments flexible. On top of that, we can provide specific control over individual routes as presented by @aturon .

@tirr-c
Copy link
Collaborator

tirr-c commented Nov 27, 2018

I've tried to use typemap, but cloneable one yields forward compatibility warning regarding object safety.

warning: the trait `typemap::internals::CloneAny` cannot be made into an object
  --> /home/tirr/.cargo/registry/src/github.com-1ecc6299db9ec823/typemap-0.3.3/src/internals.rs:33:5
   |
33 |     fn clone_any_send(&self) -> Box<CloneAny + Send> where Self: Send;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(where_clauses_object_safety)] on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
   = note: method `clone_any_send` references the `Self` type in where clauses

(...)

@bIgBV
Copy link
Contributor

bIgBV commented Nov 27, 2018

How about we just have a config struct with a few fields to start with and wrap it in an Arc to be shared across? My reasoning is that for the most part configuration is for configuring the framework, so the number of knobs that a user can turn are limited.

So we can have default values set up which are overridden with the parsed values from a config file/environment which can be further overridden programmatically using something like app.config.

Just a thought.

@bIgBV
Copy link
Contributor

bIgBV commented Nov 28, 2018

@tirr-c I experimented with the Extensions from the HTTP crate and it compiled without any issues. As an experiment I added it to the App struct and created a dummy config to be stored in it, and it worked.

I'm not sure how this typemap will be shared internally within Tide though.

@bIgBV
Copy link
Contributor

bIgBV commented Nov 30, 2018

I experimented with the Extensions type, but the issue I am facing is safely sharing it globally. I used lazy_static to store an instance of Extensions behind a RwLock, but the borrow checker is not happy about accessing the data in it.

@tirr-c
Copy link
Collaborator

tirr-c commented Dec 11, 2018

I have an idea that doesn't require global states or typemaps, but is somewhat less flexible -- using toml::Value (or serde_json::Value, or whatever) under the hood.

Configuration can be set and retrieved with typed configuration items (like the sample code above.) Those items should implement Serialize and Deserialize. Each item has its own unique name in &'static str and it will be used as the key for the configuration.

trait ConfigurationItem: Serialize + Deserialize + Default {
    const NAME: &'static str;
}

When new endpoint is established, configuration of the parent router will be cloned into the endpoint. This is easier to implement, but it makes configuration and resource declaration depend on their order, like nest and middleware. Alternatively we can lazily overlap parent and child configuration when parent router setup ends.

A positive side effect of this approach is that it makes implementing configuration files a trivial job, as we can use Serde for serializing and deserializing.

@aturon
Copy link
Collaborator Author

aturon commented Dec 11, 2018

@tirr-c Interesting thought! I'm a little worried though that it'd impose some overhead that should be avoidable.

Looking more closely at the warning for CloneAny, the problem seems to be the method-level where clauses, which I think are being used just to avoid having to duplicate the trait for various combinations of marker traits (Send, Sync) that might apply.

In other words, I bet that we could roll our own version of CloneAny that inherits from Send and Sync, and just have one method that doesn't run into these issues:

trait CloneAny: Send + Sync {
    fn clone_any(&self) -> Box<dyn CloneAny>;
}

impl Clone for Box<dyn CloneAny> {
    fn clone(&self) -> Self {
        self.clone_any()
    }
}

@tirr-c do you want to take another stab at using TypeMap<dyn CloneAny> with the above definition?

@bIgBV
Copy link
Contributor

bIgBV commented Dec 11, 2018

I'm a little curious about where this configuration typemap will live. I ask because I want to know how an endpoint implementation can access some data in this configuration. Similarly, how will any part of Tide access it?

My bad I did not see the comment on #100 , I agree having this API to tweak the configuration per endpoint is definitely more flexible. So if I'm understanding this right, it means that the configuration will be accessible on the App and then be copied over to any routers and sub routers?

@bIgBV
Copy link
Contributor

bIgBV commented Dec 11, 2018

So I was thinking about how to share configuration with middleware and was considering something along the lines of this:

The handle method of the Middleware trait can be passed a handle to this context as a parameter.

impl<Data, F> Middleware<Data> for F
where
    F: Send + Sync + Fn(RequestContext<Data>) -> FutureObj<Response>,
{
    fn handle<'a>(&'a self, ctx: RequestContext<'a, Data>, config: TypeMap<dyn CloneAny>) -> FutureObj<'a, Response> {
        (self)(ctx)
    }
}

Definitely a rough design, but what do you guys think?

@tirr-c
Copy link
Collaborator

tirr-c commented Dec 12, 2018

@bIgBV I think configuration can be wrapped in RequestContext. The context struct can provide some methods to retrieve configuration items then.

@aturon I've experimented with your trait design (with some tweaks) and it's promising! However I'm afraid we can't use existing TypeMap directly with the custom Any trait as it required implementing internal traits of typemap. I managed to write a new, simple typemap (this is a proof-of-concept): Playground

@bIgBV
Copy link
Contributor

bIgBV commented Dec 12, 2018

@tirr-c I was considering RequestContext as well, but isn't it constructed for every request? Will that mean we will pass the config from the App/Server down to the RequestContext?

@tirr-c tirr-c mentioned this issue Dec 14, 2018
8 tasks
@tirr-c
Copy link
Collaborator

tirr-c commented Dec 14, 2018

@bIgBV Well, configuration is per endpoint, so not exactly from the App; but yes, the config will be from the endpoints, passed by reference. I've opened PR #109 which implements the idea.

@yoshuawuyts
Copy link
Member

@tirr-c PR looks really good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Open design question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants