Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MockMvc ignores HTTP status code overridden by filter [SPR-11760] #16382

Closed
spring-projects-issues opened this issue May 5, 2014 · 11 comments
Closed

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented May 5, 2014

Gena Makhomed opened SPR-11760 and commented

The full source code for HttpStatusOverrideFilter is available in the attachment.

In production env., all works fine, but when I use JUnit + MockMvc, the HTTP Status Code overridden by my filter gets ignored.

Test code fragment:

@Test
public void badUri() throws Exception {
    mockMvc.perform(post("/unknown-request-uri")
            .andExpect(status().is(equalTo(230)));
}

Controller code fragment:

HttpHeaders httpHeaders = new HttpHeaders();
httpHeaders.set(X_HTTP_STATUS_OVERRIDE, "230");
return new ResponseEntity<>("none", httpHeaders, HttpStatus.INTERNAL_SERVER_ERROR);

In the production env., the filter works fine, and the HTTP status code is 230. With JUnit + MockMvc, the HTTP status code is 500, and the assertion fails.

Looks like this is bug in MockMvc.

Unit-test log fragment:

o.s.test.web.servlet.TestDispatcherServlet | Successfully completed request
c.p.i.util.filter.HttpStatusOverrideFilter | Status overridden '500' => '230'
.t.c.s.DirtiesContextTestExecutionListener | After test method: context
[DefaultTestContext@1895852 testClass = GateControllerTest, testInstance = com.pb.ivrcgate.controller.GateControllerTest@59e71, testMethod =
badUri@GateControllerTest, testException = java.lang.AssertionError: Response status
Expected: <230>
     but: was <500>, mergedContextConfiguration = [WebMergedContextConfiguration@f7e9dd testClass = GateControllerTest, locations =
'{classpath:/spring/context.xml}', classes = '{}', contextInitializerClasses = '[]', activeProfiles = '{dev}', resourceBasePath =
'src/main/webapp', contextLoader = 'org.springframework.test.context.web.WebDelegatingSmartContextLoader', parent = [null]]], class dirties
context [false], class mode [null], method dirties context [false].

Affects: 4.0.4

Attachments:

Issue Links:

  • #15486 Provide better intercepting model for controllers returning @ResponseBody
  • #16386 MockHttpServletRequest should not require setContent for non-null getInputStream() result
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 5, 2014

Sam Brannen commented

It looks like you simply forgot to register your HttpStatusOverrideFilter with the MockMvc instance.

To do that, invoke the addFilters(Filter... filters) method on the builder before you invoke build().

For example:

WebApplicationContext wac = ...;

MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(wac).addFilters(new HttpStatusOverrideFilter()).build();

// ...

Please give that a try and let us know if it addresses your issue.

Thanks,

Sam

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 6, 2014

Gena Makhomed commented

It looks like you simply forgot to register your HttpStatusOverrideFilter with the MockMvc instance.

No, I register it:

@WebAppConfiguration
@ActiveProfiles("dev")
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration("/spring/context.xml")
public class GateControllerTest {

    @Autowired
    private WebApplicationContext webApplicationContext;
    private MockMvc mockMvc;

    @Before
    public void setup() throws Exception {
        mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext)
                .addFilters(new HttpStatusOverrideFilter()).build();
    }

    @Test
    public void badUri() throws Exception {
        mockMvc.perform(post("/some-unknown-request-uri").accept(MediaType.APPLICATION_JSON))
                .andExpect(status().is(equalTo(230)));
    }
}

Also, in unit-test log you can see debug output from filter:

c.p.i.util.filter.HttpStatusOverrideFilter | Status overridden '500' => '230'

Fragment of controller source-code:

@Controller
public class GateController {

    private static final String X_HTTP_STATUS_OVERRIDE = "X-HTTP-Status-Override";

    // ...

    @RequestMapping
    public void unknownMappingHandler(HttpServletRequest request) {
        throw new InvalidRequestUriException(request.getRequestURI());
    }

    @ExceptionHandler
    @ResponseBody
    public ResponseEntity<ErrorResponse> exceptionHandler(Exception ex) {
        logger.info("", ex);
        HttpHeaders httpHeaders = new HttpHeaders();
        httpHeaders.set(X_HTTP_STATUS_OVERRIDE, ExceptionUtils.getHttpStatusOverride(ex));
        ErrorResponse errorResponse = ErrorResponseFactory.create(ExceptionUtils.getErrorCode(ex));
        return new ResponseEntity<>(errorResponse, httpHeaders, HttpStatus.INTERNAL_SERVER_ERROR);
    }
}

ExceptionUtils.getHttpStatusOverride(ex) return "230" for InvalidRequestUriException

And in unit test log I can see filter in stack trace of logged exception:

