Fix task escaping

[teslitsky]

Fix for operators with '$', such as: { id: 123, balance: { $gte: 10 } }

[e-oj]

Very nice catch. What do you think of a recursive solution? Like, instead of

if (!fix && typeof obj[key] === 'object') {
        var childKeys = Object.keys(obj[key]);
        for(var j = 0; j < childKeys.length; j++){
          var current = childKeys[j];

          if(current[0] === "$"){
            obj[key][ESCAPE_PREFIX + current] = obj[key][current];
            delete obj[key][current];
          }
       }
  }

we could have

if (typeof obj[key] === 'object') {
      handle$token(obj[key], fix);
      continue;
}

at the top of the main loop. This would recursively encode/decode(fix) the keys of all nested objects which I think is ok because mongodb doesn't allow keys to start with '$' regardless of how deeply nested they are.

[teslitsky]

Yep, great solution! Do I need make new PR or you commit this?

[e-oj]

You can make a PR. I'll accept it.

[e-oj]

it's actually a bit tricky though because "typeof" returns object for literal objects, functions, arrays and null. To make it work, change the isObject function in task.js to,

function isObject(obj){
  return obj && obj.constructor && obj.constructor === Object;
}

and handle$token to:

function handle$Token(obj, fix){
  var ESCAPE_PREFIX = constants.ESCAPE_PREFIX;
  var escapeLength = ESCAPE_PREFIX.length;
  var keys = Object.keys(obj);
  var key;
  var $key;

  for(var i = 0; i < keys.length; i++){
    key = keys[i];

   //=======> this is the fix
    if(isObject(obj[key])){
      handle$Token(obj[key], fix);
    }

    if(!fix && key[0] === "$"){
      obj[ESCAPE_PREFIX + key] = obj[key];
      delete obj[key];
    }

    if(fix && key.length >= escapeLength && key.substring(0, escapeLength) === ESCAPE_PREFIX){
      $key = key.replace(ESCAPE_PREFIX, "");
      obj[$key] = obj[key];
      delete obj[key];
    }
}

I'd do it myself but I've made many changes locally that I'm not ready to push yet. Thanks for your help.

[teslitsky]

I made some research and I think

function isObject(obj){
  return obj === Object(obj);
}

more suitable and universal way. Sorry for previous messy code, this was emergency fix for production.

[e-oj]

Thanks man. Solid work.

[e-oj]

Actually, this

function isObject(obj){
  return obj === Object(obj);
}

returns true for an array which is not ideal for queries like {name: {$in: ["name1", "name2"]}} (we don't want to recurse on arrays). So I'll revert to the other method. Thoughts?

[teslitsky]

But [] === Object([]) // false and first solution with obj.constructor do not work for me.

[e-oj]

[] === Object([]) is false because it's comparing two different arrays. They're both empty, but they're different. Try

var list = []
list === Object(list) // returns true

What error did you get with the obj.constructor approach

[teslitsky]

I am create PR #3 with edge test cases that work with my solution and do not work with obj && obj.constructor && obj.constructor === Object.

Please, check line https://github.com/teslitsky/Fawn/blob/master/tests/task.tests.js#L64

I am using code like task.save(TestMdlA, nested) and it is not work, but work if i use task.save(nested) without model definition.

[e-oj]

That's a side effect of changing isObject(); previously, it returned true for any instance of a class now it only returns true for actual objects. It's a small fix, I should have it up later today. But yeah, task.save(nested) is expected to work; that functionality wasn't affected by isObject().

I'll push a couple of updates tonight that address all these issues. Also, thanks for the tests. I'll add those.