0

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);
    }
  });
}
2
  • You don't need to complicate that code with a Promise, everything in there is synchronous. You can return a boolean directly. And also, there's no need for utils.isDefined, a simple if(req.query) is enough. Which also is not needed if you're using express, req.query is always defined as an empty object if there's no parameter. Commented Apr 3, 2019 at 15:06
  • Show how you're using fieldValidator Commented Apr 3, 2019 at 15:11

3 Answers 3

1

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;
}
Sign up to request clarification or add additional context in comments.

6 Comments

!req.query part is correct, but the empty array comment it's not. ![] is false. In any case, req.query won't be an array if this is coming from express and he's not modifying req.query in any way.
@MarcosCasagrande I updated the comment to 'Empty Object'. I feel that that was wat OP was looking for but not sure. As for express... there's no real telling if it is, so didn't want to jump to conclusions!
Acually, !{} is still false, the only falsy values in javascript are: null, undefined, 0 '', false
@MarcosCasagrande yes but I believe that's what OP wanted based on the description. Maybe your interpretation is different.
Quote from OP: "If there are no parameters, I want to resolve true"
|
0

"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.)

Comments

0

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

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

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.