2014-05-05 22:16:18.646 | INFO  | pool-1-thread-1  | com.pb.ivrcgate.controller.GateController  |
com.pb.ivrcgate.exception.InvalidRequestUriException: /some-unknown-request-uri
        at com.pb.ivrcgate.controller.GateController.unknownMappingHandler(GateController.java:55) ~[classes/:na]
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.7.0_55]
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) ~[na:1.7.0_55]
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.7.0_55]
        at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_55]
        ........................................
        at org.springframework.test.web.servlet.TestDispatcherServlet.service(TestDispatcherServlet.java:62) [spring-test-4.0.4.RELEASE.jar:4.0.4.R
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:770) ~[javax.servlet-api-3.0.1.jar:3.0.1]
        at org.springframework.mock.web.MockFilterChain$ServletFilterProxy.doFilter(MockFilterChain.java:170) [spring-test-4.0.4.RELEASE.jar:4.0.4.
        at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:137) [spring-test-4.0.4.RELEASE.jar:4.0.4.RELEASE]
        at com.pb.ivrcgate.util.filter.HttpStatusOverrideFilter.doFilterInternal(HttpStatusOverrideFilter.java:25) [classes/:na]
        at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) [spring-web-4.0.4.RELEASE.jar:4.0.4.RELEASE]
        at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:137) [spring-test-4.0.4.RELEASE.jar:4.0.4.RELEASE]
        at org.springframework.test.web.servlet.MockMvc.perform(MockMvc.java:141) [spring-test-4.0.4.RELEASE.jar:4.0.4.RELEASE]
        at com.pb.ivrcgate.controller.GateControllerTest.badUri(GateControllerTest.java:41) [test-classes/:na]
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.7.0_55]
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) ~[na:1.7.0_55]
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.7.0_55]
        at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_55]
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) [junit-4.11.jar:na]
        ........................................

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 6, 2014

Rossen Stoyanchev commented

I see no (obvious) reason why this shouldn't work. Please provide a sample project here and I'll debug it.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 7, 2014

Gena Makhomed commented

Ok, I create pull request.

Filter HttpStatusOverrideFilter works fine in MockMvc and Servlet Conatiner only if present several filters, no matter first or last filter HttpStatusOverrideFilter will be.

In case if only one filter HttpStatusOverrideFilter present - bug observed in MockMvc and in Servlet Conatiner too.

I provide three tests - one failed and two works fine with HttpStatusOverrideFilter and HttpDumperFilter in any order.

Looks like this bug is not in MockMvc, but in Spring MVC code.

P.S.

For convenience full debug log of unit test will be written in target/test.log file.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 8, 2014

Rossen Stoyanchev commented

I had a look at the repro project. The HttpStatusOverrideFilter tries to change the status at the end but it is too late when the response is committed. Writing content to the response and flushing for example causes the response to be committed. Or it can happen if you write more content than the servlet response buffer can fit. Essentially it means the server has already told the client what the response status is, so you can no longer change it.

The reason having HttpDumperFilter makes a difference is because it wraps the response and overrides flushing. However that's more of a side effect. If you really want to set the status at the end, you have to wrap the response and buffer the response body. See for example how our ShallowEtagHeaderFilter is implemented. There may be other approaches. For example you may be able to extend ResponseEntity and override its getStatus() method but I'm only guessing based on the provided sample code.

At any rate this is expected behavior.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 22, 2014

Gena Makhomed commented

You are right, it was bug in my code, sorry for inconvenience.

Feature request: add to MockMvc ability to detect such bugs in user code during unit-testing with unambiguous diagnostics. It will be very useful for finding and detecting such bugs in user code. Finding bugs in user code is the main purpose of JUnit and MockMvc, as I understand.

Extend ResponseEntity not help, because can work only with fixed list of HTTP Status Codes. This limitation of ResponseEntity is one and only one reason, why I need create own filter for setting non-standard HTTP Status Codes with Spring MVC Controllers.

Simplest solution is to have ResponseEntity constructors not only with enum HttpStatus argument, but also with int argument. But in current version of Spring this is impossible even by extending (only one) ResponseEntity class, internally enum HttpStatus used in Spring Framework almost on all levels, the lowest level is ServletServerHttpResponse:

@Override
public void setStatusCode(HttpStatus status) {
        this.servletResponse.setStatus(status.value());
}

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 22, 2014

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 22, 2014

Rossen Stoyanchev commented

Regarding diagnostics in MockMvc, have you tried adding an .andDo(print()) for example right before all the .andExpect chained calls? It prints all the information available about the request, response, what handler was selected, model contents, exception raised and handled, etc. Is there anything we can add there that would help more?

Regarding ResponseEntity, there is actually now (in master and therefore 4.1) a proper solution for this. Take a look at #15486.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 23, 2014

Gena Makhomed commented

.andDo(print()) is debug print only, this approach can't help automatically detect bugs in user code during unit testing. I am talking about IllegalStateException in case if response buffer already flushed and changing response headers or response status code not possible. This is bug in user code, and such bug can automatically be detected by JUnit+MockMvc.

#15486 ResponseBodyInterceptor does not help, because Servlet API does not contain methods for removing response headers, so if I add header X-HTTP-Status-Override to response - this header also will be send to client. HttpStatusOverrideFilter do all what I need without garbage in server reply and without necessity to parse/modify response body, so HttpStatusOverrideFilter still is the best possible solution.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 23, 2014

Rossen Stoyanchev commented

I think ResponseBodyInterceptor would help actually. Your input their is not the HttpServletResponse but a wrapper called ServletServerHttpRequest which contains HttpHeaders. Those headers are not written to the underlying HttpServletResponse until you start writing to the body or call flush. So you can still remove headers and change the response status.

As for the ISE on attempts to write to the response. It's something we may be able to enable as an option. I.e. you would have to explicitly choose to enter in this mode.

Loading

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 23, 2014

Rossen Stoyanchev commented

So you can still remove headers and change the response status.

I just confirmed that works as expected.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants