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

Middleware usage #32

Open
kevin-sakemaer opened this issue Dec 9, 2019 · 20 comments
Open

Middleware usage #32

kevin-sakemaer opened this issue Dec 9, 2019 · 20 comments
Labels
enhancement New feature or request help wanted Extra attention is needed pkg:shelf_router_generator pkg:shelf_router Package shelf_router

Comments

@kevin-sakemaer
Copy link

Is there a way to use Middleware with this package?

And better to annotate a Route with some middleware we want to use for this route ?

@kevin-sakemaer kevin-sakemaer changed the title Middleware use Middleware usage Dec 9, 2019
@jonasfj jonasfj added pkg:shelf_router Package shelf_router enhancement New feature or request and removed pending-triage Issue is pending triage labels Dec 18, 2019
@jonasfj
Copy link
Member

jonasfj commented Dec 18, 2019

I think it's technically possible by simply return null from an .all( handler, example:

router.all('/<ignored|.*>', (Request request) {
  // Do something here....
  return null;
});

router.get(....);

Maybe we should create a router.middleware method, and a @Middleware annotation.
I'm not sure middleware is the best name, but I'm open to suggestions.

@kevin-sakemaer
Copy link
Author

router.all('/<ignored|.*>', (Request request) {
  // Do something here....
  return null;
});

router.get(....);

the callback on the router.all is only called before and not after the router.get. Which is not how Middleware work.
With Middleware we can hook some code before and/or after the innerHandler.

maybe we can add a named parameter to router.get/all/post/etc called middleware where we can pass a Middleware object.
and same with the annotation we can maybe pass a Middleware object.

@kevin-sakemaer
Copy link
Author

kevin-sakemaer commented Dec 18, 2019

maybe something like that ?

    router.get('/say-hi/<name>', (Request request, String name) {
      return Response.ok('hi $name');
    }, requestHandler: (req) {
      return req.change(context: {'logHandlerTime': Stopwatch()..start()});
    }, responseHandler: (res, req) {
      final sw = req.context['logHandlerTime'] as Stopwatch;
      print(sw.elapsed);
      return null;
    });

trying some API

this example raised a problem which is we can't reuse middleware across different route :/
Maybe we can have a custom Middleware (or with an other name) class to wrap this two functions with some shared context.

@jonasfj
Copy link
Member

jonasfj commented Dec 18, 2019

@Kleak, another option is to leave middleware to shelf.Pipeline, see shelf middleware docs.

Looking at: https://pub.dev/documentation/shelf/latest/shelf/Pipeline-class.html
That might be a fine solution for middleware..

Mostly, you'll want middleware to wrap all requests, so one would do something like:

final router = Router();
router.get(...); // setup routes + handlers

final pipeline = Pipeline()
    .addMiddleware(loggingMiddleware)
    .addMiddleware(cachingMiddleware)
    .addHandler(router.handler);

