Skip to content
This repository has been archived by the owner on Sep 13, 2018. It is now read-only.

Just Middleware. #21

Closed

Conversation

withoutboats
Copy link

A middleware is a function from Service to Service.

Two combinators are provided:

  • Service::wrap (equivalent to function application)
  • Middleware::chain (equivalent to function composition)

A middleware is a function from Service to Service.

Two combinators are provided:

* Service::wrap (equivalent to function application)
* Middleware::chain (equivalent to function composition)
@withoutboats withoutboats changed the title Low level definition of Middleware. Just Middleware. Apr 18, 2017
@withoutboats
Copy link
Author

This branch doesn't have any of the streaming services stuff, just the middleware definition.

This was referenced Apr 18, 2017
@@ -155,6 +99,14 @@ pub trait Service {

/// Process the request and return the response asynchronously.
fn call(&self, req: Self::Request) -> Self::Future;

/// Wrap this Service in a Middleware component.
fn wrap<M>(self, middleware: M) -> M::WrappedService where
Copy link

Choose a reason for hiding this comment

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

Technically speaking the middleware wraps the service, but the service doesn't wrap the middleware, so I guess that the method should be named wrapped_by(). This is slightly confusing, because the middleware also has a method named wrap().

I don't have a perfect alternative name, but it might here are some ideas: apply() , filter() , using() , with() wrapped()

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK w/ the name wrap here.

Copy link
Author

Choose a reason for hiding this comment

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

I think wrap works in both directions, in the sense of wrap in {middleware} and wrap around {service}. But I'm also fine with whatever name.

///
/// This allows you to build middleware chains before knowing
/// exactly which service that chain applies to.
fn chain<M>(self, middleware: M) -> MiddlewareChain<S, Self, M> where
Copy link

Choose a reason for hiding this comment

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

Alternative you could use a builder to create MiddlewareChain. That would allow MiddlewareChain to live in a separate crate.

let middleware = MiddlewareChainBuilder::new(middleware1)
    .chain(middleware2)
    .chain(middleware3);

let service = Myservice::new().wrap(middleware);

I wonder how often MiddlewareChain will actually be used. It might be easier to write a function that applies the necessary middleware to the service or to wrap each service independently. If MiddlewareChain to lives in a separate crate, then it is easier to experiment.

Copy link
Member

Choose a reason for hiding this comment

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

I kind of like the idea of keeping the "chain" concept off the Middleware trait... though I don't know if it should be in a separate crate or not.

Also, I think finagle calls this concept a "stack", but I am not super adept at reading scala...

Copy link
Author

Choose a reason for hiding this comment

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

So I have use for chain in the framework I'm writing. HTTP services are constructed through a DSL, and then users can in that DSL declare a middleware to be applied to them. It can be easier for them if they can chain middleware together to produce a the middleware they're going to be applying. They never have access to the Service type directly so they can't just use wrap.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Just left some bike shedding level comments. Overall, I think this is good.

I will leave the PR queued up until we figure out how best to update tokio-service, which we should hopefully discuss sooner than later.

pub trait Middleware<S: Service> {
/// The service produced by wrapping this middleware around another
/// service.
type WrappedService: Service;
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to bike shed, I think it could be nice to come up w/ a shorter name for this associated type... I would say Upstream, but up and down seem to get everyone confused...

Copy link
Author

Choose a reason for hiding this comment

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

We could just call it Wrapped?

///
/// This allows you to build middleware chains before knowing
/// exactly which service that chain applies to.
fn chain<M>(self, middleware: M) -> MiddlewareChain<S, Self, M> where
Copy link
Member

Choose a reason for hiding this comment

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

I kind of like the idea of keeping the "chain" concept off the Middleware trait... though I don't know if it should be in a separate crate or not.

Also, I think finagle calls this concept a "stack", but I am not super adept at reading scala...

@@ -155,6 +99,14 @@ pub trait Service {

/// Process the request and return the response asynchronously.
fn call(&self, req: Self::Request) -> Self::Future;

/// Wrap this Service in a Middleware component.
fn wrap<M>(self, middleware: M) -> M::WrappedService where
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK w/ the name wrap here.

@flosse
Copy link

flosse commented Nov 26, 2017

Hey guys, what's the state with that issue?

@carllerche
Copy link
Member

Dev is being moved to Tower.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants