-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Comments
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 |
I created: https://github.com/FasterXML/jackson-datatype-jdk8 so this will move to that module. |
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. |
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. |
That is unfortunate since this new feature (arguably best if not requiring @JsonCreator) is revolutionary and would make Jackson a clear winner. |
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 |
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 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.
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. |
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). |
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. |
@lpandzic Only one is currently allowed for constructor, and one for factory method. |
Ok one other relevant thing wrt But there is still the problem that distinction between two types of creators:
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 This would only help in this particular problem, and not remove the need to use |
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? |
What I meant by "one for constructor, one for factory" method is that it is possible to technically have two instances of As to difference, I think the only case is simple one-argument case like:
where question is whether this should be mapped from JSON String (delegating creator), or from JSON Object like 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 A prototype would be good. |
@mmc41 @cowtowncoder |
@lpandzic sorry, haven't had time to look into this. |
FWIW, now #421 is fully resolved (for 2.5), so at least we have clean baseline. |
What needs to be done for implicit constructors? |
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 |
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. |
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. 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. |
The change should only affect this module, of course.
Have those rules changed in past few versions and what can be done about this? |
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 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 Now: in use cases where Jackson itself is embedded, there is nothing preventing container from offering specific alternate defaults. |
@lpandzic Actually, having written all of above, I realized that one easy thing that 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 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. |
@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. |
@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. If I remember Scala discussions correctly, part of the problem was that there are multiple things that can become implicit setter/getter? |
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). |
FWIW, I created #569 |
So if I understood you correctly, Tatu, jackson-databind parameters API needs to be enhanced in order to eliminate the need for |
@lpandzic Right, I think central policy and its implementation is something 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). |
All interested in this problem should take a look at this discussion and provide feedback. |
To serialize/deserialize immutable objects currently you have to add annotations like this:
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:
The text was updated successfully, but these errors were encountered: