Discuss swaggerMetadata namespacing its processed request parameters

[apigee-127]

Discuss swaggerMetadata namespacing its processed request parameters

[whitlockjc]

I would love to have the feedback of @grahamj, @BigstickCarpet and others.

[JamesMessinger]

Are you saying that swaggerMetadata should overwrite req.params?

[whitlockjc]

I'm asking your opinion on the matter. Long story short, I namespaced the parameters swaggerMetadata processed to req.swagger.params just so I didn't muck with req.params. I've gotten some feedback that this could be a little unintuitive and so I wanted to get some feedback on it. What are your thoughts right now about this?

[JamesMessinger]

Ah, ok. Thanks for the clarification. I actually like the way it is now. For a few reasons:

• Everything else that swaggerMetadata adds to req is namespaced under req.swagger, so this is consistent behavior. The docs pretty clearly point this out, so IMHO, RTFM.
• Modifying req.params directly could break other middleware. By namespacing all of swaggerMetadata's stuff under req.swagger, you're guaranteed to play nice with other middleware.
• If people really want the Swagger params directly on the req object, then they can easily add this snippet to their app:

app.use(function(req, res, next) {
    if (req.swagger.params)
        req.params = req.swagger.params;
    next();
});

[whitlockjc]

+1

[prabhatjha]

Ok so I was the one who suggested it to Jeremy from an "outsider" perspective. My take was that it's one less thing for me to think when I am in controllers since I am so used to req.blah for requests in regular node app.

Coming from Java background, I was thinking something equivalent to:

 import swagger.request;
 var p = request.params;

[JamesMessinger]

I can definitely see why it would be desirable to have Swagger's param data directly at req.params, but the main issue is that it would break any middleware that expects the "normal" req.params object. Since swagger-tools needs to work for a variety of different use-cases, it really needs to play nice with other middleware.

That said, if you're not using any other third-party middleware that relies on req.params, then it's totally safe for you to use the middleware snippet that I posted above, which will overwrite req.params with the Swagger param data

[prabhatjha]

Cool. @whitlockjc I think it can be closed then.

[6utt3rfly]

I know this is coming in late, but I thought I would offer a bit more feedback:

1. Swagger-tools utilizes tools to set req.query and req.body, but it avoids req.params? Seems like it's breaking the pattern...
2. Node Express prevents modifying the req.params with middleware, so the suggestion by BigstickCarpet to add middleware would not work.
3. I think a simple solution would be to modify swagger-router.js to add:

var swaggerParamsToParams = function(req){
  _.forEach(req.swagger.params, function(param, key){
    if (param.schema.in === 'path')
      req.params[key] = param.value;
  })
}

just before calling the handler. That wouldn't break any middleware, but it would also allow the use of the more common req.params variable

[whitlockjc]

1. swagger-tools does nothing of the sort regarding req.body or req.query. The server middleware is responsible for these things. And while swagger-tools provides default middleware for you so things just work, it's up to you to own this if need be. req.swagger is the only object swagger-tools creates/manipulates

2. swagger-tools does not alter req.params

3. I don't know that we have a problem here so the solution doesn't make much sense to me