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

Java 8: Support for serializing and deserializing immutable objects without annotations. #399

Closed
lpandzic opened this issue Feb 6, 2014 · 30 comments

Comments

@lpandzic
Copy link
Contributor

lpandzic commented Feb 6, 2014

To serialize/deserialize immutable objects currently you have to add annotations like this:

public final class Point {

    private final int x;
    private final int y;

    @JsonCreator
    public Point(@JsonProperty("x") int x, @JsonProperty("y") int y) {

        this.x = x;
        this.y = y;
    }
}

One obvious downside to this appcorach is that it adds complexity and pollutes the source code.

This can be solved by using the new API for formal parameters names retrieval in Java 8.
To pick the constructor following strategies could be used:

  • @JsonCreator for explicit declaration
  • implicitly reasonable default should be used such as constructor with longest parameter list.
@cowtowncoder
Copy link
Member

Jackson has JDK6 as the baseline, and it won't be possible use any JDK8 for quite a while.

But there has been talk about creating jackson-module-jdk8 (or -java8) which would be natural place for such improvement. I think that would be a good idea, perhaps also worth discussing (again) on jackson-dev mailing list.

I think this could omit need for @JsonProperty. Question of @JsonCreator is bit trickier; could perhaps have a feature to enable auto-detection; I am bit wary of advanced heuristics between default (no-args) constructor and one or more alternatives. Although existence of a single public multi-arg constructor might be good enough hint.

@cowtowncoder
Copy link
Member

I created:

https://github.com/FasterXML/jackson-datatype-jdk8

so this will move to that module.

@mmc41
Copy link

mmc41 commented Jul 7, 2014

Brilliant with the Java 8 support retrieving parameter names in CTRs for immutable objects. I am strongly considering using Jackson instead og JAXB just because of the new jackson-datatype-jdk8 module.

@lpandzic
Copy link
Contributor Author

lpandzic commented Jul 7, 2014

I'm sorry to inform you that the module supposed to add this feature to jackson called jackson-module-parameter-names is not fully functional yet and my work on it has been halted due to my other project. It still requires the use of @JsonCreator thus limiting it's usability.

If you are interested please respond to the topic regarding this module on jackson-dev mailing list.
I'd gladly continue my work if we could reach a consensus about the module.

@mmc41
Copy link

mmc41 commented Jul 9, 2014

That is unfortunate since this new feature (arguably best if not requiring @JsonCreator) is revolutionary and would make Jackson a clear winner.

@cowtowncoder
Copy link
Member

I don't see any blockers for work on java8 Jackson module. As per above, jackson-databind itself can not use Java8 features, but java8 module can. Requirement to use @JsonCreator is there, sure, but I can't see that as a blocker for initial support; and even then it is possible to improve that aspect incrementally.

@lpandzic
Copy link
Contributor Author

lpandzic commented Jul 9, 2014

That is unfortunate since this new feature (arguably best if not requiring @JsonCreator) is revolutionary and would make Jackson a clear winner.

After some testing my previous statement doesn't seem to be true and the README.md is wrong. Current implementation of the module doesn't check anything about the @JsonCreator (just take a quick glance at the implementation) so it can work without @JsonCreator.

The real problem is the situation in which you use @JsonCreator. For example, lets say a class has identical constructor and static factory method (identical in term of parameter names and types) and the static factory method is annotated with @JsonCreator. Since the module ignores the @JsonCreator and jackson-databind inherently prefers use of constructors, deserialization of this class will result in the constructor being called instead of the static factory method. This presents a violation of the @JsonCreator contract.

The source of this problem is the fact that jackson-databind is "coupled" to the current shortcoming of the JacksonAnnotationIntrospector. JacksonAnnotationIntrospector cannot resolve the parameter names without @JsonProperty and it doesn't make any sense from the client side to write @JsonProperty without using @JsonCreator. This coupling is documented in this issue.

This problem can not be simply solved in the module alone because of the way the current API available to the module for resolving parameter names works. The API, which mainly consists of the method PropertyName findNameForDeserialization(Annotated annotated), is called on the module multiple times for each parameter available but in no predefined order which makes it impossible to detect when the module should not return the parameter name (in case a class does have a @JsonCreator).

Now in order for the module to abide by the contract of @JsonCreator it should mimic JacksonAnnotationIntrospector and not return parameter names in cases where a @JsonCreator is present and the currently inspected parameter is not it's member.

Yesterday I've been playing around with a possible, although hackish, solution.
The basic idea goes like this - for every new class that is presented to the module it scans the whole class for @JsonCreator. In case where no @JsonCreator is present the module can work as normal.
In case a @JsonCreator is present we need to store that information and not resolve parameter names for constructors/methods which are not annotated with @JsonCreator. This solution does abide by the contract but has cons of it own:

  • it doesn't feel right scanning the class for @JsonCreator, the jackson-databind should do it before calling the module
  • the information whether the scanned class has @JsonCreator should be cached since I doubt it's cheap to scan it for every findNameForDeserialization call
  • the cache (map) implementation choice is deferred to the client since there is no real implementation of a cache map available in the JDK

I don't see any blockers for work on java8 Jackson module. As per above, jackson-databind itself can not use Java8 features, but java8 module can. Requirement to use @JsonCreator is there, sure, but I can't see that as a blocker for initial support; and even then it is possible to improve that aspect incrementally.

I think he's trying to say what I've been saying for some time - gain of this module is greatly diminished if you still must use @JsonCreator for all immutable classes. I'm not saying that we should ignore @JsonCreator but I'm saying that we need to resolve this issue for parameter names module to be useful/successful.

@mmc41
Copy link

mmc41 commented Jul 10, 2014

How would scanning work? You would need some way to limit which classes are scanned otherwise it could SLOW startup of an application up like I have observed with other tools. Also, there is the principle of least surprise. One should not be surprised that a immutable class is instantiated by jackson. Maybe an annotation (and maybe a new one) on package or class level would be ok (but not all constructors which too intrusive and to much I think).

@lpandzic
Copy link
Contributor Author

Scanning is done only on the classes that are to be serialized/deserialized and only when first asked by jackon-databind (with the method I mentioned in my previous reply). The AnnotatedParameter that is passed to the module contains the information about the constructor/method to which the parameter belongs and also the class. The scanning is done on that class and the scanning is only looking for @JsonCreator on methods/constructors.

@cowtowncoder I haven't found any documentation about whether one or more @JsonCreator are possible per class? I'm assuming that only one is allowed.

@cowtowncoder
Copy link
Member

@lpandzic Only one is currently allowed for constructor, and one for factory method.

@cowtowncoder
Copy link
Member

Ok one other relevant thing wrt AnnotationIntrospector: as of 2.4, there is one new method, findImplicitPropertyName(), which differs from findNameForSerialization/findNameForDeserialization in that result does not indicate that parameter name is not explicitly annotated by developer. Distinction mostly matters to property renaming (implicit names are overridden by explicit names), but matters here as well -- databind is able to tell difference between something marked with @JsonProperty, and something that isn't.

But there is still the problem that distinction between two types of creators:

  1. "Delegating" creator: one argument without explicit name, and
  2. "Property-based" creator: one or more arguments, all with explicit name (or marked as injectable)

where expectation of explicit names is problematic at first; but if relaxed (to allow implicit names), case between 1 and 2 becomes problematic; basically it is not possible to distinguish which one is meant for creator with one argument.

For this, one possibility would be to add a new property for @JsonCreator, which would explicitly indicate type of creator using enumeration (delegating, property-based... and perhaps "auto" as default?). This would resolve possible ambiguity.

This would only help in this particular problem, and not remove the need to use @JsonCreator, or allow use of multiple creators (of which most optimal is chosen).
But it could be a start, and would be easy to add for 2.5.

@lpandzic
Copy link
Contributor Author

Only one is currently allowed for constructor, and one for factory method.

It is possible to have 2 JsonCreators on one class?

About the issue I mentioned in my previous, longer post, please take a look at this gist. If ran against jackson-module-parameter-names 2.4.1 it will fail which demonstrates the issue with implicit constructor preference.

About the problem with distinction between two types of creators, can you make a test (gist) to demonstrate it?
I'll try to finish the prototype I mentioned before which doesn't force use of @JsonCreator and still passes the before mentioned test.

@cowtowncoder
Copy link
Member

What I meant by "one for constructor, one for factory" method is that it is possible to technically have two instances of @JsonCreator annotation; but even then, it resolves into exactly one creator method.
There is no support for dynamically choosing from set of more than one creators.

As to difference, I think the only case is simple one-argument case like:

public class X {
   public X(String value) {
       ...
   }

where question is whether this should be mapped from JSON String (delegating creator), or from JSON Object like { "value" : "something" }. With current rules it would be deemed delegating creator (i.e. require JSON String to match), but if explicit name was specified (with @JsonProperty), it would be seen as property-based creator. This is not problematic before implicit names since explicit names are needed anyway.

But maybe this is more a documentation issue: in case of single-argument creator, explicit name must be added, to avoid use as delegating creator. Or, alternatively, by introducing 'type' for @JsonCreator.
Or... If deserialization process was extended to allow multiple creators, one could perhaps allow both, depending on type of incoming Object.

A prototype would be good.

@lpandzic
Copy link
Contributor Author

@mmc41
Give the currently released version a try, just be sure to check your corner cases.

@cowtowncoder
Have you tried the test maybe? If those when and then calls aren't clear take a look at first revision of the gist.

@cowtowncoder
Copy link
Member

@lpandzic sorry, haven't had time to look into this.

@cowtowncoder
Copy link
Member

FWIW, now #421 is fully resolved (for 2.5), so at least we have clean baseline.
Adding support for alternative Creators (not just resolving into one), or allowing implicit constructors, is still some work. But at least shouldn't have old bugs blocking it.

@lpandzic
Copy link
Contributor Author

What needs to be done for implicit constructors?

@cowtowncoder
Copy link
Member

I think there is still the part of figuring out what is acceptable criteria to avoid accidental use of constructors (and perhaps factory methods) that are not meant as creators.

I wish we could use existing Visibility stuff for determining this, but entries that we have are used in a way that will probably not work -- creators are only for single-argument constructors, and do not apply to multi-arg (property-based) discovery.

Another possibility would be adding yet another DeserializationFeature, to enable finding best match (constructor with most arguments), within visibility limits (as per above, current creator visibility is "any level"). Perhaps this is the way to go?

@lpandzic
Copy link
Contributor Author

I'd prefer this behavior to be the default one since it was my original aim to provide this kind of behavior in this module by default and I believe users would like to see it as well. A DeserializationFeature to enable it wouldn't be much worse but since this is a new module I'd rather have a disable than enable feature.
And of course, this behavior should be well documented.

@cowtowncoder
Copy link
Member

Ok, there are two separate questions; whether to enable it by default for module, or for Jackson as whole. If this was just for module, I would tend to agree. But unfortunately I don't think modules can control this logic as is; they can provide necessary information (implicit names), but they don't control application and main rules.

And then regarding jackson-databind: based on my experiences, changing this as default would result in many bug reports indicating that change broke something, or introduced undesired behavior.
In fact I'd bet that some of my own projects would start behaving in odd or wrong ways when constructors used solely for testing would suddenly be attempted to be used by deserializer.

So: it's not nearly as much a question of how many would like to see the new feature as it is how many would be negatively affected by it. Ratio has to be very high before default behavior may be changed.

But as I said, if default was just for new module, calculation would be different because it wouldn't be as much of a change, given that there are very few users. For most developers it would be original default.

@lpandzic
Copy link
Contributor Author

The change should only affect this module, of course.

But unfortunately I don't think modules can control this logic as is; they can provide necessary information (implicit names), but they don't control application and main rules.

Have those rules changed in past few versions and what can be done about this?
If nothing can be done, then a feature to turn it on would be better than the current state.

@cowtowncoder
Copy link
Member

Rules have not changed. The main change is that names provided by modules were always considered explicit (since there was neither distinction between implicit/explicit, nor need/ways to access implicit names); and as an unintended side-effect, it looked like parameter-names module was finding explicit naming for Creator parameters. And this in turn fulfilled requirement of "all Creator names being explicit, use of @JsonCreator is optional".

So you could say that a bugfix changed behavior here.

This is why it would be necessary to have a central setting, enabling of which states that it is not even necessary for all Creator parameters to have explicit names, but just "a name" (implicit or explicit); and that this being the case, creators may be detected.

And why not let module change this setting? Because modules are add-ons and conceptually should not change central settings. Partly because there are no rules governing what should happen if modules disagreed, partly because it would be confusing if various DeserializationFeatures were changed simply due to registration of modules.
But mostly it's just the design of things where modules are to help make things work, but not control the workflow directly.

Now: in use cases where Jackson itself is embedded, there is nothing preventing container from offering specific alternate defaults.

@cowtowncoder
Copy link
Member

@lpandzic Actually, having written all of above, I realized that one easy thing that jackson-module-parameter-names could do is have an on/off feature/option that toggles whether names for parameters are returned as "implicit" or "explicit". Latter would then have the desired effect of causing inclusion of constructor(s) in question.

This does not solve all possible issues (regarding resolving what to do if multiple creators are found and such), but I think those could more easily tackled here in jackson-databind.
For example, if only one has actual @JsonCreator, it'd have precedence; otherwise check count of arguments and so forth.

We can still consider other features here as well (for detecting creators via implicit names). But I thought I'd add a note for short-term thing, for 2.5.0.

@christophercurrie
Copy link
Member

@cowtowncoder let me know as you're getting ready to look into this, as it definitely overlaps with functionality in the Scala module, so we should line up behavior as it makes sense to do so. Kotlin module could probably use it as well.

@cowtowncoder
Copy link
Member

@christophercurrie All the underlying existing bugs have been resolved as far as I know, so most of the work would be for adding new features. Although I guess I could start by improving conflict resolution for creators; and perhaps it'd be possible to allow pluggable accessor conflict resolvers.
That is, something that can say, given 2 things of type A (setter, getter, field, creator) if one of them should have higher precedence (and thereby be used, and the other one dropped); and only if no resolver can do that, would there be a conflict to report.

If I remember Scala discussions correctly, part of the problem was that there are multiple things that can become implicit setter/getter?

@christophercurrie
Copy link
Member

That is certainly one of the potential areas of resolution. The other is, determining the correct constructor, or, in some cases, factory method on another object; (as in, not a static method, but an instance method, on a singleton instance of a related class).

@cowtowncoder
Copy link
Member

FWIW, I created #569

@lpandzic
Copy link
Contributor Author

lpandzic commented Oct 1, 2014

So if I understood you correctly, Tatu, jackson-databind parameters API needs to be enhanced in order to eliminate the need for @JsonCreator?

@cowtowncoder
Copy link
Member

@lpandzic Right, I think central policy and its implementation is something jackson-databind is responsible for.

I will try to re-start discussion on jackson-dev now. We can then either file new issue(s), or re-open this one, whatever makes most sense (and if new, add a reference back to this discussion).

@lpandzic
Copy link
Contributor Author

lpandzic commented Oct 8, 2014

All interested in this problem should take a look at this discussion and provide feedback.

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

4 participants