In the rare case that we need a set of middlewares that wraps all end-points under /api/ we could create a new sub-router for /api/ and simply have an router.all('/api/' handler that forwards to a new pipeline and sub-router.

That way we keep the logic simple, and reuse the constructs given in package:shelf. If anyone is interested, I think it would be worth while to document this pattern in the example for package:shelf_router or package:shelf_router_generator. Or maybe by adding a section in the README.md..

@kevin-sakemaer
Copy link
Author

That's seems great, I always forget the Pipeline class 🙁.

I can make a PR to add this as an example.

@kevin-sakemaer
Copy link
Author

In fact there is a little problem.
If I want to use a middleware only on specific route I have to wrap it in a Router.
For example a middleware that check if the user is authenticated.

@jonasfj
Copy link
Member

jonasfj commented Jan 6, 2020

If I want to use a middleware only on specific route I have to wrap it in a Router.
For example a middleware that check if the user is authenticated.

Yes.

I wish we could do some sort of middleware annotations too.. Like:

class MyService {

  @shelf_router.Route.get('/my-path/<param>')
  @WithAuthorization(permission: 'user-read')
  shelf.Response myHandler(shelf.Request req, String param) {...}
}

class WithAuthorization extends shelf_router.MiddlewareAnnotation {
  /// Required permissions
  final String permission;

  const WithAuthorization({this.permission})

  @override
  shelf.Response wrapHandler(shelf.Request req, shelf.Response Function(shelf.Request) handler) {
     if (!getUser(req).permissions.contains(this.permission)) {
        return shelf.Response.unauthorized();
     }
     return handler(req);
  }
}

Such that we can create custom annotations by extending MiddlewareAnnotation. This would be super cool... But I'm not sure exactly how this should be designed or exactly how it should work.

@jonasfj jonasfj added the help wanted Extra attention is needed label Jan 6, 2020
@jonasfj
Copy link
Member

jonasfj commented Jan 6, 2020

If someone wants to draft a design and implement this, I'll be happy to help with reviews.

@kevin-sakemaer
Copy link
Author

@jonasfj In your example the handler in wrapHandler correspond to the myHandler function ?

@jonasfj
Copy link
Member

jonasfj commented Mar 31, 2020

@Kleak, yes, that's the thinking..

So the code-generator would be aware of annotations that implement MiddlewareAnnotation, instantiate them, and call the wrapHandler method, instead of calling the actual handler.

I'm not sure exactly how this would work, or if it's flexible enough.. But playing with this might be a good enough start.

@kevin-sakemaer
Copy link
Author

Seems you added a way to add a middleware but didn’t expose it in shelf_router.
Maybe we can expose this first then we can use it in shelf_router_generator ?

Seems the signature of the 2 functions are not compatible we will need to do something between.

@jonasfj
Copy link
Member

jonasfj commented Apr 1, 2020

Ah, yes, myHandler(shelf.Request, String) has a signature that depends on the number of variables in the route.. I doubt we can expose that in wrapHandler.

But I see no reason wrapHandler can't trivially by given a handler that wraps myHandler to hide the extra parameters.

Like said, this was just a possible design -- I think it can be implemented.

Seems you added a way to add a middleware but didn’t expose it in shelf_router.
Maybe we can expose this first then we can use it in shelf_router_generator ?

I haven't added anything for middleware, I just outline a possible design. And yes, ideally it would be exposed in shelf_router though I'm not sure that's easy or possible -- so maybe one just do it in code-gen.

I haven't explored this in depth yet, there is lots of details to be worked out. These are just ideas.

@kevin-sakemaer
Copy link
Author

kevin-sakemaer commented Apr 1, 2020

in shelf_router here a route entry can have a middleware but we can’t add it from here
Maybe we can let the user add a middleware with a named parameters.

Then we can transform this to a List and then use Pipeline to insert them all.

Then we can add an annotation to shelf_router_generator.

@jonasfj
Copy link
Member

jonasfj commented Apr 1, 2020

in shelf_router here a route entry can have a middleware but we can’t add it from here

ah, I had completely forgotten about that.

Hmm, I don't think I ever really considered how middleware would be added.

Using a named parameter for adding middleware is certainly an option. But I'm not sure it's very pretty or ergonomic.


Crazy idea: given that middlewares always have the signature:
typedef Middleware = FutureOr<shelf.Response> Function(shelf.Request, Future<shelf.Response> Function(shelf.Request))

We could make it so that Router.add(verb, route, handler) checks if handler is Middleware, and then considers it middleware for all requests matching verb and route.

We would need to refactor the internals of Router and we would get rid of RouterEntry._middleware.

Example

var app = Router();

// Middleware applied to all requests for all handlers after this point.
app.all('/<|.*>', (Request request, Handler handler) async {
  // Tweak [request] as desired
  final response = await handler(request);
  // Tweak [response] as desired
  return response;
});

app.get('/hello', (Request request) {
  return Response.ok('hello-world');
});

// Middeware applied to /user/<user>
app.get('/user/<user>', (Request request, Handler handler) async {
  // Tweak [request] as desired
  final response = await handler(request);
  // Tweak [response] as desired
  return response;
});

app.get('/user/<user>', (Request request, String user) {
  return Response.ok('hello $user');
});

// Note: Declaring middleware here is useless because handlers are declared higher up.

var server = await io.serve(app.handler, 'localhost', 8080);

This might be too complicated. Perhaps it's better to have a single Router.use(route, middleware) method.. It's not clear to me how opinionated we should be.

@kevin-sakemaer
Copy link
Author

This might be too complicated

i think so :/


Thinking about an other approach which is breaking.

app.get('/user/<user>', (Request request, String user) {
  return Response.ok('hello $user');
});

if this handler doesn't get the url params user in the function signature it would be easier because it would be possible to use the Pipeline syntax to get an handler with multiple middleware.

And to get the url params we can write an extension on Request to get the params from the context (they already are in the context so it would not be difficult).

@kevin-sakemaer
Copy link
Author

a pseudo-code of my example above
without middleware :

app.get('/user/<user>', (Request request) {
  return Response.ok('hello ${request.getPathParams['user']}');
});

and with middleware :

app.get(
  '/user/<user>', 
  Pipeline().addMiddleware(myMiddleware).addHandler((Request request) {
    return Response.ok('hello ${request.getPathParams['user']}');
  })
);

@jonasfj
Copy link
Member

jonasfj commented Apr 1, 2020

app.get(
  '/user/<user>', 
  Pipeline().addMiddleware(myMiddleware).addHandler((Request request) {
    return Response.ok('hello ${shelf_router.params(request, 'user')}');
  })
);

This you can already do using params. The signature of the handled is NOT required to have all URL parameters. It's required to have none OR all.

If this isn't evident from examples and documentation feel free to suggest updates. Maybe we should add a section in the README.md with an explanation of how to use Pipeline for middlewares.

@kevin-sakemaer
Copy link
Author

that's true i did it little bit after writing my message.
Don't you think it's a good solution ?

@jonasfj
Copy link
Member

jonasfj commented Apr 14, 2020

I think using Pipeline whenever possible is a good solution... it's not as elegant as code-gen, but as the underlying support for code-gen it's probably fine :)

