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

Register Express middleware per route, controller class or controller method #2035

Closed
bajtos opened this issue Nov 16, 2018 · 11 comments
Closed
Assignees
Labels
feature REST Issues related to @loopback/rest package and REST transport in general user adoption

Comments

@bajtos
Copy link
Member

bajtos commented Nov 16, 2018

As a user migrating from Express, or simply wishing to leverage an existing Express middleware in my application, I would like to enable this middleware for certain routes, controller methods or entire controller classes.

Consider the following definition in an Express app:

app.get('/api/users', 
  requireAuthenticated(),
  requireScope('read:users:all'), 
  withCacheControl({maxAge: '2 hours', private: true}),
  function getUsers(req, res, next) {
    // the route
  });

The code snippets below illustrate different ways of applying the middleware. Please note they are just examples, not a final proposal.

Per route

A possible LB4 solution (not very ergonomic, a better solution is needed):

app.route(new ExpressRoute(
  'get',
  '/api/users',
  {
    parameters: [
      {name: 'filter', in: 'query', schema: {/*...*/}}
    ],
    responses: { /*...*/ }
  },
  [
    requireAuthenticated(),
    requireScope('read:users:all'), 
    withCacheControl({maxAge: '2 hours', private: true}),
  ],
  function findUsers(filter: Filter) {
    return userRepo.find(filter);
  }
));

Per controller method

class UserController {
  @get('/api/users')
  @use(
    requireAuthenticated(),
    requireScope('read:users:all'), 
    withCacheControl({maxAge: '2 hours', private: true}),
  )
  find(
    @param.query.object('filter', {/*schema*/})
    filter: Filter
  ) {
    return userRepo.find(filter)
  )
}

Per controller class

This allows developers to apply the same set of middleware for multiple routes.

@use(requireAuthenticated())
class UserController {
  @get('/api/users')
  @use(requireScope('read:users:all'))
  find() {
    // ...
  )

  @del('/api/users')
  @use(requireScope('write:users:all'))
  delete() {
    // ...
  )
}

Acceptance criteria

TBD

Out of scope

@bajtos bajtos added feature REST Issues related to @loopback/rest package and REST transport in general feature parity user adoption and removed feature parity labels Nov 16, 2018
@raymondfeng
Copy link
Contributor

See #1671, which can be possibly used as the plumbing.

@ghost
Copy link

ghost commented Jan 27, 2019

@bajtos @raymondfeng @dhmlau
why do you need to use this proposal ? please elaborate .
just an idea , perhaps we can make it look like lb3 .
an middleware dir that holding all the middlewares classes (like controller) each class should have PreHandling and PostHandling method (preHandling before invoke controller , posthandling after invoke the controller). with keeping in minds the order of the middleware.

for example this method will added to restApplication

  middleware(middlewareCtor: any): Binding {
    return this.bind('mids.' + middlewareCtor.name).toClass(
      middlewareCtor,
    ); 

class MyMiddleWare{
 readonly order =  1;
preHandlingRequest(req,res,next){
next();
}
postHandlingRequest(req,res,next){
next(); 
}
}

so developer can run app.middleware(MyMiddleWare);
or by the booter inside the load function .

using this approach we can make the whole thing separated and more clean .

@raymondfeng
Copy link
Contributor

raymondfeng commented Jan 28, 2019

@sanadHaj Yes, I have something similar to allow middleware registration using extension point/extension via Context. See #2249 as an example.

There is another related PR - #1671

Are you willing to submit a patch?

@ghost
Copy link

ghost commented Jan 29, 2019

@raymondfeng
yep , but first off all let me know how you intend to combine extension and phases to handle middleware
keep in minds before and after controller handle the request ?
secondly may we need to refactor restServer loopback/rest instead of mount requesthandler directly to exprees
(
https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest.server.ts#L224
).
just to call the phases and the phases will forward the request to requestHandler.

let me know if i am right,
the developer will implement the extension and under the hod the phases will inject the
extension inside the middileWarePhaseList . and from there call the
phases and continue the job .

@bajtos
Copy link
Member Author

bajtos commented Feb 1, 2019

Good discussion! Here is my point of view.

From our experience with using Express and Express middleware in LoopBack 1.x/2.x/3.x, middleware handlers are difficult to compose and reason about, especially when there are multiple places contributing them (the application + 3rd-party extensions). We have somewhat addressed this problem with "middleware phases" (docs). Unfortunately, it seems like this concept did not gain wider adoption.

For LoopBack 4, we would like to offer a different design, one that's closer to our concept of a Sequence, a Controller with methods, and a handler route. I think beforeRemote/afterRemote hooks in LoopBack 3.x are a good example of a solution that's different from Express middleware and more tailored to LoopBack programming model.

This GitHub issue IS NOT about those hooks, it's not about the LB4 way of handling things.

Having a custom design for before/after hooks comes with a downside too: app developers cannot reuse existing Express middleware and have to either wait until somebody ports Express middleware to LB4, or implement that functionality themselves.

There are also Express users with large applications containing many routes and leveraging express middleware configured differently for each route. I would like to make it easier for them to upgrade their Express-based applications to LoopBack 4, to allow them to upgrade incrementally.

The goal of this issue is to support those two groups of users:

  • Developers looking for LB4 alternative an existing Express middleware, an alternative that does not exist yet.
  • Developers migrating from Express to LB4.

I don't think loopback-phases (#1671) is a good solution for this particular feature. Phases may be useful for application-wide middleware, that's out of scope of this issue though (see #1293 and #1982).

an middleware dir that holding all the middlewares classes (like controller) each class should have PreHandling and PostHandling method (preHandling before invoke controller , posthandling after invoke the controller). with keeping in minds the order of the middleware.

This may work for LB4-specific hooks/middleware, but that's not what we want here. This GitHub issue is focused specifically on allowing LB4 routes to call Express middleware.

@sanadHaj could you please open a new issue to discuss your specific scenario? It looks like something LoopBack should support, but I feel we need more details to better understand what you are trying to achieve. It's better to move the discussion to a new issue to keep the discussion here focused on the original topic.

@ghost
Copy link

ghost commented Jun 6, 2019

@bajtos
how you plan to provide the requestContext ?

@use( requireAuthenticated())

@bajtos
Copy link
Member Author

bajtos commented Jun 14, 2019

@sanadHaj

how you plan to provide the requestContext ?

As I wrote before, this GH issue is about using regular Express middleware, e.g. helmet, cors or even passport. AFAIK, there is no concept of requestContext in Express, I don't see why we should be exposing requestContext to Express middleware.

Based on your older comment:

an middleware dir that holding all the middlewares classes (like controller) each class should have PreHandling and PostHandling method (preHandling before invoke controller , posthandling after invoke the controller). with keeping in minds the order of the middleware.

I think you are looking for "method interceptors" that have been recently introduced by #2687, see also #2907, https://loopback.io/doc/en/lb4/Interceptors.html and https://loopback.io/doc/en/lb4/Interceptor-generator.html.

@clayrisser
Copy link
Contributor

clayrisser commented Jun 21, 2019

You can register middleware on a controller using the lb4-middleware component.

  @middleware((_req: Request, _res: Response, next: NextFunction) => {
    return next();
  })
  ping(): object {
    return {
      greeting: 'Hello from LoopBack'
    };
  }

https://github.com/codejamninja/lb4-middleware

@victor-enogwe
Copy link

In case someone else comes here, see the comment here

@dhmlau
Copy link
Member

dhmlau commented Apr 21, 2020

See PR #5118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature REST Issues related to @loopback/rest package and REST transport in general user adoption
Projects
None yet
Development

No branches or pull requests

5 participants