Support for JSON Pointer

[glassfishrobot]

Consider adding support for JSON Pointer. The RFC is at http://tools.ietf.org/html/rfc6901

[glassfishrobot]

Reported by jitu

[glassfishrobot]

asotobu said: JSON Pointer is really a must for next release of JSR 374 because without them it is going to be pretty hard for example implementing JSON Patch feature. In fact because JSON Pointer is required for implementing Json Patch we should discuss this first.

I have collaborated with Jackson guys implementing Json Pointer support so it can be used in Json Patch implementation. Although now I am going to mention something of low level implementation I prefer to note now so I don't miss it later.

Json Pointer allows developers to get some part of a json document (something like x-path for xml). But you also need some operations to manage json pointers themselves. In this sense next management operations are required for json patch implementation.

• last() -> Returns the leaf of current JSON Pointer expression. Leaf is the last non-null segment of current JSON Pointer.
• tail() -> Accessor for getting a "sub-pointer", instance where current segment has been removed and pointer includes rest of segments. For matching state, will return null.
• head() -> Accessor for getting a pointer instance that is identical to this instance except that the last segment has been dropped. For example, for JSON Point "/root/branch/leaf", this method would return pointer "/root/branch" (compared to tail() that would return "/branch/leaf").

Of course we would need other operations which will involve the json document and the json pointer, but last, tail and head are operations that only affects the json pointer expression.

[glassfishrobot]

kchung said: Let's up level the api design for Json Pointer a bit. There are a number of issues to consider.

1. Should JsonPointer be an interface or a concrete class? I guess the answer depends on whether we need to create an instance of it from the factory (SPI), like how it is now for JsonParser etc. I think it would be simpler if we can just do a new JsonPointer("q/b/c/d") to create one.