@EmreSURK
Copy link

EmreSURK commented Feb 10, 2022

Hi,
We have middleware but the problem is they are here for the whole app. We need also route-based middleware.

Middlewares should have the capability

  1. to prevent a request to reach the target handler.
  2. to make some changes in the Request object.

Authentication for example:
Some routes like log in or register should not have an authentication middleware.
Some other routes need authentication middleware like sendMessage, etc.

And we have a middleware like:

 Future<Response> authenticationMiddleware(Request request) async {
    // check user token.
    final user = {}; // find user from DB or check JWT.
    
    // user not found, we return an object and interrupt the request so the handler does not need to deal with authentication, we can be sure if the request reached the handler then the user has the necessary authentication.
    if (user == null) {
      return Response.forbidden("You have no permission.");
    }

    request.context["user"] = user;

    next();
  }
// no need for authentication.
app.get("/login",loginRouteHandler);

// we need an authentication.
app.get("/me",  [ authenticationMiddleware ] ,meRouteHandle);

Another use case is the body or URL parsing. We can give the name of the model to the middleware that implements Serializable and if the middleware can not parse the body, it interrupts the request and returns a' bad request' error. If the request reaches the handler, the handler just reads the parsed model and uses it. I am not sure how can we give a parameter to the middleware function, but we can even start with creating middleware for everybody object since everybody object is defined as DTO, a separate class that is created for data serving and accepting.

I believe both scenarios are a must for enterprise-grade backend applications.

In Router class there is some code about middleware, I am not sure why we cannot give a middleware with app.get or app.post and NY other methods.

I can write middleware like in the example but:

  1. It does not take any other parameter
  2. we can not use multiple middlewares for a route.

Here is a trick to give a middleware for a router:

app.get("/test", (Request request) => authenticationMiddleware(request, AuthController().me));

I can also try to implement that feature if you may.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed pkg:shelf_router_generator pkg:shelf_router Package shelf_router
Projects
None yet
Development

No branches or pull requests

3 participants