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

InputStream consumed by DispatcherServlet logRequest #24176

Closed
martinvw opened this issue Dec 10, 2019 · 9 comments
Closed

InputStream consumed by DispatcherServlet logRequest #24176

martinvw opened this issue Dec 10, 2019 · 9 comments
Assignees
Labels
status: invalid An issue that we don't feel is valid

Comments

@martinvw
Copy link

Affects: 5.2.1


A few weeks ago I enabled debug logging for my application, today I had forgotten about but noticed that my injected InputStream was empty. After a debugging session (first assuming it again was related to the HiddenHttpMethodFilter) I ended up at

private void logRequest(HttpServletRequest request) {
LogFormatUtils.traceDebug(logger, traceOn -> {
String params;
if (isEnableLoggingRequestDetails()) {
params = request.getParameterMap().entrySet().stream()
.map(entry -> entry.getKey() + ":" + Arrays.toString(entry.getValue()))
.collect(Collectors.joining(", "));
}
else {
params = (request.getParameterMap().isEmpty() ? "" : "masked");
}

So every time I enable debug logging I cannot use my injected InputStream:

@PostMapping
public ResponseEntity<String> doPost(InputStream input, HttpServletRequest request) {
    ...

I do now get this logging:

2019-12-10T10:27:46,305 [http-nio-8082-exec-7] DEBUG org.springframework.web.servlet.DispatcherServlet - POST "/communication", parameters={masked}

But maybe it would make sense to auto-wrap the request in DEBUG when that logging is active or give a warning when injecting the consumed InputStream later and even report who did it 😄

Thanks!

Related to (but not covered by): #22985

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 10, 2019
@martinvw
Copy link
Author

Or maybe its completely related it seems to be a application/x-www-form-urlencoded request as well here, so kind of by design

@rstoyanchev
Copy link
Contributor

Yes this is expected, and besides the logging of request details, any other code could cause a similar issue by accessing a request parameter.

Note that you can inject the content reliably as @RequestBody String or @RequestBody ByteArrayResource because we reconstruct the body from request parameters in ServletServerHttpRequest#getBody.

@martinvw
Copy link
Author

Great, I will do some tests with the @RequestBody

@rstoyanchev rstoyanchev added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 10, 2019
@martinvw
Copy link
Author

martinvw commented Dec 10, 2019

A major drawback is that even the ByteArrayResource contains an interpreted value (in this case url-encoded which make sense but brakes our code), I will have to convince my callers to pass the proper content-type headers :-)

@rstoyanchev rstoyanchev self-assigned this Dec 10, 2019
@rstoyanchev
Copy link
Contributor

The InputStream should also give you encoded form data. Perhaps you can provide some sample input to explain? Also can you clarify what you mean by the proper content-type header?

@carlsagan21
Copy link

carlsagan21 commented Dec 25, 2019

So may I interpret this thread as follows?: Receiving stream via HTTP request body is not what Spring is designed to support..? Because by any reason, reading stream can happen before the request body reaches to controller.

@rstoyanchev
Copy link
Contributor

This issue concerns the use of one of the getParameter methods on HttpServletRequest which indirectly consume the body of the request body. That's related to form data only, not streaming, and it is how the Servlet API works, with or without Spring.

@brecht-yperman-tb
Copy link

brecht-yperman-tb commented Jan 17, 2020

This does cause the CommonsMultipartResolver to no longer work (if logging on DispatcherServlet is enabled), resulting in an error MissingServletRequestPartException: Required request part 'file' is not present

Should we no longer use CommonsMultipartResolver?

@rstoyanchev
Copy link
Contributor

Yes, just use the standard multipart support in the Servlet container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

5 participants