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

Controller middlewares must be applied to its controller only. #27

Closed
Rokt33r opened this issue Jan 22, 2019 · 2 comments
Closed

Controller middlewares must be applied to its controller only. #27

Rokt33r opened this issue Jan 22, 2019 · 2 comments
Labels
💰Bounty enhancement New feature or request help wanted Extra attention is needed

Comments

@Rokt33r
Copy link
Member

Rokt33r commented Jan 22, 2019

Tachijs's controllers are just representation of express routers. So, for now, tachijs apply controller middlewares to their each router directly.

This causes the below problem:

// A middleware which check if current user is signed in
const userOnly: express.RequestHandler = (req, res, next) => {
  if (req.user == null) {
    next(throw new Error('You have to sign in'))
    return
  }
  next()
}

@controller('/posts', [userOnly])
class UserOnlyPostController {
  // ...some methods just for signed in user, like writing, editing and deleting Post
}

@controller('/posts')
class PublicPostController {
  // ...some methods for everyone like listing articles and reading a specific article.
}

tachijs({
  controllers: [UserOnlyPostController, PublicPostController]
}).listen(8000)

is identically same to

const app = express()

const userOnlyPostRouter = express.Router()
userOnlyPostRouter.get(...)
const publicPostRouter = express.Router()
publicPostRouter.get(...)

// Every request have to pass userOnly middleware
app.use('/posts', userOnly, userOnlyPostRouter)
// So if current user isn't signed in, he cannot reach `publicPostRouter`
app.use('/posts', publicPostRouter)

app.listen(8000)

So userOnly middleware is applied the both controllers.
I think this should make people confusing. So we have to change the behavior that controller middlewares are applied to their controller only.

It should be really easy to fix because all we have to do is apply controller middlewares for each route just before applying method middlewares.

router.get('/', ...controllerMiddlewares, ...methodMiddlewares, handler)
@Rokt33r Rokt33r added enhancement New feature or request help wanted Extra attention is needed labels Jan 22, 2019
@IssueHuntBot
Copy link

@BoostIO has funded $50.00 to this issue. See it on IssueHunt

@IssueHuntBot
Copy link

@Rokt33r has rewarded $45.00 to @medusalix. See it on IssueHunt

  • 💰 Total deposit: $50.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $5.00

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

No branches or pull requests

2 participants