dchester / jsonpath

Query and manipulate JavaScript objects with JSONPath expressions. Robust JSONPath engine for Node.js.
MIT License
1.32k stars 216 forks source link

`.apply` skips the last path #183

Open PsyGik opened 10 months ago

PsyGik commented 10 months ago

given the code below:

var jp = require("jsonpath")
var data = {
  "store": {
    "book": [ 
      {
        "category": "reference",
        "author": "Nigel Rees",
        "title": "Sayings of the Century",
        "price": 8.95
      }, {
        "category": "fiction",
        "author": "Evelyn Waugh",
        "title": "Sword of Honour",
        "price": 12.99
      }, {
        "category": "fiction",
        "author": "Herman Melville",
        "title": "Moby Dick",
        "isbn": "0-553-21311-3",
        "price": 8.99
      }, {
         "category": "fiction",
        "author": "J. R. R. Tolkien",
        "title": "The Lord of the Rings",
        "isbn": "0-395-19395-8",
        "price": 22.99
      }
    ],
    "bicycle": {
      "color": "red",
      "price": 19.95
    }
  }
}
var nodes = jp.apply(data, '$..author', function(value) { return value.toUpperCase() });
console.log(nodes)

Expected:

[
{"path":["$","store","book",0, "author"],"value":"NIGEL REES"},
{"path":["$","store","book",1, "author"],"value":"EVELYN WAUGH"},
{"path":["$","store","book",2, "author"],"value":"HERMAN MELVILLE"},
{"path":["$","store","book",3, "author"],"value":"J. R. R. TOLKIEN"}
]

Actual:

[
{"path":["$","store","book",0],"value":"NIGEL REES"},
{"path":["$","store","book",1],"value":"EVELYN WAUGH"},
{"path":["$","store","book",2],"value":"HERMAN MELVILLE"},
{"path":["$","store","book",3],"value":"J. R. R. TOLKIEN"}
]

Runkit example: https://runkit.com/psygik/6538cdcc485c4f000831f0dc

apiwat-chantawibul commented 10 months ago

can confirm. about to write the same bug report.

apiwat-chantawibul commented 10 months ago

might have something to do with the use of pop in this line:

https://github.com/dchester/jsonpath/blob/c1dd8ec74034fb0375233abb5fdbec51ac317b4b/lib/index.js#L42

apiwat-chantawibul commented 10 months ago

This hot fix seems to work. Need reviews though, I just learn nodejs 2 weeks ago.

var assert = require('assert');
var jp = require('jsonpath');

jp.apply = function(obj, string, fn) {

  assert.ok(obj instanceof Object, "obj needs to be an object");
  assert.ok(string, "we need a path");
  assert.equal(typeof fn, "function", "fn needs to be function")

  var nodes = this.nodes(obj, string).sort(function(a, b) {
    // sort nodes so we apply from the bottom up
    return b.path.length - a.path.length;
  });

  nodes.forEach(function(node) {
    var key = node.path.slice(-1);
    var parent = this.value(obj, this.stringify(node.path.slice(0, -1)));
    var val = node.value = fn.call(obj, parent[key]);
    parent[key] = val;
  }, this);

  return nodes;
}
PsyGik commented 10 months ago

you can update the tests https://github.com/dchester/jsonpath/blob/master/test/query.js to ensure it works?

apiwat-chantawibul commented 10 months ago

you can update the tests https://github.com/dchester/jsonpath/blob/master/test/query.js to ensure it works?

Sure, but after looking around, I think https://github.com/dchester/jsonpath/blob/master/test/sugar.js might be a better place. The rationale is jp.apply probably counts as a syntactic sugar function. Plus, there already exists some other tests of jp.apply in the /test/sugar.js

would you agree?

apiwat-chantawibul commented 10 months ago

One more thing @PsyGik, I just noticed that the expected result in your opening post does not match the behaviour of jp.apply describe in README.

Specifically, the document said jp.apply should "Returns matching nodes with their updated values". Your opening post implies you expect the returned values to be the original values.

I'm going to stick with the README for now.

PsyGik commented 10 months ago

One more thing @PsyGik, I just noticed that the expected result in your opening post does not match the behaviour of jp.apply describe in README.

Specifically, the document said jp.apply should "Returns matching nodes with their updated values". Your opening post implies you expect the returned values to be the original values.

I'm going to stick with the README for now.

Yeah you are right. The values should have been formatted to uppercase. I was focusing on the main issue i.e missing the last key from the paths array