Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.apply skips the last path #183

Open
PsyGik opened this issue Oct 25, 2023 · 7 comments
Open

.apply skips the last path #183

PsyGik opened this issue Oct 25, 2023 · 7 comments

Comments

@PsyGik
Copy link

PsyGik commented Oct 25, 2023

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
Copy link

can confirm. about to write the same bug report.

@apiwat-chantawibul
Copy link

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

var key = node.path.pop();

@apiwat-chantawibul
Copy link

apiwat-chantawibul commented Nov 8, 2023

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
Copy link
Author

PsyGik commented Nov 8, 2023

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

@apiwat-chantawibul
Copy link

apiwat-chantawibul commented Nov 9, 2023

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
Copy link

apiwat-chantawibul commented Nov 9, 2023

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.

apiwat-chantawibul added a commit to apiwat-chantawibul/jsonpath that referenced this issue Nov 9, 2023
A test describing expected behaviour has also been added.
@PsyGik
Copy link
Author

PsyGik commented Nov 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants