ServerJacksonMessageBodyReader and JacksonBasicMessageBodyReader "should" catch JsonProcessingException

[damonsutherland]

Describe the bug

When de-serializing JSON in a request, we want to catch client input errors due to malformed data. Unfortunately, we have run into situations where the exception Jackson is throwing is one of several JsonProcessingExceptions, i.e., a JsonMappingException, JsonParseException or DatabindException. As a result, the intended BAD_REQUEST is met with a SERVER_ERROR.

Using ExceptionMappers can work, if we are diligent in catching JsonProcessingException's in any location we use an ObjectMapper. If we are not diligent, then an ExceptionMapper will lead to a client error when it is actually a server error.

Corresponding locations:
JacksonBasicMessageBodyReader

ServerJacksonMessageBodyReader

Updating these locations to catch JsonProcessingException should resolve the issues we are seeing.

Expected behavior

BAD_REQUEST is thrown when input is malformed.

Actual behavior

SERVER_ERROR is thrown.

How to Reproduce?

This is a simplified version of what we are doing, only the details causing the issue are included.

public class Data {
  private String type;

  @JsonCreator
   public Data(
      @JsonProperty("type") String type
      ...
   ) {
      this.type = Objects.requireNonNull(type);
   }
}

Deserialize with the following line of code (throws a ValueInstantiationException):

mapper.readValue("{}", Data.class)

When via a controller, i.e., we should get 400 if the request body is "{}".

public Controller {

  @POST
  @Path("/")
  public void receive(Data data) {
  
  }
}

Output of uname -a or ver

Darwin Kernel Version 21.6.0: Thu Sep 29 20:12:57 PDT 2022; root:xnu-8020.240.7~1/RELEASE_X86_64

Output of java -version

17.0.3 (Eclipse Adoptium 17.0.3+7)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.8.3-Final

Build tool (ie. output of mvnw --version or gradlew --version)

------------------------------------------------------------ Gradle 7.3 ------------------------------------------------------------ Build time: 2021-11-09 20:40:36 UTC Revision: 96754b8c44399658178a768ac764d727c2addb37 Kotlin: 1.5.31 Groovy: 3.0.9 Ant: Apache Ant(TM) version 1.10.11 compiled on July 10 2021 JVM: 17.0.3 (Eclipse Adoptium 17.0.3+7) OS: Mac OS X 12.6.1 x86_64

Additional information

No response

[quarkus-bot]

/cc @evanchooly, @geoand, @gsmet

[damonsutherland]

As a side note, the work-around is easy; just posting this bug to help keep the intention of the ServerJacksonMessageBodyReader pure.

[geoand]

Are you sure that JsonProcessingException isn't too broad? I can't really tell from its Javadoc whether it's only about malformed input

[damonsutherland]

We could certainly be more targeted. JsonProcessingException seemed appropriate to me. That said, there are two general issues I am hoping to address:

1. Malformed JSON: Missing a closing brace, comma's in the wrong place, i.e., basic syntax issues. From what I can tell, these types of issues manifest as a JsonParseException (which inherits StreamingReadException, which inherits JsonProcessingException).
2. Syntactically correct JSON with invalid constraints: Missing required field(s). These types of exceptions seem to manifest as ValueInstantiationException when Object construction is initiated by constructor injection or factory method. ValueInstantiationException is a sibling to MismatchedInputException and InvalidDefinitionException. All of these inherit JsonMappingException, which is slated to go away in favor of DatabindException. DatabindException inhereits JsonProcessingException.

Looking at the docs, there are basically two paths through Jackson, parsing and generating. JsonProcessingException is used in both paths, so in that sense, it is too broad. However, in the context of a MessgeBodyReader, only the parsing path is exercised.

The other place that it may be too broad is with InvalidDefinitionException, which generally would indicate a problem with the target type definition and not necessarily the input. Generally, I would expect a mistake that would prevent de-serialization to be caught in unit tests, but when used in a broader framework like Quarkus, it is probably naive to think unit tests would be used that vigorously. So ...

While JsonProcessingException works for our case, it would probably be better to be surgical and catch the following Exceptions:

• ValueInstantiationException
• MismatchedInputException
• StreamReadException (it will eventually replace JsonParseException)

There maybe others, but I haven't done an exhaustive search. Thoughts?

[geoand]

While JsonProcessingException works for our case, it would probably be better to be surgical and catch the following

I think this is the safest thing to do. Would you like to contribute this fix?

[damonsutherland]

Sure

[geoand]

@damonsutherland just checking in: are you still up for making this contribution or do you want me to add the fix?

[damonsutherland]

Sorry, I’ve been on vacation. I am back today though. I will work on it this afternoon. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Georgios Andrianakis ***@***.***> Sent: Monday, November 28, 2022 6:47:46 AM To: quarkusio/quarkus ***@***.***> Cc: Damon Sutherland ***@***.***>; Mention ***@***.***> Subject: Re: [quarkusio/quarkus] ServerJacksonMessageBodyReader and JacksonBasicMessageBodyReader "should" catch JsonProcessingException (Issue #29316) @damonsutherland<https://github.com/damonsutherland> just checking in: are you still up for making this contribution or do you want me to add the fix? — Reply to this email directly, view it on GitHub<#29316 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACNNTM7WQCNIGKR22CIVYG3WKSZYFANCNFSM6AAAAAASCP4YMA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[geoand]

Thanks for the update!

[damonsutherland]

@geoand, I submitted the following PR (#29548).

[damonsutherland]

@geoand, I wasn't happy with my work and I found a couple things I missed. Just letting you know I am still working on this and will issue a new PR in the next day or so.

I would like to close/reject the current PR. Are you OK with that?

[geoand]

That's perfectly fine, not a problem at all

[damonsutherland]

@geoand, my replacement PR has been linked above.

Thanks again for all your help.