Allow deserialization of unknown Enums using a predefined value

[AlejandroRivera]

Use case:

When using Jackson to deserialize a json payload coming from a 3rd party provider, it's common not to have the entire list of enum values required to support proper Enum deserialization. Jackson already offers a way to deserialize unknown values as null, nevertheless, it can be more useful to be able to deserialize them into a pre-determined value (perhaps named "UNKNOWN") to avoid NPEs or other issues.

The current way to achieve this desired effect is to have a custom deserializer or a @JsonCreator constructor. This approach, while effective, is also somewhat cumbersome since we lose the ability to rely on other annotations like @JsonProperty or @JsonValue to serialize and deserialize values.

Proposal

Through the creation of a new Jackson annotation @JsonEnumDefaultValue, developers should be able to annotate a specific enum that they wish to use as default. This annotation, along with a new DeserializationFeature toggle, should make it easy to accomplish the desired effect.

Complimentary Jackson annotation:

/**
 * Marker annotation that can be used to define a default value
 * used when trying to deserialize unknown Enum values.
 * <p>
 * This annotation is only applicable when the
 * {@link com.fasterxml.jackson.databind.DeserializationFeature#READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE}
 * deserialization feature is enabled.
 * <p>
 * If the more than one enum value is marked with this annotation,
 * the first one to be detected will be used. Which one exactly is undetermined.
 */
@Target(ElementType.FIELD)
@Retention(RetentionPolicy.RUNTIME)
@JacksonAnnotation
public @interface JsonEnumDefaultValue
{
}

Open questions:

• Build-related: how do you handle compilation dependencies across jackson projects?
• Is there a way to avoid the introspection penalty incurred in EnumResolver from scanning for the new annotation if the feature toggle is disabled?
• What's the right way to deal with logging? I see code where the default Java logging is used, but other places where it's not and just comments mentioning logging.

[cowtowncoder]

Ok makes sense at usage level. I'll have to think about annotation used; it would be nice if an existing one could be reused, but at the same time it is dangerous to overload meaning too much, so this may work fine.

One suggestion for implementation is that the only way to access any of the annotations should be via AnnotationIntrospector, and not through direct lookups. So it would be necessary to either add a new method there (with default implementation of returning null), or, if possible (probably not) try to see if a change to existing method would work. Latter usually only works for methods that return a structure value object, in which case it may be possible to change that value definition.

On questions:

• There is no real support for enforcing dependencies, except through CI system (Travis). However, as long as we have full set of branches, this should tease out compilation errors, as well as test suite.
  • Challenge is to balance needs to upgrade master for next minor version vs overhead of merges for fixes -- I went ahead and upgraded jackson-annotations to be 2.8.0-SNAPSHOT to allow merging of changes (for annotations, no changes to any of API accepted for patch versions, only for 2.x.0 minor releases)
• I'll have to see the lookup behavior: but EnumResolver should only ever be created once per type so unless calls are made during deserialization this should not be an issue.
• Logging: Jackson does not use logging at all. This is conscious decision. The only deviation are fatal errors which may in rare cases use java.util.logging (Afterburner Module). Errors should be reported using Exceptions; other communication if necessary through pluggable listeners/handlers (there are some limited cases but these are not common).

[AlejandroRivera]

One suggestion for implementation is that the only way to access any of the annotations should be via AnnotationIntrospector

Ok, i can do that, but i might need some more guidance because currently, only the method com.fasterxml.jackson.databind.util.EnumResolver#constructFor receives an AnnotationIntrospector as parameter, but the other two places in EnumResolver where the lookup happens don't have any other option other than constructing a new AnnotationIntrospector manually (see constructUsingMethod and constructUsingToString). Is this desirable?

So it would be necessary to either add a new method there (with default implementation of returning null)

Yeah, a new method seems a better idea since i wasn't able to think of a way of modifying an existing one to suit both use-cases. I could make a change in the AnnotationIntrospector and will make it more generic so there's no need to override in the Jackson-specific implementation. The signature will be:

public <T extends Annotation> Enum<?> findFirstAnnotatedEnumValue(Class<Enum<?>> enumClass, Class<T> annotationClass) { ... }

And the EnumResolver will invoke it like so:

Enum<?> defaultEnum = ai.findFirstAnnotatedEnumValue(enumCls, JsonEnumDefaultValue.class);

Let me know your thoughts on how to handle the lack of an AnnotationIntrospector as mentioned above and i'll commit the changes for you to review.

[AlejandroRivera]

@cowtowncoder based on your comment on logging, i'm not sure if it would it be best to wrap this into an exception and let it bubble up or just swallow it and return null?

[cowtowncoder]

Good question. I think I would start by throwing an exception (wrapping in IllegalArgumentException, perhaps -- that gets converted into more meaningful JsonProcessingException or such, with more information). It will be easier to allow suppressing it later on if need be.

[cowtowncoder]

@AlejandroRivera Yes, inaccessibility of information can often be problematic: originally AnnotationIntrospector was envisaged to be quite stateless -- if I was to design API again, I would probably pass MapperConfig (that is, either SerializationConfig or DeserializationConfig).
But since information is minimal, choices are to either pass more, or use a work-around.

Adding new version of method is acceptable if there is a way to retrofit old code, make it call new method, deprecate old method (you may be able to find examples). This not trivial to make work as intended wrt backwards-compatibility (it tends to violate things as this part of Public API and not fully internal), but there is a trade-off to make wrt improving behavior vs maximal compatibility.

So basically I am open to suggestions; I would like to help more, but at this point I am limited by time (last work day before vacation). I will be back with more time in about 1 week and should be able to help more if need be. I will try to check emails and updates, but response times may be bit slow.

[AlejandroRivera]

After examining the codebase a bit better, i think i found the ideal way of handling the logic without creating new package dependencies. It goes as follows:

EnumResolver invokes AnnotationIntrospector asking for a given Enum's default value. The base method in the abstract AnnotationIntrospector returns null since there's no global concept of a default Enum value. The JacksonAnnotationInstrospector invokes ClassUtil asking for the first Enum field annotated with @JsonEnumDefaultValue since it's this is Jackson-specific concept. ClassUtil then uses reflection to find the first field/value marked with a generic annotation.

The only downside I see of this approach is the link from EnumResolver to the specific AnnotationIntrospector. Like i mentioned before, a couple of methods that need it don't have a reference to it. For the time being, i hardcoded the instantiation to the JacksonAnnotationIntrospector.

I'm planning on looking some more through the code to see if it's possible to pass the reference from other places, but in order to maintain backwards binary compatibility (since you mentioned that this EnumResolver is somewhat of a public-API), i'm going to need to have two methods (one with the AnnotationIntrospector and one without) and will have to hardcode a default instance to use (or even a null value). How does that sound?

PS. Enjoy your vacation!

[AlejandroRivera]

@cowtowncoder: I think I'm done with my changes. Let me know what you think once you have time to review.

[cowtowncoder]

@AlejandroRivera Thank you for the updates! I am leaving for a 1 week vacation now, so it will take a while, but I will review this after I come back. Given that it involves changes to API it can (and needs to) go in 2.8, so we have quite a bit of time.

[cowtowncoder]

@AlejandroRivera Looks good, good job! I merged it, made minor tweaks (need to delegate via introspector pair), will be included in 2.8.

[AlejandroRivera]

Sweet! Thanks for the tweaks 👍

LMK if i can help with anything else related to this change (e.g. documentation or anything).

[cowtowncoder]

@AlejandroRivera I think we are good -- your patch was very well made, including javadocs and all. Much appreciated!

[AlejandroRivera]

I was actually thinking of the documentation found in the Wiki, but i take it you've got that covered. Let me know otherwise :)

[cowtowncoder]

@AlejandroRivera Ah. Actually yes good point: it would be great to get an update for

https://github.com/FasterXML/jackson-databind/wiki

Not sure what'd be the best way; perhaps if you can add a snippet here for change?
(github doesn't seem to have a way to give access to just wiki modifications, or PRs?)

[AlejandroRivera]

Added documentation for the config here:
https://github.com/FasterXML/jackson-databind/wiki/Deserialization-Features/_edit#value-conversions-coercion

Also here https://github.com/FasterXML/jackson-annotations/wiki/Jackson-Annotations#deserialization-details

[cowtowncoder]

Excellent.