spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.64k stars 38.14k forks source link

Add Javadoc to MvcResult getResponse().getErrorMessage() #31386

Closed berse2212 closed 11 months ago

berse2212 commented 1 year ago

We are currently in the middle of migrating to spring 6 (via Spring Boot 3)

In Spring 5 we checked the error Message in SpringBoot Tests like this:

this.mvc.perform(put("/badUrl/abcde"))
                .andExpect(status().isBadRequest())
                .andExpect(result -> assertThat(result.getResponse().getErrorMessage()).isEqualTo(ERROR_MSG));

Now with the change to ErrorDetail weg cannot get the error Message via the getErrorMessage() Method:

MockHttpServletResponse:
           Status = 400
    Error message = null
          Headers = [Vary:"Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers", Content-Type:"application/problem+json", X-Content-Type-Options:"nosniff", X-XSS-Protection:"0", Cache-Control:"no-cache, no-store, max-age=0, must-revalidate", Pragma:"no-cache", Expires:"0", X-Frame-Options:"DENY"]
     Content type = application/problem+json
             Body = {"type":"about:blank","title":"Bad Request","status":400,"detail":"Fehler beim Zuweisen. Es wurden keine Änderungen der durchgeführt.","instance":"/badUrl/abcde"}
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

org.opentest4j.AssertionFailedError: 
expected: "Fehler beim Zuweisen. Es wurden keine Änderungen der durchgeführt."
but was: null
Expected :"Fehler beim Zuweisen. Es wurden keine Änderungen der durchgeführt."
Actual   :null

Now the workaround would be validate the JSON and get the detail field manually, but I would expect the MockMvc Result to handle this. Otherwise the get ErrorMessage is highly misleading. Or am I misunderstanding something here?

bclozel commented 1 year ago

This .getErrorMessage() method is about the Servlet response itself, and more precisely the HttpServletResponse#sendError method. This is not about the recent ProblemDetail support.

Otherwise the get ErrorMessage is highly misleading.

Maybe you would like to send a PR to add a Javadoc section for this method to disambiguate the current state of things?

Thanks!

berse2212 commented 1 year ago

I am not sure I understand your response correctly. We are not calling the HttpServletResponse#sendError method but rather just throw a ResponseStatusException like this:

    throw new ResponseStatusException(HttpStatus.BAD_REQUEST, ERROR_MSG);

And rethrow it in the GlobalExceptionHandler:

    @ExceptionHandler({ResponseStatusException.class})
    public void handleResponseStatusExceptions(ResponseStatusException e) {
        throw e;
    }

Now that the automatic ResponseStatusException handling has changed to ProblemDetail like described here: https://stackoverflow.com/questions/75029947/error-response-body-changed-after-boot-3-upgrade we had to change our tests for these cases and manually parsing the JSON to get the error message.

bclozel commented 1 year ago

I am not sure I understand your response correctly. We are not calling the HttpServletResponse#sendError method but rather just throw a ResponseStatusException like this:

Yes, I understand. Previously, letting an exception unhandled would reach the Servlet container as HttpServletResponse#sendError and would render an error page with the Servlet container configuration. See https://github.com/spring-projects/spring-framework/blob/a53d3f3cea235220a50dcdb631687eba65b5a298/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/annotation/ResponseStatusExceptionResolver.java#L137-L148

Here, the error is instead rendered as a ProblemDetail JSON response, so the getErrorMessage() call doesn't apply anymore. From a Servlet perspective, this is just a regular JSON response.

we had to change our tests for these cases and manually parsing the JSON to get the error message.

I think this is the right approach. Feel free to create another issue if you have ideas on how to improve this.

I think we'll reuse this issue to add the missing javadoc on this method.