Differentiate between input and code exceptions on deserialization

[nickbabcock]

Summary of dropwizard discussion, specifically the following comment:

When a server receives a payload, attempts to deserialize it, and an exception is raised, there are two possibilities in our eyes:

• A client sent unexpected input and should receive a 4xx response back
• There is a programmatic error (forgot proper annotations, etc) and a 5xx response should be returned

The issue is that both of these situations may throw the same root exception JsonMappingException, which makes it hard to determine what response should be returned. I propose a new exception: JsonInputMappingException (name subject to change) or increase usage of InvalidFormatException

Examples

Examples that use just a plain JsonMappingException where new JsonInputMappingException or InvalidFormatException would be used

• Expecting an object, but JSON is false throws a JsonMappingException of "Can not construct instance of foo: no boolean/Boolean-argument constructor/factory method to deserialize from boolean value (false)". This should be a client error because the wrong json type was submitted.
• Expecting a list but given a string throws a JsonMappingException of "Can not deserialize instance of java.util.ArrayList out of VALUE_STRING token"
• Deserializing a DateTime from [1,1,1,1] throws a JsonMappingException of "Unexpected token (VALUE_NUMBER_INT), expected END_ARRAY: Expected array to end."

The Dream

The code needed to determine if it client or server issue:

final JsonMappingException ex = (JsonMappingException) exception;
final Throwable cause = Throwables.getRootCause(ex);

// Exceptions that denote an error on the client side
final boolean clientCause = cause instanceof InvalidFormatException ||
    cause instanceof PropertyBindingException ||
    cause instanceof JsonInputMappingException;

if (!clientCause) {
    LOGGER.error("Unable to serialize or deserialize the specific type", exception);
    return Response.serverError().build();
}

or maybe even:

final boolean clientCause = cause instanceof JsonInputMappingException

and have InvalidFormatException and PropertyBindingException extend from JsonInputMappingException, though I do not know if this would be 100% applicable.

Considerations

By creating a new exception that is supposed to represent bad input, exceptions thrown due to bad input in third party libraries that use plain JsonMappingException, if not changed, can be unexpectedly interpreted as a server/programmatic error. I believe it is best to err on the side of blaming the client/input. Instead, creating a JsonProgrammaticMappingException may fix this problem.

Would JsonParseException be a part of JsonInputMappingException

The suggested exception, JsonInputMappingException, does not apply to serialization. An error on serialization is almost certainly a programmatic error, though I can see a situation where a custom serializer throws on certain input. I understand this to be less of a use case.

Do not create a new exception JsonInputMappingException or JsonProgrammaticMappingException, but instead throw instances of InvalidFormatException for the given examples (more examples may exist, but I'm not aware of them at this point).

Conclusion

Let me know if you need help recreating any examples. I'm torn whether it's best to create a new exception or use InvalidFormatException more.

[cowtowncoder]

I am open to adding an intermediate (or maybe even leaf level) JsonInputMappingException to simplify things. But as an alternative or perhaps complementary thing, perhaps there should also be something to indicate the other major case, where POJO definition itself is invalid. For example, case where there are conflicting/ambiguous setters, missing @JsonProperty for creator, or one of other annotation-related problems. Such problems would be "server problems" (to get a 5xx response).
Not sure if reference JsonProgrammaticMappingException might mean such alternative exception type?

Anyway: I do like the idea of adding a small number (one or two) new types of (more) semantic exceptions, instead of overloading small number of existing exceptions, because existing ones have some definition (except for loose JsonMappingException):

• PropertyBindingException: base type for:
  • IgnoredPropertyException thrown when a non-mappable property is encountered, but one for which there is ignoral directive for it AND configuration indicates this is an error case
  • UnrecognizedPropertyException when non-mappable property with no ignorals is encountered, configuration indicates this is an error case
• InvalidFormatException thrown when JSON token type could be acceptable, but value itself is not valid (wrong textual format, numeric value out of range, number of array elements wrong)
• InvalidTypeIdException problem with type id (unmappable/unresolvable)

From this list I think there could be one other actual semantic exception: invalid-coercion.
It could be used in cases where JSON token type is one that:

• would NOT directly map; for example, JSON String value for java.lang.Number
• under some circumstances could possible be coerced, but
• coercion not allowed ("strict numbers")

Then again, invalid coercion could also just be mapped to InvalidFormatException.
Extending from this, one could argue that mismatch from, say, JSON Array into java.lang.String is a failed coercion too.

On timing/versioning; new types will have to go in 2.9, so we can plan on that. I am still open to smaller incremental improvements; mostly cases where existing input-related problems are NOT mapped as InvalidFormatExceptions (like the recently fixed case of bad URLs) but wrapped as generic JsonMappingExceptions.

As to JsonParseException: it can not be a JsonMappingException or JsonInputMappingException for simple reason that it is defined in jackson-core. While it might be possible to juggle things, in theory, to allow different grouping, I don't think potential compatibility problems are worth the hassle here.

[nickbabcock]

I agree with everything stated, and JsonProgrammaticMappingException was my poor attempt at naming the alternative exception type 😛

Based on your description of InvalidFormatException then I believe the following example should use it instead of JsonMappingException:

Deserializing a DateTime from [1,1,1,1] throws a JsonMappingException of "Unexpected token (VALUE_NUMBER_INT), expected END_ARRAY: Expected array to end."

as [1,1,1] is valid but [1,1,1,1] is invalid (currently throws JsonMappingException, instead of what sounds like should be a InvalidFormatException)

The issue of coercion, like you pointed out is a subtle one, but I feel like there is a distinction between "a coercion exists, but failed (InvalidFormatException)" and a coercion doesn't exist (JsonInputMappingException)", for instance. This should convey the difference between different values of the same type may yield different results (exception vs non-exception) and no matter what value, the type is cannot be coerced and will never be deserializable

Don't worry about how to version everything. Do what you gotta do and we can discuss what version fits with dropwizard for the time being.

[cowtowncoder]

On naming, one suggestion: instead of JsonInputMappingException maybe:

JsonInputMismatchException

On programmatic... hmmh. Bit torn on whether it should start with Json at all, since problem is not with Json at all. For sake of consistency might make sense, but then again not all exception names start with that anyway. So maybe:

InvalidPojoDefinitionException

[nickbabcock]

InvalidPojoDefinitionException

Looks nice, though some may argue Pojo to be misleading due to all the ways Jackson can serialize/deserialize. Maybe even drop it for

InvalidDefinitionException

---

Yeah, I can get behind JsonInputMismatchException, but if we're allowing ourselves to drop Json, I smell an opportunity 😛

InputMismatchException

[cowtowncoder]

Right, "pojo" and "bean" are both sub-optimal. Could just start with base exception of InvalidDefinitionException. I am actually starting to add methods in SerializerProvider / DeserializationContext to help troubleshoot, and so far they are either for type, or property (type + property). So could probably pass those as metadata, as optional fields.

So I think:

• InputMismatchException for various "input not matching (valid) definitions" (client problem)
• InvalidDefinitionException for various "problem with Java type annotations/configuration" cases (server problem)

would be the way to go then.

[cowtowncoder]

FWIW, starting to add convenience methods in SerializerProvider / DeserializationContext, to help produce what will become InvalidDefinitionException.

[cowtowncoder]

Working on this. One unfortunate finding: there is already java.util. InputMismatchException. Not sure if we should use different name, as it's neither trivial nor fatal problem... just unfortunate; noticed when I had a naming conflict.

[nickbabcock]

Right, donning my naming cap:

• ConflictingInputException / InputConflictException
• OpposingInputException
• DifferingInputException / InputDifferException
• ClashingInputException / InputClashException
• ContrastingInputException

[cowtowncoder]

Very creative naming suggestions! I'll go, for now, with InputMismatchException (could try MismatchingInputException too). I'm actually making decent progress, last night got system mostly done, now just need to handle couple of odd test fails.

[nickbabcock]

Good to hear, let me know when/if you want me to try anything out on Dropwizard.

[cowtowncoder]

Ok, so, after all tests pass, here's the first draft. I didn't change the name (another option: InputMappingException?), so that might change. But most usage should be converted, esp. on deserialization side. I did also make serialization side use InputDefinitionException for many things, although there's not necessarily counterpart to input problem (it is possible to get lower level JsonGenerationException due to bad state of course).

There are some remaining cases where I'm not sure what to do -- specifically, catch-wrap-rethrow for exceptions from user code (Creators, setters, getters) -- but first things first.

[nickbabcock]

I wanted to do a quick test with dropwizard, and I installed snapshots of 2.9 for annotations and core, but databind is failing. Looks like the pom references a parent pom, is this correct? Because this repo doesn't have a parent pom.

[cowtowncoder]

@nickbabcock you need to built that locally, although I think I can do a snapshot deploy for convenience

[nickbabcock]

Oof, didn't see Jackson-parent repo 😊

I should be all set

[nickbabcock]

Ok tried out 2.9 on dropwizard and the following line was almost a 100% success

final boolean return500code = cause instanceof InvalidDefinitionException;

The one case that failed:

Can not construct instance of io.dropwizard.jersey.jackson.OkRepresentation: no boolean/Boolean-argument constructor/factory method to deserialize from boolean value (false)

Which is when a user submits json of

false

when expected json looks like

{
  "message": 1,
  "date": "2012-01-01"
}

The lack of a boolean constructor is causing a InvalidDefinitionException to be thrown

[cowtowncoder]

Definitely there are cases where type is not explicitly chosen, and code probably relies on throw-catch-rethrow. I'll hunt down this one, but it would be useful to add stack trace to simplify the task, for cases you encounter.

[cowtowncoder]

Hmmh. I can not reproduce this issue at this point. I'll do a new snapshot deploy to make sure latest one is out.

[nickbabcock]

I cloned and installed all the necessary repos locally (parent, core, annotations, databind).

Given

    @Test
    public void runTest() throws IOException {
        final ObjectReader reader = new ObjectMapper().readerFor(Tester.class);
        reader.readValue("false");
    }

    public class Tester {
        private Integer message;

        @JsonProperty
        public Integer getMessage() {
            return message;
        }

        @JsonProperty
        public void setMessage(Integer message) {
            this.message = message;
        }
    }

Results in stacktrace:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Can not construct instance of io.dropwizard.jersey.jackson.Tester: no boolean/Boolean-argument constructor/factory method to deserialize from boolean value (false)
 at [Source: false; line: 1, column: 1]

    at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
    at com.fasterxml.jackson.databind.DeserializationContext.instantiationException(DeserializationContext.java:1455)
    at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1019)
    at com.fasterxml.jackson.databind.deser.ValueInstantiator.createFromBoolean(ValueInstantiator.java:279)
    at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createFromBoolean(StdValueInstantiator.java:385)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromBoolean(BeanDeserializerBase.java:1329)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:168)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:150)
    at com.fasterxml.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:1623)
    at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1217)

[cowtowncoder]

Ah! My mistake -- I didn't think this through, and assumed InvalidDefinitionException was correct. But I see it's not, since it is input problem. Well I guess it could be either of course, but more likely input mismatch, in absence of other information.

[cowtowncoder]

Fixed via #1404.

One other thought: although I do prefer not to add "json" any more, since it's but one of input formats; it would be possible to use JsonInputMismatchException, or, perhaps even JacksonInputMismatchException. Auto-import of JDK exception gets old pretty fast otherwise...

[nickbabcock]

I'm +1 for preferring prefixing exceptions with Jackson over Json.

Maybe some future work for Jackson 3/4 😉

[cowtowncoder]

Renaming of other exceptions would be for later versions for sure. But in this case we still have full control of the new names, before 2.9.0 is released. I am also ok with naming discrepancy; Json is used for some exceptions (low-level), not so for higher level. Adding Jackson as 3rd choice is a minor additional offence and I don't think it's worse (maybe not better either...) than Json.
Haven't gotten much feedback via mailing list on these things, otherwise would ask there.