Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecated JsonNode.with(String) suggests using JsonNode.withObject(String) but it is not the same thing #3780

Closed
bmatasar opened this issue Feb 9, 2023 · 11 comments
Milestone

Comments

@bmatasar
Copy link

bmatasar commented Feb 9, 2023

Is your feature request related to a problem? Please describe.
Version 2.14 deprecated JsonNode.with(String) and suggests replacing it with JsonNode.withObject(String). However, this causes problems because JsonNode.withObject(String) accepts only expressions, whereas the former method accepts a property. Moreover, JsonNode.withArray(String) accepts both property names and expressions, making it quite asymmetric.

Describe the solution you'd like
Migrating the method would imply in many cases extra work and prepending the "/" before every property name in existing calls.
From the deprecation message of JsonNode.with(String) I would change myObj.with(myProp) to myObj.withObject(myProp) but that throws an exception, so I have to do myObj.withObject("/" + myProp), which I find unpleasant. Again, myObj.withArray(myProp) works just fine without need of change.

Usage example
The change from myObj.with(myProp) to myObj.withObject(myProp) should work with property or expression.

@bmatasar bmatasar added the to-evaluate Issue that has been received but not yet evaluated label Feb 9, 2023
@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Feb 13, 2023
@tanis138
Copy link

Faced the same issue. I think, adding "/" before every property is a bad idea. Because each withObject() call will cause JsonPointer.compile() on your property, which significantly decrease performance.
It would be great to add new method withProperty(String property) for creation properties.

    public ObjectNode withProperty(String property) {
        JsonNode n = _children.get(property);
        if (n != null) {
            if (n instanceof ObjectNode) {
                return (ObjectNode) n;
            }
            throw new UnsupportedOperationException("Property '" + property
                + "' has value that is not of type `ObjectNode` (but `" + n
                .getClass().getName() + "`)");
        }
        ObjectNode result = objectNode();
        _children.put(property, result);
        return result;
    }

Otherwise, migration to Jackson 3 will be very painful for many users like me, who often uses .with("property") method.

@cowtowncoder
Copy link
Member

Ok, I do not think it is a good idea at all to overload "property-or-expression" because two sets are overlapping -- even if it is not common, one can definitely use empty String as property name, or something starting with slash.

Situation with withArray(String) is a mess however, I admit. My apologies: it should be consistent -- and I need to see if it's just Javadoc being wrong.

Having said that, I am to addition of, say, 2 new methods that only allow property would make sense:

  • withObjectProperty(String)
  • withArrayProperty(String)

and I'd be happy to help merge a PR, based on code sample @tanis138 shared for example.

@bmatasar
Copy link
Author

Backwards compatibility is creating all this mess. For this purpose, "property-or-expression" fits best.
However, I think overloading withObject/withArray with String for property name and JsonPointer for expression would be clearer and cleaner. This is what overloading is all about, in the end.

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 1, 2023

I will not be adding any new "property-or-expression" functionality, fwtw. I think that is a bad idea and I will try to eventually get rid of such logic; for now (2.x) functionality will be retained for backwards-compatibility.
So these methods will stay deprecated but won't be removed.

I also dislike overloads for withObject() / withArray() because of ambiguity of String parameter: it could be either property name or un-compiled JsonPointer expression. Given that they are now implemented to support JsonPointer expression (whether compiled or as-String) that's how they shall remain.
Strings are tricky for similar cases as input source: some APIs accept file paths as String (instead of or in addition to java.io.File), wherein others take in String as content (document to decode).

This is why the way forward to me is addition of 2 new methods. And due to need to produce array or object nodes, need 2 instead of generic withProperty().

@cowtowncoder
Copy link
Member

On JsonNode.withArray(), it does indeed support "property-or-expression" -- I first looked at ArrayNode implementation where only expression works (because Arrays have no properties).

@JuMp3
Copy link

JuMp3 commented Aug 16, 2023

If it helps, I solved it like this (withObject):

objNode.withObject(JsonPointer.compile(newPath))

with this method of JsonNode.class (jackson-2.15.x)

public final ObjectNode withObject(JsonPointer ptr) {
return this.withObject(ptr, JsonNode.OverwriteMode.NULLS, true);
}

@filiphr
Copy link

filiphr commented Aug 21, 2023

I'd like to chime in on this one as well. We were in the process of replacing with with withObject as it was written in the Javadoc, I missed the fact about the change that withObject is using now an expression.

I 2.13 with(String) was only about property names, it didn't support using an expression. I don't know the reasoning for deprecating the with and for changing its behaviour to be propertyOrExpression. However, I do think that there should be something like you proposed @cowtowncoder in #3780 (comment).

To be honest, it makes it a bit cumbersome because withArray always supported property name and I guess that's why in 2.14 and 2.15 it supports property or expression. However, withObject is new from 2.14 and it doesn't support property names. In my opinion this type of API is a bit inconsistent.

e.g.

customer.withArray("addresses");
customer.with("address");

but if I do

customer.withObject("address");

then it doesn't work.

If 2 new methods are added then withArray is still going to support property I guess. This makes the API a bit inconsistent.

@cowtowncoder is it an option to make ObjectNode#withObject consistent with ObjectNode#withArray? This way withObjectProperty and withArrayProperty won't be needed. Adding those 2 methods on the JsonNode will pollute the API a bit more. Otherwise, the deprecation on JsonNode#with(String) should be removed until there is an equivalent replacement method for it.

@cowtowncoder
Copy link
Member

Deprecation was to address inconsistencies and my (seemingly mistaken) thinking that most access would be with multiple-level expressions, not "simple" property name. I also dislike heuristics of "property-or-json-pointer-expr".
Finally, since there is strict need to specify whether Array or Object value is expected, plain with() seems problematic (in contrast to withArray()).

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 22, 2023

After reading this through again, I suspect that changing withObject() to work similar to withArray() may make sense after all: as much as I dislike "expression-or-property-name", I seem to be in minority.
I filed a separate issue for this small change for 2.16 (#4096)

@filiphr
Copy link

filiphr commented Aug 23, 2023

Thanks for taking our feedback into consideration @cowtowncoder

@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Sep 30, 2023
@cowtowncoder
Copy link
Member

Ok: fixed via #4096 -- now withObject(String) does accept either JsonPointer-compatible String OR property name.

Additionally #4095 adds explicit withObjectProperty() / withArrayProperty() methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants