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

JSONPath parser is not parsing ?(@) conditions correctly #1591

Closed
Skarlso opened this issue Jul 19, 2016 · 17 comments
Closed

JSONPath parser is not parsing ?(@) conditions correctly #1591

Skarlso opened this issue Jul 19, 2016 · 17 comments

Comments

@Skarlso
Copy link
Contributor

Skarlso commented Jul 19, 2016

Hi.

Here is an interesting bug... I'm trying to create a reddit agent which lists the last 25 submits, but only those who are voted above 0. For this, I'm using this simple JSON from reddit: https://www.reddit.com/r/golang/new.json?sort=new

I can't use XML because reddit's XML api doesn't have ups in it. Which is a bit stupid. But that's how things are.

I'm trying to use this as an agent definition:

{
  "expected_update_period_in_days": 2,
  "url": "https://www.reddit.com/r/golang/new.json?sort=new",
  "type": "json",
  "mode": "on_change",
  "extract": {
    "title": {
      "path": "$.data.children[*].data[?(@.ups > 0)].title"
    }
  }
}

Which, according to JSONPath definition, should work. However, it doesn't return anything. But get this... Neither do many of the online parsers, except for this one: http://jsonpath.herokuapp.com/?path=$.store.book[?(@.price%20%3C%2010)] Which turns out to be a correct way of implementing JSONPath. It's implementation is located here: https://github.com/jayway/JsonPath

So... can we switch out the parser, or at least fix it so it interprets conditions correctly?

Thanks!

Note: Eval is enabled. I'm not getting anything back, not, getting an evaluation error.

@cantino
Copy link
Member

cantino commented Jul 20, 2016

Hey @Skarlso. We use this implementation. It looks like 0.5.8 is out, which has a bunch of new code; you could try updating the Gemfile to 0.5.8 and running bundle and seeing if it works. Also, does @['ups'] work any better?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

Hi @cantino. I shall try. :)

You mean [?(@['ups'] > 0)]?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

Nope, the gem update didn't help. :/

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

So, here's an interesting thing. This works "path": "$.data.children[*].data.ups[?(@ > 0)]".

However.. Now, how do I get title from this object? :) Since ups[?(@ > 0)].title won't work, because title is part of data. So basically, eval on the current object, like @ work, however, it can't get children's of the current object @. :(

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

Oh, that explains it -> joshbuddy/jsonpath#38 :/

So, the gem hasn't been touched in a LONG while. I'll try to take over curation of it, maybe I can make a new version of it, with the PRs that have been pending for it. In the meantime, I'll create my own version and will try to use that with Huginn.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

It works! :) Implementation here -> https://github.com/Skarlso/jsonpath

I'm not saying you should use this one.... heh. I'll try to get ownership of the gem if that's possible and update it. The code is quiet... Huh... To be honest. We'll see.

@cantino
Copy link
Member

cantino commented Jul 20, 2016

Nice, glad you found a fix! Let's see if joshbuddy responds. If not, we can switch to your fork.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 20, 2016

Niceee. Thanks! :)

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 21, 2016

Hey @cantino. I don't think he will answer. He's been away for a very long time now and clearly abandoned that gem 2-3 years ago. :/

So.... I took the liberty and created my own cut. I fixed a BUNCH of stuff in the code, refactored a lot in there, fixed a couple of warnings, and fixed the ?(@) issue. The code is like... FAR from done, but I intend to either re-write it complete, or fix it where I can.

Here is the published Gem -> https://rubygems.org/gems/jsonpathv2.

If you'd prefer a different name for it all together, shoot. But I... sort of wanted to keep it as a tribute. He did write the thing. :) I have tests and a travis build green for 2.0.0, 2.1.6 and 2.3.1. What are huginn's minimum version requirements?

@dsander
Copy link
Collaborator

dsander commented Jul 21, 2016

@Skarlso That sounds great! I think it would be a good idea to keep the old JsonPath class name, that would allow us to only switch the new gem without having to change any code. Do you see any downsides in keeping the old namespace?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 21, 2016

Uh! That is a good catch! No, there shouldn't be any problems with it. I'll change that in a moment.

In the meantime, my travis build is failing for ruby 1.9.3. I read about it, and it seems as though it's a rubygem, gem problem. :/ I'm trying to use gem update --system 2.3.0 as suggested in an issue, but it still doesn't work. It can't install a native gem, gem install json, for ruby 1.9.3.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 21, 2016

Okay, I fixed the namespace and republished with 0.0.2. 👍 Ahh... the requires. They are still v2... Give me a moment. I have to dig around if that is a good idea to leave in. I think it shouldn't cause any problems really. My only concern is, that the filename should reflect the gem name. And that's where the require is coming from. So you might have to update the require statements.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 21, 2016

You only seem to have require 'jsonpath' in one place in utils.rb. So, you should be okay re-writing that to require 'jsonpathv2'. And that's all you have to do. :) And ofc in the Gemfile.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 21, 2016

Ps.: I've run all the specs in huginn after I modified to use jsonpathv2, and everything's green. 👍 :)

@dsander
Copy link
Collaborator

dsander commented Jul 21, 2016

Nice! I think you could get around the require problem (both for Huginn and everyone who wants o switch to your version) by adding a jsonpath.rb file which only requires jsonpathv2. Not sure why we have the require 'jsonpath' in utils.rb, it should work without it due to the auto require of bundler.

We do not care about ruby 1.9.3 support, Huginn only supports ruby 2.1+ as everything else is EOL.

Switching to your version would be great as long as it does not have any breaking changes. Do you mind opening a pull request?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 22, 2016

Not at atll! I don't mind. That is excellent news! ( That you don't support that old version. :) ). I'll make the change which you suggested, than create a PR. Thanks! I'll close this ticket with that PR.

cantino pushed a commit that referenced this issue Jul 24, 2016
@Skarlso
Copy link
Contributor Author

Skarlso commented Jul 24, 2016

Done! Thanks! :)

@Skarlso Skarlso closed this as completed Jul 24, 2016
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

3 participants