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

MockHttpServletRequest.getReader, getInputStream should each return the same object on repeat calls [SPR-16505] #21048

Closed
spring-projects-issues opened this issue Feb 15, 2018 · 9 comments
Assignees
Labels
in: test type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Feb 15, 2018

Av Pinzur opened SPR-16505 and commented

A de facto rule of thumb for the Servlet paradigm is that a ServletRequest's body content can only be read once. This implies at least a couple limitations:

  1. The content retrieval methods (getReader and getInputStream) cannot be used in combination. (See #21042.)
  2. Each content retrieval method (getReader or getInputStream) will read the body only once.

This latter limitation is the subject of this ticket. Although unlike #1, this behavior is not explicitly specified in the interface documentation, major Servlet implementations appear to concur in returning the same object reference for successive calls to either getReader or getInputStream. This means that it's effectively a bug for application code to attempt to read the body contents twice from two such separate calls.

However, the current implementation of MockHttpServletRequest constructs a fresh object each time one of these methods is called. Revising this implementation to retain the returned reader or stream reference for successive calls will better reflect real-world implementations and enhance the framework's value by allowing developers to catch one more class of bugs prior to deploying to an actual container.


Affects: 4.3.14, 5.0.3

Issue Links:

  • #21042 MockHttpServletRequest shouldn't allow calls to both getReader and getInputStream
  • #21906 MockHttpServletRequest doesn't reset InputStream/Reader on setContent

Referenced from: pull request #1689, and commits 3fc8ec4

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 13, 2018

Juergen Hoeller commented

Addressed in a revised fashion along with #21042, also covering our internal copy of MockHttpServletRequest and refactoring several of our tests which tried to reuse a request instance for several tests.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 14, 2018

Andy Wilkinson commented

As currently implemented, this change breaks Spring REST Docs. It will, I believe, also break any other tests that rely on reading the body of a request obtained from MvcResult.getRequest().

To document a request body, REST Docs relies on being able to read the body of the request obtained from the MvcResult. The change made for this issue prevents that as, typically, the body has already been read as part of the request being handled.

I wonder if the improvement being made for this issue could be retained while also allowing the body of a request that's obtained from MvcResult.getRequest() to be read multiple times. This would better reflect real-world implementations for request handling code while also allowing tests to read the body as many times as necessary to make their assertions.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 14, 2018

Rossen Stoyanchev commented

The InputStream or Reader are just wrappers around the actual byte[] content that mock request was configured with. To access that you can use still getContentAsByteArray() and getContentAsString() as many times as necessary. That is what the MockMvc result matchers do. Does Spring REST Docs need to use getInputStream() for some reason?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 14, 2018

Andy Wilkinson commented

I suspect that REST Docs is using getInputStream() simply because getContentAsByteArray() didn't exist when the code was written. I see no reason why REST Docs can't move to it. All it's doing at the moment is reading the input stream into a byte array anyway. Thanks for the pointer.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 14, 2018

Rossen Stoyanchev commented

You're right, it looks like we only had them on the response side until recently, #19282.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 11, 2018

Graeme Rocher commented

This is a massive breaking change IMO. Previous versions of MockHttpServletRequest allows setting the content of the request to a different value and calling getReader() or getInputStream again to obtain the new content.

 

IMO if this change is to be retained then the reader should at least be reset when setContent is called.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 12, 2018

Juergen Hoeller commented

Graeme Rocher, indeed, we need to reset the stream/reader on setContent calls there. I've opened #21906 to track this.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 15, 2018

Graeme Rocher commented

Thanks Juergen

@ilya40umov
Copy link

@ilya40umov ilya40umov commented Jan 21, 2019

This change is breaking Swagger Request Validator:
https://bitbucket.org/atlassian/swagger-request-validator/issues/190/spring-boot-210-cannot-call-getreader
and preventing anybody using it to migrate to Spring Boot 2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test type: enhancement
Projects
None yet
Development

No branches or pull requests

3 participants