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

Collection deserialization does not invoke property initializer #509

Closed
kdkeck opened this issue Jul 30, 2014 · 4 comments
Closed

Collection deserialization does not invoke property initializer #509

kdkeck opened this issue Jul 30, 2014 · 4 comments

Comments

@kdkeck
Copy link

kdkeck commented Jul 30, 2014

I have a class in which I want one of the properties to hold a SortedSet in reverse order. I naively thought the following code would accomplish this:

public class Foo {
  @Id private ObjectId id;
  @JsonProperty private final SortedSet<String> reverseSortedSet =
      new TreeSet<>(Collections.reverseOrder());
}

But it does not. Instead, upon deserialization, foo.getReverseSortedSet().comparator() returns null.

Would making this work as one would expect be too much to ask?

@cowtowncoder
Copy link
Member

Behavior would change from default, in that code would first have to see if field already has a non-null value, and do this only for Collection and Map types (and perhaps POJOs).

This could be doable, since something similar is actually done with feature that allows use of "getters as setters". But I am trying to think of proper way to indicate that deserialization should consider existing value; this is not necessarily what is needed in other cases.

One possibility would be to add one or more DeserializationFeatures (either one general, or one for Maps, one for Collections, one for POJOs); another to add an annotation (or property for, say, `@JsonDeserialize).

My main concern is that there may be cases where changing behavior to default to trying to access existing value would cause problems. With field access this may not be problem; but with getters it could be.

@kdkeck
Copy link
Author

kdkeck commented Jul 31, 2014

I was kind of hoping you'd be inclined to classify it as a bug, rather than a request for a new feature, since the current behavior is so contrary to how Java developers expect final fields, in particular, to work. But it's just my intuition that the current behavior is much more often a source of unexpected behavior that is then worked around, than it is an expected behavior that is relied upon, and I could turn out to be wrong about that. So implementing a new DeserializationFeature and a new JsonDeserialize property first would give people a chance to experiment with it and decide whether they should perhaps consider changing the default in, say, a new major release...

@cowtowncoder
Copy link
Member

No, it's not a bug, but design since beginning of Jackson. I can see how it could be surprising, but I would not consider it a bug. I could see it as a flaw... so maybe it's just semantics. :)

One other practical thing is that doing a lookup on field will add non-trivial amount of overhead for deserialization (due to reflection being used); and this is overhead that can not really be optimized away.

Obviously performance does not trump usability, and perhaps behavior for one of cases -- that of using Field for deserialization -- should default to checking existing value, for next major version.

This may be something that would be best discussed on Jackson dev list. I'd be interested in hearing other opinions; especially considering that this is the first time issue has been reported. Perhaps others found out about behavior but didn't care enough to suggest improvements.

@cowtowncoder
Copy link
Member

Actually #1399 allows this to work: just need to annotate property with:

@JsonSetter((merge=OptBoolean.TRUE)

and this will make existing property-value be checked first.
Setting may also be used as global default (for all properties unless overridden), and via per-type "config overrides"; see #1399 for details.

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

2 participants