Pointer based add and remove methods for #392

[jgustie]

No description provided.

[cowtowncoder]

Hmm. Aside from the question of where to expose these methods (which in themselves make sense), it seems like these methods would not indicate a failure, when there is no matching path?

[cowtowncoder]

Oh also, definitely need unit tests to show that it works: I am not quite sure about correct matching for exact nodes.

Wrt return value, it should work ok with remove method; but I think ValueNode should probably either throw exception for no match, or return null or missing node. Alternative would be to create path, if possible. But there are error cases too, like trying to add a property into Array node.

[jgustie]

Yeah, sorry, this was more just some initial spit balling: I wasn't sure if the changes to JsonNode would fly or if they should be pushed down to the ContainerNode (which makes sense, but I thought it might be nice to invoke the operations even if you didn't know what type of node you had).

I was thinking of maybe a JsonPatchException extending JsonProcessingException (which could also be tossed in the ValueNode case). Originally I thought add and remove were good method names (just overrides of the existing methods); but if an exception can be generated they probably need a different name (addAt/removeFrom? patchAdd/patchRemove?).

I was thinking an error is better then creating missing path nodes: a higher level implementation could then choose to fail (that is what the JSON Patch spec says) or handle the error by creating the necessary path.

Let me try flushing it out a little; but in general, is the "add or remove by pointer" a worth while addition?

[cowtowncoder]

@jgustie First of all: yes, I think add or remove by pointer definitely makes sense. I have hoped that addition of JSON pointer would spur interest in additions like you suggest. So I am excited about this.

I am not entirely sure about JsonPatchException; I can see some merits, but it's bit granular considering the package as whole, and might give an idea that Jackson supports JSON patch.

But one thing that would probably be good here would be to maybe suggest this on Jackson dev list; I think there is interest in this work. And there are some aspects that have been discussed earlier; especailly the question of whether and how to work on multiple pluggable tree models. I have grown to realize that it is very difficult to come up with a single tree representation that works well for everyone and has intuitive API. One example is issue of mutability: immutable tree models (that is, something where changes are not done by modifying state of existing nodes, but by creating and returning new intances -- i.e. nodes immutable does not mean one can not create modified instances) would be useful, but then again not good for other use cases.

I think these additions would still work well within existing JsonNode implementation (for more generic approach, TreeNode is a base intereface for JsonNode FWIW). But I just wanted to mention the bigger questions surrounding it.

[jgustie]

I stayed away from the custom exception and stuck with IllegalArgumentException for path issues. Also fixed that issue you mentioned when the path wasn't an exact match (with some tests this time). For mutability I went the Collections API route by throwing UnsupportedOperationException (perhaps future immutable trees can do the same).

I changed the return values so they hopefully make a little more sense: e.g. implementing RFC 6902's "move" should be something like node.add(path, node.remove(from))

I also noticed that remove(String) would create a conflict so it either needs a different name or no String -> JsonPointer helpers.

[cowtowncoder]

Sounds good so far. I am open to considering addition to JsonNode, mostly since these methods have same signature, which is different from object/array-specific add/put/insert methods.
I will actually send a note on dev mailing list to see how others feel, and perhaps get additional suggestions & ideas. I think this will result in a nice additional functionality for 2.4.

[cowtowncoder]

Suggestion here; take JsonNodeFactory.instance, use its factory methods for creating ObjectNodes (etc) -- same for other usage too of course. Or, if tests have ObjectMapper, use array/object node factory methods.

[jgustie]

I just saw this thread on the dev list about JSON Patch support: it got me wondering where this pull request stands?

I originally introduced these changes because they make something like a JSON Patch implementation pretty easy (in fact that is what originally inspired these changes: I could probably dig that JSON Patch code back up).

[cowtowncoder]

@jgustie I think I dropped the ball here.

After thinking about it a bit, I think I am ok with add and remove for JsonNode.
While mutability is not otherwise supported, this seems like acceptable deviation.
Thank you for changes so far, code has improved a lot.

Some remaining issues are:

1. Use of '-'... I assume it comes from JSON Patch? Are semantics used in JSON Path?
2. Exceptions vs returning false vs quietly ignoring: I wonder how various errors should be handled. Since most functionality of JsonNode will not throw exception (but quietly ignore), it might make sense to either indicate failure (true vs false), or, if applicable, return MissingNode. I don't know offhand what makes most sense.

Another that may or may not make sense is to try to get discussion restarted on jackson-dev list.
Timing-wise it should be possible to get developers interested, and this could be one of first 2.7 features.

[jgustie]

You and me both, I guess I must have been pretty distracted :)

I tried not to get anything specific from JSON Patch in this part of the implementation: the '-' actually comes from JSON Pointer. It's a reference to the nonexistent member after the last element in an array (the last bullet point in section 4 of RFC 6901). It doesn't make much sense when querying which would explain why it wasn't needed yet.

With the error handling, I only used MissingNode internally as a way to raise an exception (i.e. an invalid path will lead to a missing node whose invocation of add/remove methods always throws IllegalArgumentException). Let me know if you think something should be changed.

I'll try picking up the conversation on the dev group to see if anyone bites.

[cowtowncoder]

Ah ok wrt '-'. Makes sense with that, was indeed wondering why accessor did not care about it, but makes sense.

Let's go with dev group, wrt error handling. I don't have strong opinion and am interested in hearing if someone has good suggestions there.

[cowtowncoder]

Ok, I create #1980 to replace add() part (via overload of with()), and will create another one for remove() (although that needs to go in ObjectNode, or maybe ContainerNode).