Jackson-databind: Add 'ObjectNode.set(String,JsonNode)' for consistent entry manipulation semantics for ObjectNode

[fge]

As discussed on the users group (http://jackson-users.ning.com/forum/topics/method-chaining-is-this-by-design): JsonNode has many .put() methods on ObjectNode, but their behaviour is inconsistent. On a given ObjectNode:

• .put("somekey", any(JsonNode.class)) will return the old value for key "somekey" if any, null otherwise;
• .put("somekey", anythingBut(JsonNode.class)) will return the ObjectNode being manipulated.

Quoting:

It is by design, in the sense that original idea was to model the way java.util.Map works; but then later additions > (adding Java values, not JsonNode) chose to instead return ObjectNode to allow chaining.

Later on in the thread, it was acknowledged that allowing chaining was a better model on the whole, but that changing .put() behavior was too late for 2.x; and that another set of methods could possibly be added for 2.1.x.

Note: I haven't looked at ArrayNode, maybe it could do with something like this too.

Proposal (for ObjectNode): .addMember()/.removeMember(), or maybe .withMember()/.withoutMember(), with the consistent behavior throughout that they return the manipulated ObjectNode; and for 3.x, mark these as deprecated and unify .put() behavior, since it is a better name anyway, to allow chaining.

There is also the question of .remove(), though.

[cowtowncoder]

Also realized that if starting from scratch, current "put" method probably should (have been) called "replace", since that would imply what return value means. Oh well.

[cowtowncoder]

I really like 'withMember()' / 'withoutMember()' in itself, and considering Jackson's naming outside of JsonNode, where 'withXxx()' are used a lot.
Alas, ObjectNode already has "with()" and "withArray()" methods, which are used for traversal; and return value is the referenced child object.

addMember()/removeMember() could work however.

Now, should this cover all the types for which put() exists; or just Object and Array variants? I guess all would allow for more consistent approach, even if it expands API a bit.

[cowtowncoder]

Actually, I think I will go with "set(...)" method instead; semantics so that 'this' is returned: starting with just one with second arg of type JsonNode.

[cowtowncoder]

Ok, added:

• ObjectNode.set(String,JsonNode) to replace matching 'put', but returning 'this'
• ObjectNode.replace(String,JsonNode), similar as "set" but returns old value
• ObjectNode.setAll (to replace putAll, with same semantics)
• ObjectNode.without(...) methods to replace cases of 'remove' where 'this' should be returned

[chitradip]

I realize this is 4 years ago, but set() should return Objectnode, otherwise chaining becomes impossible.

[cowtowncoder]

@chitradip Yes except that it is impossible to change that now because that would cause major backwards incompatibility, breaking existing code, and I'd get to handle a shitstorm. This has been discussed multiple times over multiple issues, although not yet this one.

Alas. So it will not be done unless there's a major backwards-incompatible version change (to prevent old code from trying to run).

[chitradip]

Hey @cowtowncoder,
Maybe another api which allows chaining?

Also since this is a final class, what would break if you changed it? Without analyzing it thoroughly, anyone who called it expecting JsonNode would not know the difference if you returned ObjectNode.
Does it become binary incompatible? I would love to see the discussion on this if you have it handy.

[cowtowncoder]

@chitradip I would like chainable version, but as things are API is sizable enough, and all obvious good names seem to be taken.

As to binary compatibility, yes. Changing return type changes method signature, and Java's linker (class loader) does not do any coercion -- something compiled against old signature will fail to load with version with new signature. Worse, it's source-code compatible: that is, if you recompile code, it will once again work (since bytecode has the new signature). This is not theoretical but is exactly what broke lots of Hadoop interoperability between Jackson 1.1 and 1.2 (or was it 1.0 and 1.1... either one), when a method that didn't return anything was changed to return this (one of ObjectMapper configuration methods).
It is also not possible to have overloads that only differ by return type (except that compiler can actually generate such bridge methods for return type co-variance), to support migration.

[mortalisk]

As I understand the put method was deprecated, in favor of the set and replace methods, specifically to allow method chaining? But set and replace do not support method chaining? Is there a workaround for this?

[cowtowncoder]

@mortalisk Yes, the intent was to replace put that takes JsonNode. But while return value is this, it is undertyped as JsonNode (silly mistake that I wish I had caught) which does not contain actual modification methods. So chaining is possible but cumbersome since casts are needed.

This is something we can and should tackle for 3.0. But instead of piecemeal changes, should design API improvements as a set of related changes.