2. Since a Json Pointer refers to a Json Text, should the text be part of JsonPointer? If not, the text will need to be applied when we do operations with the pointer. In other words, which of the following two scenarios is better:``` JsonObject target = ... ; # the target Json text JsonPointer pointer = new JsonPointer("a/b/c/d"); JsonValue value = pointer.get(target); # get the value referenced

   
   or
   

   JsonObject target = ...; # the target Json text JsonPointer pointer = new JsonPointer(target, "a/b/c/d"); JsonValue value = pointer.get(); # get the value referenced

   
   
   One might argument the the first is better since the same pointer can be applied to other text. but I am not sure if that is useful in practice.

3. What other operations can we do with a pointer? To support Json Patch, we should have at least the following:

   1. get the value at the reference location
   2. add a new value to the reference location
   3. remove the value at the reference location
   4. replace the value at the reference location with another value

[glassfishrobot]

asotobu said:

• Point 1) I think JsonPointer could be a concrete class, it is much simpler and I think JsonPointer is generic enough to be provided from spec.
• Point 2) First approach feels more natural to me and less restrictive in the sense that if devs want they can reuse a JsonPointer object to be used on other json objects, as noted maybe it is not useful but if we use the second one then we are not giving this freedom to developers, so I vote for the first one approach.

Only to considerations. The first one is that I think it could be better to no offer the constructor to devs but an static method. I said this because there is some rules a json pointer should follow (for example it should start with a '/') and also the first thing to do in a JsonPointer is evaluating and decoding and escaped char. It may look strange to do all this kind of stuff in a contructor, so I think that an static method that uses at the end a protected constructor may be better. something like:

JsonPointer.compile("/a/b/c");

and/or

JsonPointer.valueOf("/a/b/c");

And the second point is should be the operation of get/add/remove/... be a part of the JsonPointer or a part of JsonObject.

JsonObject target = ... ; # the target Json text JsonPointer pointer = new JsonPointer("a/b/c/d"); JsonValue value = pointer.get(target); # get the value referenced

or

JsonObject node = ... ; # the target Json text JsonPointer pointer = new JsonPointer("a/b/c/d"); JsonValue value = node.get(pointer); # get the value referenced

I think both are valid and I have used both approaches one in Jackson and another one in one project that implemented Json Pointer before it was done in Jackson. For me and it is something personal I feel more comfortable of having operations on node instead of pointer. node.get(pointer). But it is something personal.

[glassfishrobot]

eugen.cepoi said: About last/tail/head, I think the names might be confusing. Peoples are used to them in the context of collections.

1) I agree, I would prefer to see json pointer implemented in the spec.

2) I also prefer option 1, but would like to see shortcut methods on json value. So people can do stuff like JsonObject person = ... person.at("address/street").asString()

How do you plan to handle incomplete paths (when some element in the path matches a null object)?

3) In case of mutation, I guess it would return a new instance of the root object on which the operation was applied? As for 2) would be nice to have shortcuts at node level.

[glassfishrobot]

asotobu said: Well yes about giving a shortcut to users so you can do something like:

person.at("")

About incomplete paths, the RFC mention that there is no "explicit" paths as described here.

Look section 3 of RFC:

The ABNF syntax of a JSON Pointer is:

      json-pointer    = *( "/" reference-token )
      reference-token = *( unescaped / escaped )
      unescaped       = %x00-2E / %x30-7D / %x7F-10FFFF
         ; %x2F ('/') and %x7E ('~') are excluded from 'unescaped'
      escaped         = "~" ( "0" / "1" )
        ; representing '~' and '/', respectively

   It is an error condition if a JSON Pointer value does not conform to
   this syntax (see Section 7).

See that a pointer must start with '/'. So the example you mention above should be:

person.at("/address/street").asString()

[glassfishrobot]

eugen.cepoi said: Sorry I was unclear with "How do you plan to handle incomplete paths (when some element in the path matches a null object)?"

What I mean is not, how we handle a malformed path it self, but what will be the result of a json pointer expression when it encounters a null/missing value when evaluating the path? Note that null and missing could have different semantics. Would we return null? Optional? Or do something else?

[glassfishrobot]

asotobu said: I have read what Jackson-Utils JsonPointer and Jackson-databind JsonPointer solves this problem.

The first one returns a Null which as you said it may be incorrect because a value can be null as well.

The second one returns an special JsonObject which has a method called isMissingNode() that returns true in this case, meaning that there is no node there.

If you see javadoc JsonValue class has a NULL constant (JsonValue.NULL) https://json-processing-spec.java.net/nonav/releases/1.0/pfd-draft/javadocs/javax/json/JsonValue.html so what we can do is if returns a Java null means missing value and if it returns a JsonValue.NULL means the value Null.

[glassfishrobot]

kchung said: Yes, a Json pointer should begin with a '/'. My bad.

@asotobu I still like constructors better, because it is what most people would expect to use, and using static method to instantiate is less intuitive. There is nothing wrong with doing some processing in constructors. They can also be processed lazily.

Agreed that the operations starting from the target

person.get(pointer)

can be an alternate syntax. The short cut

person.get("/address/street")

also makes sense.

Here is the question. If we can use this short cut all the time, do we still need the class JsonPointer? Here are exaples for each of the 4 operations, just to see how they look like:

JsonValue value = person.get("/address/street");
  JsonObject p = person.replace("/address/street". "Main Street"); // return the transformed person
  JsonObject p = person.remove("/phones/office");
  JsonObject p = person.add("/phones/mobil", "1234-567");

This way, the Json pointer will be completely removed from the API and is hidden in the implementation. Are there things that can be done with the two step approach (first create a JsonPointer, then apply to a target) but not with the short-cut?

Regarding the question about what should be returned when pointer specifies a non-existing entry, I think spec says an error should be raised. See http://tools.ietf.org/html/rfc6901#section-7

[glassfishrobot]

mitemitreski said: +1 on hiding the implementation details since really I don't see the need to know that part.

The shortcut also looks cleaner then the 2-step approach.

Just wondering how would we retrieve a collection of values from a collection of pointers applied on a object?

Or specifically how would a collection of operations would be applied to an object dynamically without knowing the order of the operations. I am asking this since this would be needed in https://tools.ietf.org/html/rfc6902

[glassfishrobot]

asotobu said: @kchung and @mitemitreski From my point of view I would maintain the JsonPointer class in spec. Let me show why. A JsonPointer expression requires two things when it is created/used; the first one is escaping special characters and the second one (and this is an implementation details but all implementations I have worked of JsonPointer uses) it is that a json pointer expression is tokenized (or segmented) so in this way it is easier to implement the JsonPointer logic and also some operations required by Json Patch.

So let's suppose next scenario. I have 2000 request per minute which every request uses the same JsonPointer. This means that every minute you will execute 2000 times the escaping the same chunk of string and tokenize the expression. Although these are not an expensive operations it is unnecessary to execute them each time if the business logic is exactly the same.

So if spec allows to pass a JsonPointer then you will be able to create an static JsonPointer expression and call person.get(staticJsonPointer);

Of course we can still offer the string method which internally creates a new JsonPointer object, but at least we give to users the option to create only once the expression.

[glassfishrobot]

kchung said: @asotobu I don't get your argument about extra processing are need for a Json pointer. Isn't that true anyway? This is just an implementation detail, right?

I still don't know how often a Json pointer is reused. In your example, if the Json pointer is part of the request (maybe part of a Json patch). You'll need to do a comparison to determine if two pointers are same. To me it almost looks like you need a cache of JsonPointer. Note also the same optimization can be realized for the short-cut, because it can also look up the cache to reuse a previous JsonPointer.

I am not ruling out JsonPointer. I am just exploring what a JSON Pointer API we need to have. If we can make it simpler, the better.

We don't need to make decisions right now. We should have better ideas when we discuss/implement JSON patch.

[glassfishrobot]

asotobu said: Well let me explain with code. If you look here https://github.com/FasterXML/jackson-core/blob/master/src/main/java/com/fasterxml/jackson/core/JsonPointer.java#L114 you will see the process that requires creating a JsonPointer instance in Jackson. But I have used other Java libraries that implements JsonPointer and it is pretty similar. So as you can see there is some logic like escaping characters and creating segments for each sub-expression.

This is not good or bad it is simply an implementation details I know but let's suppose next code:

JsonObject[] persons = new JsonObject[5000];
//fill persons JsonObject[] addresses = new JsonObject[5000]; 

for(int i=0;i<persons.length;i++) {
  addresses[i] = persons[i].get("/personalinformation/address");
}

In that code the escaping characters and segmentation of json pointer expression will be evaluated 5000 times.

But if I could do something like:

JsonObject[] persons = new JsonObject[5000];
//fill persons JsonObject[] addresses = new JsonObject[5000]; 

JsonPointer addressPointer = new JsonPointer("/personalinformation/address");
for(int i=0;i<persons.length;i++) {
  addresses[i] = persons[i].get(addressPointer);
}

Then construction would be only one.

Because we don't know exactly if reusing a jsonpointer would be a requirement or not, I think that would have sense to implement both operations:

• get("/a/b")
• get(new JsonPointer("/a/b");

Basically because we are getting the best of both worlds with minimal requirement. The only requirement is that JsonPointer becomes an interface of the spec.

[glassfishrobot]

kchung said: OK. Let's keep both for now. I'll summarize what we have.

1. JsonPointer is a non-abstract class with a constructor``` public JsonPointer(String pointer);

2. The syntax for applying a JsonPointer to a JsonValue is``` JsonValue value; JsonPointer pointer = new JsonPointer("/a/b/c"); value.get(pointer);

3. The JsonValue operations should be overloaded to take the raw Json pointer String as a short cut``` value.get("/a/b/c");

4. Add 4 operaions to JsonValue:

   1. getValue # there is already a get in JsonObject
   2. remove
   3. replace
   4. add

5. An error is raised if the pointer reference a non-existing value.

Do we still need the alternate syntax

pointer.get(value)

?

I'll writeup the javadocs for JsonPointer and we can talk more.

[glassfishrobot]

asotobu said: jsonPointer could be an interface as well, so implementors Can decide how to implement json pointer resolution and how to create. WDYT ?

[glassfishrobot]

kchung said: @asotobu I thought you were against using SPI to instantiate a JsonPointer, and now you have reversed your position? Just to be clear, you prefer

JsonPointer pointer = Json.createJsonPointer("/a/b/c");

to

JsonPointer pointer = new JsonPointer("/a/b/c");

But be aware that there has been complaints about the use of SPI interfaces in JSON-P, and one of our task is to simplify the API and avoid using the SPI.

Personally, either would work for me. We can assume one and proceed to firm up the API for JsonPointer. Are there methods, other than the 4 I proposed, that should be added to JsonPointer? Once we have JsonPointer API, we can move on to Json Patch.

[glassfishrobot]

asotobu said: Hi, well I was not against something I was only mention other options to have different options, but both can work and I feel comfortable. But today I have started to prototyping something and probably you are right that creating JsonPointer as SPI it can be a headache, and if our task is simplify the API for implementators then I am going to start working for JsonPointer as a class and not an interface.

About if I prefer first or second option, I like the first one but the second one works as well. If JsonPointer is a class then we can implement in both ways, because it seems that the majority thinks that it is better as a constructor I am going to implement as is.

If you agree my next steps are going to be cloning the project, create a branch and implement JsonPointer class (with some inner classes) and adding the operations to JsonObject for getting a node. All with their tests. Then if it looks ok let's implement the rest of the operations described here.

WDYT?

[glassfishrobot]

kchung said: @asotobu Go ahead with a branch for JsonPointer. We can assume the first option for now. It wouldn't be to hard to switch later if necessary.

To implement JsonPointer, you'll need my proposed API for transforming a JsonArray and JsonObject. Maybe I should merge it into the master branch first. For now let's first work on getting the API right.

[glassfishrobot]

kchung said: asotobu proposed:

/* 
* Accessor for getting a "sub-pointer", representing the rest of segments of a pointer.
* For example /person/address/street if current segment is address it will return /street json pointer
*/
public JsonPointer tail();

/**
* Accessor for getting a pointer instance that is identical to this
* instance except that the last segment has been dropped.
* For example, for JSON Point "/root/branch/leaf", this method would
* return pointer "/root/branch" (compared to {@link #tail()} that
* would return "/branch/leaf").
*/
public JsonPointer head();

/**
* Returns the leaf of current JSON Pointer expression.
* Leaf is the last non-null segment of current JSON Pointer.
*/
public JsonPointer last();

/**
* Method that may be called to see if the pointer would match property
* of a JSON Object.
* 
*/
public boolean matchesProperty(String name);
public JsonPointer matchProperty(String name);

/**
* Method that may be called to see if the pointer would match
* array element with given index.
* 
*/
public boolean matchesElement(int index);

Almost all of these methods were implemented by me in Jackson but we can change the names to something better (I am pretty bad given name to things) feel free to adapt it.

[glassfishrobot]

kchung said: @asotobu: All the methods seems to me too low level to be exposed in an API. What can an use do with a JsonPointer? I think the answer is that he/she wants to use it on some JSON structure. Here is my proposal, coming from that perspective.

/**
     * Return the value at the referenced location
     * in the specified {@code JsonStructure}
     *
     * @param target the {@code JsonStructure} referenced by this {@code JsonPointer}
     *
     * @return the {@code JsonValue} referenced by this {@code JsonPointer}
     */
    public JsonValue getValue(JsonStructure target);

    /**
     * Add or replace a value at the referenced location
     * in the specified {@code JsonStructure}.
     * <ol>
     * <li>If the reference is the target {@code JsonStructure},
     * the value, which must be the same type as the target,
     * is returned.</li>
     * <li>If the reference is an index to an array, the value is inserted
     * into the array at the index.  If the index is specified with a "-",
     * or if the index is equal to the size of the array,
     * the value is appended to the array.</li>
     * <li>If the reference is a name of a {@code JsonObject}, and the
     * referenced value exists, the value is replaced by the specified value.
     * If it does not exists, a new name/value pair is added to the object.
     * </li>
     * </ol>
     *
     * @param target the target {@code JsonStructure}
     * @param value the value to be added
     * @return the resultant JsonStructure
     * @throws IndexOutOfBoundsException if the index to the array is out of range
     */
    public JsonStructure add(JsonStructure target, JsonValue value);

    /**
     * Replace the value at the referenced location
     * in the specified {@code JsonStructure} with the specified value.
     *
     * @param target the target {@code JsonStructure}
     * @param value the value to be stored at the referenced location
     * @return the resultant JsonStructure
     * @throws JsonException if the value does not exists,
     *    or if the reference is the target.
     */
    public JsonStructure replace(JsonStructure target, JsonValue value);

    /**
     * Remove the value at the reference location
     * in the specified {@code JsonStructure}
     *
     * @param target the target {@code JsonStructure}
     * @return the resultant JsonStructure
     * @throws JsonException if the referenced value does not exists,
     *    or if the reference is the target.
     */
    public JsonStructure remove(JsonStructure target);

We should probably overload these methods to return JsonArray or JsonObject, if the target is a JsonArray or JsonObject.

[glassfishrobot]

asotobu said: Yes I agree that my approach exposes maybe too much the internals, but my approach (and probably wrong) was to not expose JsonStructure inside JsonPointer. But probably as you said for an API it would be better your approach.

Only I have next question should we add remove, replace, add... methods here? I said that because JsonPointer doesn't talk about replacing or removing elements but only as getting elements. Of course we need these operations for json patch but should be this place to implement them or they should be used in upper level let's say in json patch implementation? So they only call jsonpointer.get(path) and with returned JsonStructure they call let's say the remove method.

From my point of view JsonPointer should only provide get (that is the main purpose of the json pointer spec) and because add, remove, replace operations are also implemented in JsonObject, then the json patch API should use both classes to do the operations.

What do you think? The API would be more focused on Json Pointer and also simpler.

[glassfishrobot]

kchung said: I think your methods are not what JSON users would want or need to do with JSON Pointers. Of course if you have valid use cases, I can change my mind.

You are right that RFC 6901 only talks about get, and not add, remove, and replace. But all of them are needed by Patch, and they are useful outside of Patch (think of the scenario where you want to do a minor modification and don't want to create a JSON Patch for it). We just need a way to refer to a node in the JSON tree, and the spec provides us with the syntax. If there is not a spec for it, we may have to invent something.

I also think that it is good to keep all Pointer operations in one place. It would be confusing if we have only get in JsonPointer and others in other places.

There is not much anyone can do with a JsonPointer unless its operations are applied to a JSON structure. I have wondered before if we even need a JsonPointer, but you have convinced me that we do. If we have operations in JsonPointer, other places that use a JSON Pointer (e.g JsonPatch, JsonMergePatch, or even JsonObject etc) can just refer to them in JsonPointer. Architecturally, this approach is good.

[glassfishrobot]

asotobu said: Ok from this point of view I agree that JsonPointer can agglutinate all these kind of operations so it can be used standalone by developers or in json patch, json merge, ....

So what I am going to implement if I understand correctly is a class (not interface) that implements all these operations using the operations you implemented for transformations, isn't it?

Only one question and I think the answer is yes, when you do a replace, add or remove to a JsonStructure, does the returned structure be another JsonStructure with the change? I think that the answer is yes because if not then the JsonStructure was not immutable.

That's all if you agree I will clone the current master and I will start working on that.

Alex.

[glassfishrobot]

kchung said: Alex, +1.

[glassfishrobot]

asotobu said: Hi, I have implemented initial version of JsonPointer. For now only getValue operation is implemented to see if I have understood correctly where to put JsonPointer class and the tests and so on. (https://github.com/lordofthejars/jsr374-unofficial/blob/master/tests/src/test/java/org/glassfish/json/tests/JsonPointerTest.java)

There are some things that need to be implemented like the remaining operations, extract some constants like (~, 0 or 1) and add javadoc.

Well let's review first the work that I have done and if it all is ok then I will finish the whole implementation.

Thank you so much. Alex.

[glassfishrobot]

asotobu said: I have just implemented the remove operation with one minor change API. Basically that remove now returns a JsonValue instead of JsonStructure, but with this change I can reuse methods from getValue and add.

Then it is time for replace. I have seen that there is no replace operation in JsonArrayBuilder class. Should we add it or we can assume that a replace is a remove + add? In case of JsonObjectArray it is already implemented in add method.

That's all after this implementation I will need to refactor some parts, write more tests and start using JsonExpcetion in some places and that's all. I think that next week I will have JsonPointer implemented.

The implementation is at: https://github.com/lordofthejars/jsr374-unofficial/blob/master/api/src/main/java/javax/json/JsonPointer.java

[glassfishrobot]

kchung said: @asotobu Some general comments first.

1. You should merge the latest source for JsonPointer into you codes, including all the javadocs. It makes it easier for anyone to review you codes.
2. You should implement what we have agreed on the API, and not modify it to suit your implementation. If you think the API should be changed, we should discuss it with other EG.
3. You should be careful about following the javadocs. For instance, we know that a null JSON Pointer string is not allowed, so you don't need to test for null in add. Also, when the javadocs says JsonException should be raised, you should wrap an IllegalArgumentException in a JsoException.

OK, let's discuss the design of implementation of JsonPointer. Say we have a pointer "/a/b/c/d" on target t and we want to do an operation add, remove, replace (we won't discuss getValue, since it is easy). We just need to take the tokens in the reverse order and apply the operations to the last two nodes, and apply the replace operations as we move up the tree:

c' = c.{add,remove,replace}(d)
    b' = b.replace(c')
    a' = a.replace(b')
    t' = t.replace(a')

So if we do it right, we should handle all of these operations the same ways. To me it seems to be straight forward to remember the nodes a, b, c, and d in t, either using an array or recursion .

Maybe it is useful to abstract the execution of an operation op on a node n with a value v (n.op(v)) with an interface and use a JsonArray or JsonObject implementation, depending on the node type. That way we don't need to do a lot of type testing on the node. I'll think about this more.

[glassfishrobot]

asotobu said: Hi, about points 1, 2 and 3 I really understand what you mean and for this reason I decided to put the project in my github so I could use as sandbox not as final solution, then I can implement globally without worrying much in code conventions and so on, try things that I can share with all of you and then with something "running" we can take decisions on how API should be. Maybe I am too visual guy (I mean I can think better if I have something real to iterate). For this reason I said on my previous comment that I was totally aware that I was not following exactly the API defined in root (for example some return type or throwing IllegalArgumentException), but I think that in this way it is easier to purpose changes to the API, but in any case my intention was to change anything of the official API without consensus. I wanted to show only how using current API can be implemented the operations and see if it fits well or we can find another way that fits better (for example returning JSonValue instead of JsonStructure so we can reuse internal methods), of course if the change was an strong change then I would have updated to latest sources for sure. Note that because of this PoC of JSonPointer now we have some questions that we need to fix and that was exactly my main intention in this case. For example there are some add operations that are not implemented yet because they are pretty simple when you have the most generic one.

But also I understand that I need to change something on how I work because probably with an small more effort I can make PoC and having always the updated API.

Saying that let's see next discussion:

Yes this is exactly how I am doing by applying the operation and then replacing (which is a copy operation of previous nodes) you can see the call here https://github.com/lordofthejars/jsr374-unofficial/blob/master/api/src/main/java/javax/json/JsonPointer.java#L95. I think that using Lambdas we can make the algorithm even more generic but for now I think it is ok.

As you suggested the implementation may reacall on an if to know if it is a JsonArray or JsonObject and depending on that do one change or another.

Maybe we could create an interface that has the add and remove operations (replace can be done by removing and adding), so I can remove the next ifs

if (currentNode.getValueType() == ValueType.OBJECT)
if (currentNode.getValueType() == ValueType.ARRAY)

The only problem I see in this approach is that the interface must be common so for example it could looks like:

JsonStructure add(String name, JsonValue value);
void remove(String name);

The problem that I see is that meanwhile in case of objects the name is really the name of the property you want to add or remove, in case of arrays we must ensure to clarify to the implementators that this string is going to be a position that will need to be translated. I would try to avoid using two different interfaces one for objects and another for arrays because then we will still have the same problem of having to "cast" the instances.

[glassfishrobot]

kchung said: First, let me point out that there is a replace operation in JsonArrayBuilder, and that is "set".

The reason we have an "add" method that takes a JsonValue as a parameter and returns a JsonValue is to allow cases where the target is not a JsonArray or JsonObject (allowed now by rfc 7159) and the Json Pointer is empty. Strangely, RFC 6902 only allow empty pointer for add. I think we should add an "add" method that take a JsonStructure and returns a JsonStructure, just like other methods, and special case empty json pointer.

I think your code is fine, but can be simplified. Instead of having copyJsonValue (shouldn't it be addJsonValue instead?), traverseAndCopyJsonObject, removeJsonValue, traversetAndRemoveJsonValue, traverseAndReplace, they can be combined into one or two more generate methods, because they all similar, except for the operation on the last node.

What we need is the abstraction that encapsulates the reference of a JsonStructure with a member, and in which we can apply the operations. I have in mind something like,

interface NodeRef {
      JsonStructure add(JsonValue v);
      JsonStructure remove();
      JsonStructure replace(JsonValue v);
}
public class ArrayRef implements NodeRef {
    ArrayRef(JsonArray a, int index){ 
    ...
}
public class ObjectRef implements NodeRef {
    ObjectRef(JsonObject obj, string name)
}

So the operations would be handled in implementation classes.

[glassfishrobot]

asotobu said: Well if we create an add method that returns a JsonStructure then we will need some instanceof code because Builders works with JsonValue but well there is no problem in doing in this way.

About the NodeRef interface I don't catch you sorry. I mean how current JsonStructure interface meets NodeRef interface and how it can be used from the point of view of JsonPointer. I mean JsonStructure is something that it is not dynamic in the sense that it is provided by users. But on the other side we have NodeRef which is dynamic and depends on the evaluation of the JsonPointer expression. So in this case I am not sure to understand exactly how to work with these classes.

[glassfishrobot]

kchung said: Instead of explaining how NodeRef works, it would be easier to show you the codes. Therefore I've checked in a NodeReference.java. Essentially it encapsulate all the operations you want to do with a JsonStructure member.

I've also implement some operations in JsonPointer, using NodeReference. I've reuse your codes that parases the escaped reference tokens.

The codes compiles, but not tested yet. Please go over the codes, and let me know if you have questions. Also let me know if you spot errors and/or improvements.

There are still some more works to do to finish JsonPointer, and you are welcome to do that. But I think we should start on JSON patch and JSON merge patch, which build on top of JsonPointer.

[glassfishrobot]

asotobu said: Ok now I understand what you mean, first I was thinking that JsonStructure should implements NodeRef for this reason I didn't catch you, now I see that what you mean was an aggregation so all operations are inside a class and not in JsonPointer, really good idea.

Ok from my part this weekend I am going to complete this class, run all the tests I have on my project (I need to refactor some of them but overall they are good and so exhaustive), fix any error I can find (if any) and then push the code to master.

Now I am going to start a conversation in PATCH issue so we can start discussing the API and then I think that implementation will be pretty easy.

Thank you so much for your reviews

[glassfishrobot]

asotobu said: I have just pushed my latest work on jsonpointer. I am using the mirrored github repo (https://github.com/json-processing-inofficial/jsonp) and you can see the PR here https://github.com/json-processing-inofficial/jsonp/pull/1/files I think that when the change is accepted it will be automatically synchronized with java-net repo.

Things that I have modified:

• equals and hash methods to be a real implementation. (Should we do the same for toString()?)
• in getValue in case a non JsonStructure is passed a JsonException is thrown because it has no sense (I think after reviewing the spec) to apply a json pointer to a single value but I may be wrong.
• in getValue there was a bug in case pointer points to a non existing element. Instead of returning null now it returns a JsonException.
• finished add(JsonValue, JsonValue) method following similar approach as getValue. I think it has no sense to apply an add operation to a single value.
• in NodeReference implementations there were two bugs on array remove and replace operations, in case of using "-" char in expression an ArrayIndexOutOfBounds was thrown instead of JsonException.
• Added tests for all operations, more tests can be done but for now it is a good start.

[glassfishrobot]

kchung said: Line 112 - 123: can be replace with

return jsonPointer.equals(other.jsonPointer);

since jsonPointer cannot be null (see javadocs for the constructor)

hascode(): why not just return jsonPointer.hashCode();

After reading the spec again, it does seems to say that the target must be a JsonStructure. If so, we should remove the method

public JsonValue add(JsonValue target, JsonValue value)

and replace

public JsonValue getValue(JsonValue target)

with

public JsonValue getValue(JsonStructure target)

According to the spec, we should throw JsonException when value is non-existing (including array index out of bounds). We should push the checking to NodeReference

[glassfishrobot]

asotobu said: Yes I read it before the PR again and I arrived to same conclusion, for this reason I didn't implement JsonValue but casting to JsonStructure or throwing an exception. We can remove methods without much problem.

[glassfishrobot]

kchung said: @Alex I committed and pushed the changes we discussed. Please review and test. You can also check in the tests, if they pass.

[glassfishrobot]

asotobu said: Good this weekend I will incorporate the tests I have already written and start working on JsonPatch as we discussed in the issue.

[glassfishrobot]

asotobu said: @Kin-man while writing tests for JsonPatch I have found some issues regarding to JsonPointer/NodeReference. For example being target an array and the json pointer something like /~1 which is translated to "/" and not to the number 1 it throws a NumberFormatException. What I have done is catching the NumberFormatException and throw a JsonException wrapping the NumberFormatException. I think this is the correct way to procedure, WDYT?

And it happens the same when you try to put a value on an index array out of bounds.

[glassfishrobot]

Issue-Links: blocks JSON_PROCESSING_SPEC-66

[glassfishrobot]

This issue was imported from java.net JIRA JSON_PROCESSING_SPEC-60

[glassfishrobot]

Marked as fixed on Tuesday, March 7th 2017, 9:05:12 am