google / dart-neats

neat dart packages.
Apache License 2.0
464 stars 85 forks source link

Middleware usage #32

Open Kleak opened 4 years ago

Kleak commented 4 years ago

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 ?

jonasfj commented 4 years ago

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.

Kleak commented 4 years ago
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.

Kleak commented 4 years ago

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 commented 4 years ago

@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..

Kleak commented 4 years ago

That's seems great, I always forget the Pipeline class šŸ™.

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

Kleak commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Kleak commented 4 years ago

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

jonasfj commented 4 years ago

@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.

Kleak commented 4 years ago

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 commented 4 years ago

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.

Kleak commented 4 years ago

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 commented 4 years ago

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.

Kleak commented 4 years ago

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).

Kleak commented 4 years ago

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 commented 4 years ago
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.

Kleak commented 4 years ago

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

jonasfj commented 4 years ago

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 commented 2 years ago

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.