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

Added @withMiddleware decorator for controllers and handlers #337

Merged
merged 4 commits into from
Nov 25, 2023

Conversation

Minibrams
Copy link
Contributor

@Minibrams Minibrams commented Jan 17, 2021

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)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

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

@priyath
Copy link

priyath commented Apr 21, 2021

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

@notaphplover
Copy link
Member

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

@Minibrams
Copy link
Contributor Author

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

@PodaruDragos
Copy link
Contributor

@Minibrams as soon as I merge this #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.

@dcavanagh
Copy link
Member

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

@Minibrams
Copy link
Contributor Author

Minibrams commented Oct 24, 2021

@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
Copy link
Contributor Author

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

@dcavanagh Ready to merge now?

@silentroach
Copy link

IMG_0250

@PodaruDragos
Copy link
Contributor

@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 PodaruDragos merged commit ebb986f into inversify:master Nov 25, 2023
4 checks passed
@rev42
Copy link
Contributor

rev42 commented Nov 27, 2023

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

@abarghoud
Copy link

Hello @PodaruDragos, can you please publish a new version including this feature to npm, it would be a very nice feature 🙏

@PodaruDragos
Copy link
Contributor

hey @abarghoud i do not have any permissions for releasing.

@rev42
Copy link
Contributor

rev42 commented Sep 19, 2024

Hello @dcavanagh and @Jameskmonger,
Could you please release a new version of the library with latest changes from master?

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

Successfully merging this pull request may close these issues.

8 participants