Support READ_UNKNOWN_ENUM_VALUES_AS_NULL with @JsonCreator

[joelittlejohn]

The deserialization feature READ_UNKNOWN_ENUM_VALUES_AS_NULL allows enum values that are not recognised to take a null value when parsed. Before this commit, this deserialization feature did not work whenever an enum had a method marked @JsonCreator, and the enum creator method itself had
to choose null in the case of unknown input.

With this change, if an enum creator method throws an IllegalArgumentException then this is considered to be an unknown value and (if READ_UNKNOWN_ENUM_VALUES_AS_NULL is active) then null will be
used.

[cowtowncoder]

Makes sense, thank you for contributing this!

Quick question? Have I already asked for a CLA? I thought I might have, and if so we are good.
If not, I'd need one filled before merging, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the quickest usual way is to print, fill & sign, scan, email to info at fasterxml dot com.

Looking forward to merging this in (I can backport in 2.8 as well).

[joelittlejohn]

Done, cheers! 👍

[cowtowncoder]

Hmmh. Ok. I did merge this, but I am bit uncomfortable with swallowing the exception part; especially for wide applicability. Part of this is that throwing (or, rather, constructing) exceptions is very costly; but the other part (wide applicability) that this could mask real problems.

But maybe a reasonable compromise would be only "convert" IllegalArgumentException (and subtypes), and document that? This would seem to match the intent, but not block exceptions that are more likely to indicate other severe issues? (NPE, ArrayIndexOutOfBounds).

[joelittlejohn]

@cowtowncoder maybe I'm not understanding you entirely here, but my intent with this change is to only 'convert' IllegalArgumentException (and subtypes), and only do this in the case that READ_UNKNOWN_ENUM_VALUES_AS_NULL is activated.

Here's the snippet (my new lines have a +):

             Throwable t = ClassUtil.throwRootCauseIfIOE(e);
 
 +            if (ctxt.isEnabled(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL) &&
 +                    t instanceof IllegalArgumentException) {
 +                return null;
 +            }
 +
              return ctxt.handleInstantiationProblem(_valueClass, value, t);

Does that not do what I think it does?

In the case that t is anything other than an IllegalArgumentException, or in the case that READ_UNKNOWN_ENUM_VALUES_AS_NULL is not activated, ctxt.handleInstantiationProblem(_valueClass, value, t) will handle t in the normal way and throw it.

[cowtowncoder]

@joelittlejohn as is, it would catch all kinds of Exceptions (although not IOExceptions). So I'm just worried about masking, say, NullPointerException. I think the real problem here is just that due to call via reflection, constructor does not have information to know how to handle unknown values; and then caller is trying to decipher intent out of exception.

But I think that if just IllegalArgumentException (and subtypes) are considered to convert into null I'm fine with this.

[joelittlejohn]

@cowtowncoder As I understand the code, if the root cause is a NullPointerException then ctxt.handleInstantiationProblem will throw this, just as it always has done:

jackson-databind/src/main/java/com/fasterxml/jackson/databind/DeserializationContext.java

Line 1029 in 3e75d54

public Object handleInstantiationProblem(Class<?> instClass, Object argument,

[cowtowncoder]

Acutally, never mind; I was being blind. Check is being already done for IllegalArgumentException. :-D
For some reason I just noticed basic "catch (Exception ...)" and blanked on further checks.
So it's fine; apologies for noise.

[yichuan1118]

quick question: has this fix been backported to 2.8?

[cowtowncoder]

@yichuan1118 As per my earlier comment on a commit:

Backported 1642 in 2.8.9