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

Fix bug with integer keys #8

Merged
merged 5 commits into from
Feb 6, 2020

Conversation

AntoineGagne
Copy link
Contributor

@AntoineGagne AntoineGagne commented Feb 3, 2020

Previously, a query such as $.3 would fail. This PR fixes this problem.

This also fixes one of the issues in #6.

@seriyps
Copy link
Contributor

seriyps commented Feb 3, 2020

I'm not sure it's the best solution.
I tried following expression $.phoneNumbers.1.type on this page http://jsonpath.com/ and it returns home. So, at least for JavaScript implementation integer key in path may work as subscription operator [1]. So, $.phoneNumbers.1.type and $.phoneNumbers[1].type are equivalent if $.phoneNumbers is array.
My suggestion is to implement this in evaluator, not parser.

Should be somewhere around here:

https://github.com/ostrovok-team/ejsonpath/blob/b9ef15d6d85966f27685e78c97ea5008f6f61bb9/src/ejsonpath_eval.erl#L71-L72

And here as well (for transform feature to work):

https://github.com/ostrovok-team/ejsonpath/blob/b9ef15d6d85966f27685e78c97ea5008f6f61bb9/src/ejsonpath_transform.erl#L54

@AntoineGagne
Copy link
Contributor Author

Hmm... I didn't know that this worked on lists too. However, I don't see how the parser can be left alone if the integer key has to be supported for an object because it seems as if the lexer tokenizes a key if it starts with a letter and the parser doesn't seem to know how to parse a dot followed by a number at the moment (please correct me if I have missed something).

I'd be willing to change my PR to work with lists though by modifiying the files you said though!

@seriyps
Copy link
Contributor

seriyps commented Feb 4, 2020

I mean, it should be ok to add key_predicate -> int : {predicate, {key, value('$1')}}. but it's better to not convert it to binary inside the parser but do this in evaluator / transformator based on what kind of objects this key matches on: leave is as integer if we are on array and convert to binary if we aro on hash

Copy link
Contributor

@seriyps seriyps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks! Ping @vanadium23

@vanadium23 vanadium23 merged commit 056b15b into ostrovok-tech:master Feb 6, 2020
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

Successfully merging this pull request may close these issues.

3 participants