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

Add ObjectMapper.updateValue() method to update instance with given overrides #1556

Closed
syncer opened this issue Mar 12, 2017 · 11 comments
Closed
Milestone

Comments

@syncer
Copy link

syncer commented Mar 12, 2017

ObjectMapper already have method to convertValue by doing two-step conversion from given value, into instance of given value type.

It will be great if ObjectMapper can update already created instance.

public void convertValue(Object fromValue, Object toValue) 
               throws IllegalArgumentException {
    if (fromValue == null || toValue == null) {
        return;
    }
    JavaType toValueType = _typeFactory.constructType(toValue.getClass());
    TokenBuffer buf = new TokenBuffer(this, false);
    if (isEnabled(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)) {
        buf = buf.forceUseOfBigDecimal(true);
    }
    try {
        SerializationConfig config = getSerializationConfig().
                without(SerializationFeature.WRAP_ROOT_VALUE);
        _serializerProvider(config).serializeValue(buf, fromValue);
        JsonParser jp = buf.asParser();
        DeserializationConfig deserConfig = getDeserializationConfig();
        JsonToken t = _initForReading(jp);
        if (t != JsonToken.VALUE_NULL
                && t != JsonToken.END_ARRAY
                && t != JsonToken.END_OBJECT) {
            DeserializationContext ctxt = createDeserializationContext(jp, deserConfig);
            JsonDeserializer<Object> deser = _findRootDeserializer(ctxt, toValueType);
            deser.deserialize(jp, ctxt, toValue);
        }
        jp.close();
    } catch (IOException e) { // should not occur, no real i/o...
        throw new IllegalArgumentException(e.getMessage(), e);
    }
}
@cowtowncoder
Copy link
Member

Seems potentially useful. Couple of questions:

  • Due to multiple existing overloads, should this be named slightly differently... maybe like updateValue()? (or convertInto() or mergeValues()....)
  • For convenience, shouldn't this return toValue? (if so, could define type parameter T to allow chaining)

Also curious as to what use cases you might have; since unit tests are needed, would be interesting to use something realistic.

@syncer syncer closed this as completed Mar 15, 2017
@syncer syncer reopened this Mar 15, 2017
@syncer
Copy link
Author

syncer commented Mar 15, 2017

  • I think updateValue seems to be consistence with the term valueToUpdate use already in the ObjectMapper
  • Returning toValue seems to be consistence with the rest of the API

Our use case; A response object which loaded from two different data sources one of the data sources have multiple format, therefore I need to load it and transform it to the current format then merge it with the response object.

@cowtowncoder
Copy link
Member

@syncer Ok let me have a look. I think this could play nicely with other new 2.9 features which allow (f.ex) deep merging.

@cowtowncoder
Copy link
Member

Ok implementation is fine, but there is one open issue I am wondering about: how to detect non-updateable Objects, and what to do about them.

On one hand, there is a method added in 2.9 to detect types for which "merge" does not work:

public Boolean supportsUpdate(DeserializationConfig config) ...

which rules out a class of types, most notable scalars.
But aside this, there are types that physically do not allow changes -- arrays, specifically have fixed size; and Strings are immutable -- but may allow logical changes (appending an array works by creating a new one).
So to support merging, this method operates for logical, not physical changes, and thereby does not prevent merging of arrays.

However, here we are returning exact first argument. So, in case of arrays, we could do one of following choices:

  1. Prevent operation for all arrays (and perhaps small number of other types if need be) -- would be hard-coded, so custom handlers could not override (if user wanted different semantic of "update")
  2. Let operation proceed, but returning first argument: essentially quietly failing (but no exception)
  3. Change semantics to return whatever ObjectReader.readValue() returns, which may be first argument, or a newly allocated other object.

I guess I could see either (1) or (3) working. If we went with (3), however, maybe name "updateValue()" would not make sense after all. We could consider some other name, which would reflect the fact that operation may either update given Object, or create a new Object that is combination of two input Objects.

What do you think?

In the meantime, I think I'll check in my work so far, even if it is likely to change.

@cowtowncoder cowtowncoder changed the title ObjectMapper: Add convert method to update instance from given value. Add ObjectMapper.updateValue() method to update instance with given overrides Mar 21, 2017
@cowtowncoder cowtowncoder added this to the 2.9.0.pr2 milestone Mar 21, 2017
@cowtowncoder
Copy link
Member

Decided to make semantics such that whatever the result of logical merge is is returned: this is usually first argument, but not always -- Java arrays, for example, can not be changed (with respect to length), so updateValue() on an array will succeed, but has to create a new instance and return that. But type should be the same so usage should be convenient and allow chaining.

Testing so far is limited to shallow updates, but will try to find time for deep merge: this will, however, only work via #1399 that is, configuration and/or annotations need to indicate the need for merging.

@syncer
Copy link
Author

syncer commented Apr 21, 2017

I tried the deep merge with ObjectMapper.setDefaultMergeable(true); The updateValue() method will always set a new value on any nested property even if its already set to non-null value. 2.9.0.pr2

@cowtowncoder
Copy link
Member

@syncer If so, I would appreciate a failing unit test to show the problem, along with a new issue (just so we can keep track of what work goes into which version). For what it is worth, I did some work on deep merge of "untyped" values (java.lang.Object) yesterday, so it may be good to try with master.

@syncer
Copy link
Author

syncer commented Apr 25, 2017

@cowtowncoder Thank you for your time. After I did some digging I found a conflict with lombok @AllArgsConstructor after set suppressConstructorProperties = true everything is working perfectly fine.

@cowtowncoder
Copy link
Member

@syncer Ah. Yes, those can be problematic. Thank you for verifying.

@cvc-saksoft
Copy link

cvc-saksoft commented Aug 27, 2019

What if the values to be overridden from, are with different name, like firstName to be overridden with fName in another object, for example? Can we make a use of @JsonAlias here? And is it mandatory for updateValue to have same types as parameter?

@cowtowncoder
Copy link
Member

@cvc-saksoft Usage questions should generally be sent to mailing list (https://groups.google.com/forum/#!forum/jackson-user) or Gitter (https://gitter.im/FasterXML/jackson-databind).

But as to naming: property names need to match, so incoming JSON just has to map to type, same as if just using readValue(). No special handling is applied aside from skipping construction of value.
Type parameter is used for finding deserializer to use.

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

3 participants