Numeric keys in object break json pointer

[avalanche123]

https://github.com/josephburnett/jd/blob/master/v2/pointer.go#L50-L52 seems to be the culprit.

According to the JSON Pointer RFC:

If the currently referenced value is a JSON object, the new
referenced value is the object member with the name identified by
the reference token. The member name is equal to the token if it
has the same number of Unicode characters as the token and their
code points are byte-by-byte equal. No Unicode character
normalization is performed. If a referenced member name is not
unique in an object, the member that is referenced is undefined,
and evaluation fails (see below)

Which I read as, in the context of an object, a json pointer obviously refers to object keys, and object are allowed to have numeric keys, as weird as that is. I think this extra check is unnecessary. This is an example path that is failing:

[]github.com/josephburnett/jd/lib.JsonNode len: 10, cap: 10, ["definitions","items",33,"resources","items",0,"json","spec","sequenceNumberForAllocation","2"]

And according to the CRD, it's expected to be a number - https://github.com/projectcalico/calico/blob/master/manifests/calico.yaml#L2928-L2933.

Please let me know if this report makes sense and if you'd like my help removing that check.

[josephburnett]

Thanks for the report @avalanche123!

You're right that object keys can include just number characters. I think that error message of mine is poorly phrased. JSON Pointer does support this. The problem I encountered is that the elements of a JSON Pointer are untyped. So when I read 0 I don't know if that's supposed to be a map key or an array index. This is one of the reasons my paths are plain JSON, to make this clear.

When applying a patch, sometimes the object or array must be created in order to "inject" the new value. So we can't necessarily rely on the existing structure to tell us how that bare 0 should be interpreted. Should we create an array or an object? And sometimes the value we encounter mid-path exists but is the wrong type, which should cause the hunk to be rejected. So jd must know the type of each element.

Can you tell me more about your use case? Are you just making a diff? Or are you generating an RFC patch? I would like to know if this problem ☝️ is relevant to you. If it's not, maybe there is a way we can be more generous in our interpretation without opening edge cases that cause other bugs.

[avalanche123]

Thanks for getting back to me @josephburnett!

I'm using jd to generate json patch diffs while watching a CRD. In this case calico CRD can sometimes lead to this error when the above referenced property changes.

I can see how it might be ambiguous when applying a patch, like when a nonexistent index is provided, which can mean append or set. However, due to my ignorance, I'm not sure I understand the situation where this ambiguity might arise. Mustn't we always have an existing object or array that we're patching? Are there cases where this is not true?

Looking forward to learning more!

[josephburnett]

Consider these two files:

{"foo":"bar","sequence":{"2":3}}

{"foo":"bar","sequence":{"0":1,"2":3}}

Their diff would be as follows:

@ ["sequence","0"]
+ 1

When applying this ☝️ diff to the follow file:

{"foo":"bar"}

We (should) get the following result:

{"foo":"bar","sequence":{"0":1}}

(I see that the current version 1.7.1 throws an error which is a bug, but this used to work)

The fact that the second path element is "0" instead of 0 tells jd that it should create a object. If the patch was this instead:

@ ["sequence",0]
+ 1

Then this would be the result:

{"foo":"bar","sequence":[1]}

Because the second element was a number, jd created an array instead.

Both of those diffs would look like this as a JSON Patch (RFC 6902):

[{"op":"add","path":"/sequence/0","value":1}]

Applying this patch to {"foo":"bar"} would give ambiguous results so jd refuses to produce that patch. That's the reason for the check you're running into.

Maybe that's too strict. Maybe we should only throw an error if we encounter the ambiguous situation during patching.

[avalanche123]

Oh, now I understand. And I agree, if we considered this an error at patch time, this would be perfect. If we always patch the original and not a similar looking document, we shouldn't get any errors.

[josephburnett]

@avalanche123 how do you get jd? Do you get it from a package manager, from the downloads on GitHub, by building from source, or use it as a golang library? I want to know in order to figure the best way to get a fix to you.

[avalanche123]

Thanks @josephburnett, I get it as a go mod download.

[josephburnett]

Hey @avalanche123, I've finally gotten around to implementing this. I created a new type jsonStringOrInteger which is only created when encountering a number in a JSON Pointer. When indexing into an object it behaves like a string. When indexing into a list it behaves like a number.

And I just released it as v1.8.1. Will you try it out? Hopefully you won't see this error anymore. Once you confirm it works for you, I'll close this issue.