MockHttpServletRequest should not require setContent for non-null getInputStream() result [SPR-11764]

[spring-projects-issues]

Gena Makhomed opened SPR-11764 and commented

Yet another filter, and unit test failed with exception

java.lang.IllegalArgumentException: No InputStream specified
	at org.springframework.util.Assert.notNull(Assert.java:112)
	at org.springframework.util.StreamUtils.copy(StreamUtils.java:118)
	at org.springframework.util.StreamUtils.copyToByteArray(StreamUtils.java:56)
	at com.pb.ivrcgate.util.filter.HttpRequestWrapper.<init>(HttpRequestWrapper.java:27)
	at com.pb.ivrcgate.util.filter.HttpDumperFilter.doFilterInternal(HttpDumperFilter.java:25)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
	at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:137)
	at org.springframework.test.web.servlet.MockMvc.perform(MockMvc.java:141)
	at com.pb.ivrcgate.controller.GateControllerTest.badUri(GateControllerTest.java:40)

Filter code fragment:

public class HttpDumperFilter extends OncePerRequestFilter {

    @Override
    protected void doFilterInternal(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse, FilterChain filterChain) throws ServletException, IOException {
        try {
            HttpRequestWrapper request = new HttpRequestWrapper(httpServletRequest);
            HttpResponseWrapper response = new HttpResponseWrapper(httpServletResponse);
            filterChain.doFilter(request, response);
            // ...
        } catch (Exception ex) {
            logger.error("", ex);
            throw ex;
        }
    }
    // ...
}

HttpRequestWrapper code fragment:

public class HttpRequestWrapper extends HttpServletRequestWrapper {

    private final byte[] content;

    public HttpRequestWrapper(HttpServletRequest request) throws IOException {
        super(request);
        this.content = StreamUtils.copyToByteArray(request.getInputStream()); // line #27
    }
    // ...
}

In compliance with Servlet API and documentation - MockMvc must return object of class ServletInputStream, and can't return null.

For example, tomcat return ServletInputStream object even for GET requests, and never return null from getInputStream() method.

Looks like this is bug in MockMvc.

---

Affects: 4.0.4

Issue Links:

• MockMvc ignores HTTP status code overridden by filter [SPR-11760] #16382 MockMvc ignores HTTP status code overridden by filter
• MockHttpServletRequest.getReader() returns null in case of no content [SPR-15215] #19780 MockHttpServletRequest.getReader() returns null in case of no content

[spring-projects-issues]

Sam Brannen commented

This is potentially related to #16382.

[spring-projects-issues]

Rossen Stoyanchev commented

The Servlet API documentation doesn't seem very clear on that. The Javadoc has no mention on what should be returned if there is no body (see here).

This is a long standing intentional behavior of MockHttpServletRequest so it's certainly not a bug. It is also quite reasonable IMO to return null if there is no body. You will need to update you wrapper to guard against null.

[spring-projects-issues]

Gena Makhomed commented

See javadoc for other methods, for example, ServletRequest.getCharacterEncoding()
if method can return null - this unambiguously documented:

Returns:

a String containing the name of the character encoding, or null if the request does not specify a character encoding

Documentation of getInputStream() very clean about return value:

Returns:

a ServletInputStream object containing the body of the request

So, getInputStream() method not allowed to return null in any case, even in case of one-byte or zero-byte request body.

This is a long standing intentional behavior of MockHttpServletRequest so it's certainly not a bug.

Incompatibility with Servlet api - is bug, in any case.

It is also quite reasonable IMO to return null if there is no body.

No reason at all. Servlet container does not read request body before passing it to servlet,
so servlet container does not know - zero size request, one byte size or one terabyte size.
any Servlet container just create ServletInputStream object and return it to servlet, as described in API.

and Servlet API not allow to return null instead ServletInputStream object -
in any case ServletInputStream object must be returned.

And you can see javadoc for any other method: ServletRequest
if null allowed as return value, such return value unambiguously documented in Servlet API.

P.S.

Do you know about SpecialCase pattern from Martin Fowler book? This pattern already used in many places in Spring Framework and Java EE API. Other name of this pattern is Null Object pattern.

[spring-projects-issues]

Rossen Stoyanchev commented

Alright personally I prefer to see it explicitly mentioned either way but you have a point about how null return value is documented in the Servlet API.

I am re-opening to consider a fix.

[spring-projects-issues]

Juergen Hoeller commented

There is a subtle additional notion: We're returning -1 from getContentLength() if no content has been set, indicating "unknown length" (as per the Servlet API javadoc). We're only doing that if no content has been set (the default). Preserving that, we can't simply use an empty byte array as default content, which would otherwise be a worthwhile option. Instead, we'd have to locally return an empty ServletInputStream in the no-content case.

Note that simply calling setContent(new byte[0]) on a custom MockHttpServletRequest instance also solves the problem, since that will lead to an empty ServletInputStream getting returned... Arguably, the current impl simply requires a setContent call to happen (even if just with empty content) if you'd like to access an InputStream or Reader - and what's being asked for here is easier setup for the no-content case, with no need to call setContent.

Juergen

[spring-projects-issues]

Rossen Stoyanchev commented

Note that simply calling setContent(new byte[0]) on a custom MockHttpServletRequest instance also solves the problem

This is something we can do in MockHttpServletRequestBuilder so it becomes the default for MockMvc.

That said returning an empty ServletInputStream seems a safe option as well. It is unlikely that existing tests depend on getInputStream returning null (asserts on the request would be unusual).

[spring-projects-issues]

Rossen Stoyanchev commented

Commit c269d2.