inversify-express-utils icon indicating copy to clipboard operation
inversify-express-utils copied to clipboard

Added @withMiddleware decorator for controllers and handlers

Open Minibrams opened this issue 4 years ago • 6 comments

The pull request adds a @withMiddleware() decorator for registering middleware on controllers and handlers.

Description

The @withMiddleware() piggybacks off of the existing way of registering controller- and handler middleware. The decorator takes a list of middleware and stores these as metadata that will be loaded when the corresponding @controller() or http...() decorator is invoked. The middleware must implement the interfaces.Middleware interface, and as such can be defined as a simple (req, res, next) => { } function or the symbol/string identifier for a containerised service that implements the BaseMiddleware interface.

Related Issue

#288

Motivation and Context

The @withMiddleware() decorator provides a nice and clean way of decorating endpoints with middleware, slightly reminiscent of ASP.NET controllers:

function authenticate() {
    return withMiddleware(
        (req, res, next) => {
            if (req.user === undefined) {
                res.status(401).json({ errors: [ 'You must be logged in to access this resource.' ] })
            }
            next()
        }
    )
}

function authorizeRole(role: string) {
    return withMiddleware(
        (req, res, next) => {
            if (!req.user.roles.includes(role)) {
                res.status(403).json({ errors: [ 'Get out.' ] })
            }
            next()
         }
    )
}

@controller('/api/user')
@authenticate()
class UserController {

    @httpGet('/admin/:id')
    @authorizeRole('ADMIN')
    public getById(@requestParam('id') id: string) {
        ...
    }
}

It also provides a quick way of registering one-off middleware functionality:

@controller('/api/user')
class UserController {

    @httpGet('/admin/:id')
    @withMiddleware(
        (req, res, next) => { ... },
        (req, res, next) => { ... }
    )
    public getById(@requestParam('id') id: string) {
        ...
    }
}

How Has This Been Tested?

Wrote unit tests to define the desired behaviour of the decorator (see /test/features/decorator_middleware.test.ts). Wrote a small test controller and used curl to see if the middleware was indeed invoked as expected.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] My code follows the code style of this project.
  • [x] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

Minibrams avatar Jan 17 '21 02:01 Minibrams

This is pretty sweet. Any chance this will get merged to the project?

priyath avatar Apr 21 '21 19:04 priyath

Hi @priyath , @Minibrams, we are focused now on getting the main project green, it looks great! I'll have a look as soon as I can

notaphplover avatar Apr 24 '21 22:04 notaphplover

@notaphplover Let me know if I can be helpful in getting this merged or getting the project green 🙂

Minibrams avatar May 30 '21 20:05 Minibrams

@Minibrams as soon as I merge this https://github.com/inversify/inversify-express-utils/pull/344 We might need to change a bit due to stricter typescript ( we'll see ) but this will be merged . Didn't check but are the tests 100% ?

Thanks for doing this.

PodaruDragos avatar Aug 25 '21 11:08 PodaruDragos

@Minibrams Will you please update this PR and resolve the conflicts? Thank you

dcavanagh avatar Sep 28 '21 12:09 dcavanagh

@dcavanagh Tried my best to resolve the conflicts. Unfortunately, the new linter lints my dist/ folder, so I can't run tests anymore to verify that everything works.

In the future I think it would be wise to postpone massive code-changes like those introduced by a linter until all/most other functional PRs have been merged. Linter changes will always cause a massive amount of merge conflicts that can be difficult to resolve, and it's very easy to overlook code being lost in the process.

All of the currently open PRs are blocked by this.

Minibrams avatar Oct 24 '21 22:10 Minibrams

Rebased with changes from @rev42, tests now pass again - thank you!

@dcavanagh Ready to merge now?

Minibrams avatar Nov 24 '23 13:11 Minibrams

IMG_0250

silentroach avatar Nov 24 '23 13:11 silentroach

@Minibrams hello, I went ahead and merged main and added some other things I wanted to change into main. I'll merge this, Thanks for this and sorry for the wait

PodaruDragos avatar Nov 25 '23 12:11 PodaruDragos

@PodaruDragos thank you for the merge. When do you think a new version can be published to npm?

rev42 avatar Nov 27 '23 16:11 rev42