Validating query parameters for REST api requests in node.js

Question

I have an object of valid query parameters for each object type for a GET request to the API.

var queryFields = {
  'organisation': ['limit', 'page', 'id', 'search'],
  'actor': ['limit', 'page', 'id', 'search'],
  'version': ['limit', 'page', 'search'],
  'product': ['limit', 'page', 'search', 'id', 'type', 'brand', 'model', 'manufacturerSpid'],
  'asset': ['limit', 'page', 'search', 'order', 'sort', 'id', 'name', 'currentCustodianSpid', 'currentLocationSpid', 'productSpid', 'search'],
  'location': ['limit', 'page', 'search', 'id'],
  'workorder': ['limit', 'page', 'search', 'id', 'type', 'status', 'requirementSpid', ],
  'move': ['limit', 'page', 'search'],
  'transfer': ['limit', 'page', 'search'],
  'requirement': ['limit', 'page', 'search', 'id', 'type', 'source', 'productSpid', 'status', ],
  'artefact': ['limit', 'page', 'search'],
  'attestation': ['limit', 'page', 'search'],
};

I want to use this function to make sure that only these valid parameters are accepted for a request. Right now the promise resolves false with valid, invalid, or 0 parameters. It seems to be an issue with the way I am filtering. I pass in the object type and the request. If the request has query parameters, I want to grab the valid parameters from the object, and check that the parameters in the req are all valid matches to ones in the object. If there are any that are invalid, I want to resolve false. If there are no parameters, I want to resolve true. If there are only valid parameters, I want to resolve true. Is there some tweaking I can do to this function to get that outcome?

function getQueryFields(object) {
  if (utils.isDefined(queryFields[object])) return queryFields[object];
  return [];
}

function fieldValidator (objType, req) {
  return new Promise(function(resolve) {
    if (utils.isDefined(req.query)) {
      var fields = getQueryFields(objType);
      //Only resolve true with valid fields
      fields = fields.filter(function(field) { return Object.keys(req.query).indexOf(field) > -1;});
      if (Object.keys(req.query) !== Object.keys(fields)) {
        resolve(false);
      } else {
        resolve (true);
      }
    } else {
      resolve(true);
    }
  });
}

Answer 1

If using expressjs there is a nice way doing this using check api.

https://express-validator.github.io/docs/check-api.html

Answer 2

There's a few issues with your function. I want to fix the first issues before getting into your actual problem, because it will increase the clarity quite a bit. First: you don't need Promises, this is a synchronous function.

Rewrite #1:

function getQueryFields(object) {
  if (utils.isDefined(queryFields[object])) return queryFields[object];
  return [];
}

function fieldValidator (objType, req) {
  if (utils.isDefined(req.query)) {
    var fields = getQueryFields(objType);
    //Only resolve true with valid fields
    fields = fields.filter(function(field) {
      return Object.keys(req.query).indexOf(field) > -1;
    });
    if (Object.keys(req.query) !== Object.keys(fields)) {
      return false;
    } else {
      return true;
    }
  }
} else {
  return true;
}

Another thing this function could use is an 'early' return. This makes it easier to follow what is going on and reduces the number of branches:

Rewrite #2:

function fieldValidator (objType, req) {
  if (req.query === undefined) {
    return true;
  }

  var fields = getQueryFields(objType);
  //Only resolve true with valid fields
  fields = fields.filter(function(field) {
    return Object.keys(req.query).indexOf(field) > -1;
  });
  return (Object.keys(req.query) === Object.keys(fields));
}

None of this answers your question, but I needed it to get more clarity on what you're doing =)

The issue is actually in comparing Object.keys(). Object.keys() returns an iterator, but every iterator that it returns is unique.

Objects in Javascript can't really compared 'by value'. The only way to compare objects by value is to inspect their keys one by one.

Since you want the properties to exactly match, I think I would change this to:

1. Checking if you have the same number of query parameters.
2. Check if every query parameter that was passed appears in the set of valid query parameters.

Based on that, I think this would be my version:

function fieldValidator(objType, req) {
  if (!req.query || Object.keys(req.query).length === 0) {
    // Covers the 'undefined' and 'empty object' case
    return true;
  }

  const fields = getQueryFields(objType);
  const keys = Object.keys(req.query);

  // Do we have enough query parameters?
  if (keys.length !== fields.length) return false;

  // Does every query parameter appear in the list?
  for(const key of keys) {
     if (!fields.includes(key)) return false;
  }
  return true;
}

Answer 3

"Fields" is an array of key names. You're checking the array of req.query keys against the object keys of the array of key names. That's the indexes of the array, just consecutive integers, ["0", "1", "2", ... etc] . Not to mention, you're doing an inequality check between two arrays, which will never be true unless the references are the same, which they're not here. So of course that first condition always fails and resolves to false. Try it yourself in a console: [1, 2, 3] === [1, 2, 3] will be false (same with loose equality checks) because they're different objects that just happen to have the same entries.

So I think a better approach is to change your filter so it filters away every query field that's in the list, and ensure the final array has no entries (as anything left would be a key that doesn't match the list).

  fields = Object.keys(req.query).filter(function(field) { return fields.indexOf(field) > -1;});
  if (fields.length > 0) {
    resolve(false);
  } else {
    resolve (true);
  }

(I'm assuming you have an untold reason for using a Promise; if not, then I'd go with Marcos Casagrande's suggesting of getting rid of the Promise altogether and just returning true or false from the function directly.)