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

req.param('length') == 0 #3738

Closed
jessgwen opened this issue May 21, 2016 · 7 comments
Closed

req.param('length') == 0 #3738

jessgwen opened this issue May 21, 2016 · 7 comments

Comments

@jessgwen
Copy link

jessgwen commented May 21, 2016

Sails version:0.12.3
Node version:0.10.29
NPM version:1.4.21
Operating system:Debian Jessie


req.param('length') always returns zero, even if the request contains a parameter called length with some other value. For a minimal demonstration, see this example repo.

This has been discussed on StackOverflow, but the work-around proposed there isn't always viable (say the app uses a client-side JS library (for example datatables) which it's infeasible to rewrite to use a different parameter name).

It's also possible to work-around by using req.allParams().length rather than req.param('length'), but since, in my experience, the latter is the standard pattern for accessing request parameters, it's seriously unexpected for it not to work.

If, as suggested in some of the StackOverflow answers, it's difficult to make this work as expected because of the specialness of length to JS arrays, then this behaviour should at least be noted in the documentation

@mikermcneil
Copy link
Member

@jghall Thanks for the detailed explanation. I agree with you completely, this is unexpected. The cleanest solution here IMO would be to get this working straight at the source, but if that's not possible, we can wrap the function.

@mikermcneil
Copy link
Member

mikermcneil commented Jun 2, 2016

Talked w/ @sgress454 and looked at the impl in Express, and I think the best approach here is to wrap the method in either the request hook or in the http hook-- but specifically the http hook would be better here, since this is Express-specific (Sails' VIR doesn't have this problem):

// In http hook:
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
var origReqParam = req.param; 
req.param = function getValForParam (name){
  if (name === 'length') {
    // If `req.params.length` is different from the actual number of route path params,
    // then we know this route address must be like `/foo/bar/:length/baz`.  So in that case, we'll
    // allow req.param('length') to return the runtime value of `length` as a string.
    var trueReqParamsLength = _.reduce(req.params, function (memo, routePathParam){
      memo += 1;
      return memo;
    }, 0);
    // Note that we're careful to cast to a number for comparison and cast to a string for the result.
    if (trueReqParamsLength !== +req.params.length) {
      return req.params.length+''; 
    }
    if (!_.isArray(req.body) && _.isObject(req.body) && !_.isUndefined(req.body.length) && !_.isNull(req.body.length)) {
      return req.body.length;
    }
    else if (_.isObject(req.query) && !_.isUndefined(req.query.length) && !_.isNull(req.query.length)) {
      return req.query.length;
    }
    else { return undefined; }
  }
  return origReqParam.apply(req, Array.prototype.slice.call(arguments));
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 

...and then add a note at the bottom of the reference page for req.param() in the Sails docs explaining the behavior when calling req.param('length').

@mikermcneil
Copy link
Member

For context, the req.param() implementation in the VIR circumvents this problem by only checking enumerable properties:

param: function(paramName, defaultValue) {

@mikermcneil
Copy link
Member

mikermcneil commented Jun 2, 2016

Another pass at the code to better account for req.param('length'):

// In http hook:
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
var origReqParam = req.param; 
req.param = function getValForParam (name){
  if (name === 'length') {
    // If `req.params.length` is a string, instead of a number, then we know this request
    // must have matched a route address like `/foo/bar/:length/baz`, so in that case, we'll
    // allow `req.param('length')` to return the runtime value of `length` as a string.
    if (_.isString(req.params.length)) {
      return req.params.length; 
    }
    else if (!_.isArray(req.body) && _.isObject(req.body) && !_.isUndefined(req.body.length) && !_.isNull(req.body.length)) {
      return req.body.length;
    }
    else if (_.isObject(req.query) && !_.isUndefined(req.query.length) && !_.isNull(req.query.length)) {
      return req.query.length;
    }
    else { return undefined; }
  }
  return origReqParam.apply(req, Array.prototype.slice.call(arguments));
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 

...and then add a note at the bottom of the reference page for req.param() in the Sails docs explaining the behavior when calling req.param('length').

@sailsbot
Copy link

sailsbot commented Jul 3, 2016

@jghall,@mikermcneil: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

@sailsbot sailsbot added the waiting to close This label is deprecated. Please don't use it anymore. label Jul 3, 2016
@sgress454
Copy link
Member

Thanks for the nudge @sailsbot. You are a useful member of society no matter what anyone says.

I think we need a test for this, then we can close. I'll write one up before we push 0.12.4.

@sailsbot sailsbot removed the waiting to close This label is deprecated. Please don't use it anymore. label Jul 5, 2016
sgress454 added a commit that referenced this issue Jul 5, 2016
@sgress454
Copy link
Member

Ok, tests written, thanks all!

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

No branches or pull requests

4 participants