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

Replace github.com/yalp/jsonpath with github.com/PaesslerAG/jsonpath #49

Closed
wants to merge 1 commit into from

Conversation

sjlee
Copy link

@sjlee sjlee commented Jan 30, 2018

Fix #48.

It replaces the github.com/yalp/jsonpath dependency which does not have a license with github.com/PaesslerAG/jsonpath.

@coveralls
Copy link
Collaborator

coveralls commented Jan 30, 2018

Coverage Status

Coverage remained the same at 97.198% when pulling 5dc8e9f on sjlee:jsonpath into c44a6d7 on gavv:master.

@gavv
Copy link
Owner

gavv commented Feb 7, 2018

Thanks. LGTM, but I think I need to test it a bit before merging to ensure that we don't break compatibility.

@gavv gavv added this to the v1 milestone Feb 7, 2018
@sjlee
Copy link
Author

sjlee commented Feb 7, 2018

Thanks. Absolutely.

@gavv
Copy link
Owner

gavv commented Jul 29, 2018

Hi, sorry for delay.

I've added some tests for jsonpath (based on original yalp/jsonpath tests):
https://github.com/gavv/httpexpect/blob/master/value_test.go#L417

Unfortunately, this small PR breaks the compatibility.

Below you can find the list of cases where yalp/jsonpath and PaesslerAG/jsonpath work differently.

Test JSON:

{
    "A":["string",23.3,3,true,false,null],
    "B":"value",
    "C":3.14,
    "D":{
        "C":3.1415,
        "V":["string2",{"C":3.141592}]
    }
}
special -1 index

expression:
$.A[:-1]

yalp/jsonpath:
["string", 23.3, 3.0, true, false]

PaesslerAG/jsonpath:
[]
unquoted key

expression:
$[A][0]

yalp/jsonpath:
"string"

PaesslerAG/jsonpath:
null
unquoted key

expression:
$[C,B]

yalp/jsonpath:
[3.14, "value"]

PaesslerAG/jsonpath:
[]
recursive search

expression:
$..C

yalp/jsonpath:
[3.1415, 3.141592]

PaesslerAG/jsonpath:
[3.14, 3.1415, 3.141592]
recursive search

expression:
$..["C"]

yalp/jsonpath:
[3.1415, 3.141592]

PaesslerAG/jsonpath:
[3.14, 3.1415, 3.141592]

You can check these cases using this online tester: https://codebeautify.org/jsonpath-tester.

So what we have so far:

  • PaesslerAG/jsonpath doesn't handle special -1 index
  • PaesslerAG/jsonpath doesn't handle unquoted keys
  • yalp/jsonpath has a bug with recursive search

I'm not sure if the first two features are standard, but if we reject them we will break the compatibility.

So we have several options:

  • stay on yalp/jsonpath (it seems that it has the LICENSE file now)
  • fix PaesslerAG/jsonpath before switching to it
  • add a possibility to switch the JSONPath engine (but this is an overkill I think)

@gavv
Copy link
Owner

gavv commented Jul 29, 2018

@gavv
Copy link
Owner

gavv commented Aug 3, 2018

All bugs I've found in yalp/jsonpath are now fixed in upstream. Since it also has a LICENSE file, I guess there is no urgency now to make a switch.

On the other hand, all important JSONPath features are now covered with httpexpect tests, so making a switch would be safe.

Switching to PaesslerAG/jsonpath makes sense since it seems to be a more complete implementation. However, compatibility issues should be addressed first. If someone is interested, pull requests are welcome.

@gavv
Copy link
Owner

gavv commented Apr 18, 2019

Closing this for now.

@gavv gavv closed this Apr 18, 2019
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