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

populate ctx.params before running any matched middleware #490

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

smkoyan
Copy link

@smkoyan smkoyan commented Dec 6, 2018

The problem is that when I use router.use(someMiddleware), in that middleware I have access to payload from query string via ctx.query and to payload from request body via ctx.request.body but I haven't access to route params and this is not so convenient especially in my case.

To be more clear I will tell my story. I have a validation middleware which works like this.
It takes request method from ctx.request.method and matched route from ctx._matchedRoute and using that it takes necessary validation rules from validation schemas that look like this:

'PUT:/users/:id': {
    params: Joi.object().keys({
        id: Joi.string().length(24).alphanum().required(),
    }),

    query: Joi.object().keys(definitions),

    body: Joi.object().keys(definitions)
}

and for each type of payload, if it exists in the scheme, it checks for validity and problem comes here when it tries to validate params because params are not available in the middleware defined using router.use method.

This pull request fixes this type of problems and I think this will will bring much good in the future.

Thanks!

@tranan89
Copy link

We need this as well

@divmgl
Copy link

divmgl commented Jun 13, 2019

This is huge. @ZijianHe can you please merge this?

@divmgl
Copy link

divmgl commented Jun 15, 2019

@smkoyan thanks for your work on this. this resolves most one of my issues with Router.prototype.param.

if (mostSpecificLayer.name) {
ctx._matchedRouteName = mostSpecificLayer.name;
}

layerChain = matchedLayers.reduce(function(memo, layer) {
memo.push(function(ctx, next) {
ctx.captures = layer.captures(path, ctx.captures);
ctx.params = layer.params(path, ctx.captures, ctx.params);
Copy link

@divmgl divmgl Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is needed in order to pass in params to the Router.prototype.param function in nested routers.

ctx.params = layer.params(layer.captures(path))

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #508

expect(res.body.param).to.equal(undefined);
done();
});
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smkoyan this test is important and removing it will not allow us to check use cases where users want a specific route to be hit followed by a catch-all route.

router.get('/specific-route') // Hit when specific-route is provided
router.get('/:catchall') // Hit in any other case with a capture

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #508

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.

3 participants