CODEWITHSUNDEEP
javascriptperformancenode.js
user1707250
user1707250

Reputation: 67

A better and faster way for eval?

I want to build my queries dynamically and use the following snippet:

--snip--
module.exports = {

get : function(req, res, next) {
  var queryStr = "req.database.table('locations').get(parseInt(req.params.id))";

  if (req.params.id) {
    if (req.fields) {
      queryStr += '.pick(' + req.fieldsStr + ')';
    }

    console.log(queryStr);
    eval(queryStr).run(function(result) { 
      console.log(result);
      res.send(result);
    });
  } else if (!req.params.id) {
--snip--

However introducing eval opens up my code to injection (req.fields is filled with url parameters) and I see the response time of my app increase from 7 to 11ms

Is there a smarter way to accomplish what I did here?

Please advice.

Upvotes: 0

Views: 132

Answers (2)

freakish
freakish

Reputation: 56467

As with every ( or most ) injection the best solution is to have predefined list of possible fields. For example:

var predefined_fields = [ "id", "name", "age" ];

if (predefined_fields.indexOf( req.fieldsStr ) !== 0) {
    // do something
}

Also, you can push this: parseInt(req.params.id) outside of queryStr:

var id = parseInt(req.params.id);
var queryStr = "req.database.table('locations').get("+id+")";

which solves second injection issue.

Of course it might get a bit complicated in future, so I advice to write ( or use ) some kind of query generator.

I don't know the library you are using, but it looks, like you can simply do query without string concatenation and eval. After all this .run method stands for something, right? This will definetly improve performance and safety.

EDIT It seems that you don't need these strings at all. This should work:

var query = req.database.table('locations').get(parseInt(req.params.id));
if (req.params.id) {
    if (req.fields) {
      query = query.pick( req.fieldsStr );
    }

    console.log(queryStr);
    query.run(function(result) { 
       console.log(result);
       res.send(result);
    });
}

Safe and efficient. :)

Upvotes: 1

Nathan Wall
Nathan Wall

Reputation: 10614

If I understand this correctly, you should go with something like the following:

--snip--
module.exports = {

get : function(req, res, next) {

  var queryResult = req.database.table('locations').get(parseInt(req.params.id));

  if (req.params.id) {
    if (req.fields) {
      queryResult = queryResult.pick.apply(queryResult, getFields(req.fieldsStr));
    }

    queryResult.run(function(result) { 
      console.log(result);
      res.send(result);
    });
  } else if (!req.params.id) {
--snip--

where getFields is something like:

var fields = {
        'name': name,
        'address': address,
        'zipcode': zipcode
        // ...
    };

function getFields(str) {
    return str.split(',').map(function(u) {
        return fields[u];
    });
}

Of course, if req.fields is an array of strings itself, you could use that instead of splitting req.fieldsStr.

Upvotes: 1

Related Questions