No exception logged when JSON request conversion fails in an annotated service

[trustin]

For example:

public class MyService {
    @Post("/post")
    void post(MyValue value) {
        ...
    }
}

public class MyValue {
    final String value;
    public MyValue(String value) {
        this.value = value;
    }
}   

When an HTTP client posts a JSON document:

curl -X POST \
  http://localhost:8080/post \
  -H 'Content-Type: application/json' \
  -H 'Host: localhost:8080' \
  -d '{ "value": "foo" }'

.. a 400 Bad Request response is sent by the server, but no log message is written, making it hard to know what is wrong. The problem goes away if a user annotates MyValue.<init> with @JsonCreator and the parameter value with @JsonProperty("value").

[sivaalli]

Is it possible for me to take a look at this and possibly fix it by adding a log statement?

[trustin]

@sivaalli Sure! Thanks a lot for looking into it. 🙇

[sivaalli]

Hi @trustin , while making a change I got a doubt and wanted to get it clarified. If a ExceptionHandlerFunction is declared and the conversion fails(at runtime), then the function will be able to catch exception and handle it. If we log it in JacksonRequestConverterFunction and the ExceptionHandlerFunction that the user provided also logs it, then log appears twice. Can you please let me know what do you think.

public class Armeria {

    public static void main(String[] args) throws ExecutionException, InterruptedException {
        Server build = new ServerBuilder()
                .port(8080, SessionProtocol.HTTP)
                .annotatedService("/", new MyService()).build();
        build.start().get();

    }

    private static class MyService {

        @ExceptionHandler(LoggingExceptionHandlerFunction.class)
        @Post("/a")
        public HttpResponse a(MyValue value){
            System.out.println(value.value);
            return HttpResponse.of(200);
        }
    }

    public static class MyValue{
        @JsonProperty("value")
        public String value;
    }

    public static class LoggingExceptionHandlerFunction implements ExceptionHandlerFunction{

        @Override
        public HttpResponse handleException(ServiceRequestContext ctx, HttpRequest req, Throwable cause) {
            System.out.println(cause);
            return HttpResponse.of(400);
        }
    }
}

[trustin]

That is an excellent question, @sivaalli. IIRC, we already have a fallback exception handler that performs some logging: com.linecorp.armeria.internal.annotation.DefaultExceptionHandler.

Because JacksonRequestConverterFunction wraps the Jackson exception with an IllegalArgumentException, it is automatically and silently translated into 400 Bad Request by DefaultExceptionHandler.

What we could do in my opinion is to make JacksonRequestConverterFunction translate the Jackson exception into an IllegalArgumentException only for JSON parse errors. What do you think?

[sivaalli]

Thank you @trustin for your reply. Your plan sounds good to me. Currently JacksonRequestConverterFunction catches JsonProcessingException(a generic Jackson exception) and re-throws IllegalArgumentException. I will see if I can target subtype of JsonProcessingException that is thrown for JSON parse errors.

And since we already have aDefaultExceptionHandler, I will also add a log statement too in there. So that we know why JSON parse error happened(solves the original intent of this issue) and when user provides his/her implementation, then it is given priority over default implementation and will let user handle exception and(or) log it(also solves logging exception twice). Do you think this is a good idea?

[trustin]

Yeah, I think it's good. If other users report about the increased warning logs, we could reconsider it.