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

JSON Pointer implementation #222

Merged
merged 11 commits into from
May 14, 2016
Merged

JSON Pointer implementation #222

merged 11 commits into from
May 14, 2016

Conversation

erosb
Copy link
Contributor

@erosb erosb commented Apr 26, 2016

See #218 for details

@johnjaylward
Copy link
Contributor

This looks like a nice simple implementation to me. I have not looked at the test cases to see if they cover the different pointer types available.

@erosb can you also update the README to indicate which RFC this is implementing?

@erosb
Copy link
Contributor Author

erosb commented Apr 28, 2016

@johnjaylward I've just added it in cbb1546

@stleary
Copy link
Owner

stleary commented Apr 29, 2016

What problem does this code solve?
This is a new feature for JSON-Java. It implements JsonPointer as specified in https://tools.ietf.org/html/rfc6901.

Changes to the API?
Yes. 2 new public methods are defined:

  • Object JSONObject.query(String)
  • Object JSONArray.query(String)

2 new classes are also defined:

  • JSONPointer
  • JSONPointerException

Does it break the unit tests?
No, but new unit tests will be added after this commit. Those tests will fail unless this code is committed. See stleary/JSON-Java-unit-test#46.

Will this require a new release?
No, but this will be rolled into the next release, which is expected in May 2016.

Should the documentation be updated?
Yes. The JavaDoc and the README should be updated.

@johnjaylward
Copy link
Contributor

johnjaylward commented Apr 29, 2016

I finally got around to looking at the test cases and saw this:

assertSame(document.get("c%d"), query("#/c%25d"));

assertSame(document.get("k\"l"), query("/k\\\\\\\"l"));

I was wondering if it made sense to provider a "builder" where you can add parts. Something like this maybe:

JSONPointerBuilder jpb = new JSONPointerBuilder(); // will return the whole document
jpb.append("c%d");

the append method would return this like any normal builder so you can chain the commands.

So the example from the RFC "/foo/0" would look something like this with a builder:

JSONPointer jp = new JSONPointerBuilder().append("foo").append(0).compile();
// this would compile to "/foo/0"

maybe this would be better in a separate pull request?

I was also thinking that being able to get the path back from a JsonPointer may be useful

jp.toString(); // should return the normalized non-URI path "/c%d"
jp.toURIPath(); // should return the uri encoded path "#/c%25d"

@erosb
Copy link
Contributor Author

erosb commented May 3, 2016

@johnjaylward @stleary I've just updated the PR according to the above suggestions.

}
}

public JSONPointer(List<String> refTokens) {
Copy link
Contributor

@johnjaylward johnjaylward May 3, 2016

Choose a reason for hiding this comment

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

This should either be a protected/private/or default constructor, and the List should be copied so the Pointer can't be modified outside of the class.

this.refTokens = refTokens.clone();

as it is now, I could do this:

Builder b = JSONPointer.builder().append("key1");
JSONPointer jp1 = b.build();
b.append("key2");
JSONPointer jp2 = b.build();
if(jp1.toString().equals(jp2.toString()){
    throw new Exception("Oops, my pointers are sharing a backing array");
}

@erosb
Copy link
Contributor Author

erosb commented May 5, 2016

@johnjaylward good point, I've just added token list copying to the constructor. I kept it public though, it might be useful for somebody creating the reference token list in a dynamic way.

@erosb
Copy link
Contributor Author

erosb commented May 13, 2016

@stleary will you merge it in the next release?

@stleary
Copy link
Owner

stleary commented May 13, 2016

@erosb thanks for being patient. No review comments or code changes in over a week. No cautions about adding code that is not central to the purpose of the app. Still need to come up with some content for the readme. Will do a final code review and test run, then should be ready for the merge. Should be able to get to this over the weekend.

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