JSONPointer should not process reverse solidus or double-quote chars in tokens

[fossterer]

1. As never defined in https://tools.ietf.org/html/rfc6901#section-3 and
2. as discussed in JSONPointer: Encoding quotes not required? #563,

backslashes(\) and quotes(") are not to be treated any special by JSONPointer

[stleary]

What problem does this code solve?
This PR claims that the JSONPointer implementation does not follow the spec RFC-6901

Risks
High This code has been in place for several years

Changes to the API?
API behavior is changed

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
Yes

Was any code refactored in this commit?
No

Review status
APPROVED
Following a truncated 7 day window for comments, with concurrence of original author.

[stleary]

@erosb @jimblackler @johnjaylward Please comment if you have any thoughts or concerns.

[johnjaylward]

Section 5 of the RFC states:

https://tools.ietf.org/html/rfc6901#section-5

5. JSON String Representation

A JSON Pointer can be represented in a JSON string value. Per
[RFC4627], Section 2.5, all instances of quotation mark '"' (%x22),
reverse solidus '' (%x5C), and control (%x00-1F) characters MUST be
escaped.

Note that before processing a JSON string as a JSON Pointer,
backslash escape sequences must be unescaped.

So, I take this to mean that the current implementation may be correct.

See the continuation of section 5 for a test document and test cases.

[johnjaylward]

@fossterer , can you add an explicit test case that matches the ones in the RFC?

Given the document here: https://github.com/stleary/JSON-java/blob/master/src/test/resources/jsonpointer-testdoc.json (which I believe matches the RFC at a glance)

Test JSON Pointer	Expected Result
""	// the whole document
"/foo"	["bar", "baz"]
"/foo/0"	"bar"
"/"	0
"/a~1b"	1
"/c%d"	2
"/e^f"	3
"/i\\j"	5
"/k\"l"	6
"/ "	7
"/m~0n"	8

And this one (because GitHub doesn't like the | in table values)

• "/g|h" -> 4

Test JSON Pointer (URI Fragment)	Expected Result
#	// the whole document
#/foo	["bar", "baz"]
#/foo/0	"bar"
#/	0
#/a~1b	1
#/c%25d	2
#/e%5Ef	3
#/g%7Ch	4
#/i%5Cj	5
#/k%22l	6
#/%20	7
#/m~0n	8

The paths are expressed as JSON strings, not Java strings. so "/k\"l". However, going by the test example for the URI fragments, I believe you can leave them as-is. i.e. The key for the value 5 should be the actual string i\j and the key for the value 6 should be the exact string k"l

[johnjaylward]

Update my previous comment.

Also @fossterer, if some of these test cases already exist, can you confirm that they are working as expected?

[johnjaylward]

linking original implementation discussion: #218, #222 and #292

[johnjaylward]

OK, after reading through the comments by @jimblackler in #563 and re-reading the RFC section 5 a few times, I'm reaching the conclusion that the escape and unescape functions in JSONPointer are wrong, and this PR corrects them.

Test cases like this one:

@Test
    public void quotationEscaping() {
        assertSame(document.get("k\"l"), query("/k\\\\\\\"l"));
    }

are essentially double escaping the "query" string. The RFC states that the query only needs to be escaped when stored as JSON. For the purpose of the test, the query is not a JSON String and is instead a Java String, and should not have the escaping applied.

The above test should be changed to (as provided in this PR):

@Test
    public void quotationEscaping() {
        assertSame(document.get("k\"l"), query("/k\"l"));
    }

As such, I'm giving my stamp of approval for this PR and I believe it matches the "the bug is worse than the fix" requirement.

[johnjaylward]

@stleary can we try to get this in the next release too? @jimblackler has been waiting on it for a while now.

[stleary]

We can include it in the release at the end of this week. I would still like to give @erosb a few more days to respond, as he is the original JSONPointer author.

[erosb]

Agreed, based on section 3 and 5 of RFC 6901, this is the correct behavior. Thanks for the fix.

[stleary]

Merging the code but someone needs to complete the unit tests per the comments in this PR, before the next release.

[fossterer]

@stleary All the tests indicated by @johnjaylward exist already in the test suite!

Reading from the highlighted line you can spot all the m~, k\"l, foo etc. cases

[johnjaylward]

Thanks for taking the time to double check.

[stleary]

@stleary All the tests indicated by @johnjaylward exist already in the test suite!

Reading from the highlighted line you can spot all the m~, k\"l, foo etc. cases

Thanks for pointing this out, I will confirm.

[stleary]

Some of the tests exist, some do not. Most of the tests do not clearly call out the expected result. For example, if both document.get() and query() return the same incorrect result, the test will not catch it. Will open an issue to track this to completion.