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

org.springframework.web.client.MessageBodyClientHttpResponseWrapper.hasMessageBody() works improperly in some conditions [SPR-13414] #17993

Closed
spring-projects-issues opened this issue Sep 1, 2015 · 5 comments
Assignees
Labels
in: web status: declined

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 1, 2015

Alexey Ukhov opened SPR-13414 and commented

Function org.springframework.http.HttpHeaders.getContentLength() returns -1 if header Content-Length does not exist in response.

And in case if we have no body and no header Content-Length function returns true anyway.

In attached file is patch for this problem.

Also I've created pull request:
#868


Affects: 4.1.7

Attachments:

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 2, 2015

Brian Clozel commented

Hi Alexey Ukhov,

After taking a look at it and running the full test suite, it seems this patch is breaking other parts of the framework.

Some tests in HttpMessageConverterExtractorTests fail; in fact, in HttpMessageConverterExtractor, we can read the following:

if(!responseWrapper.hasMessageBody() || responseWrapper.hasEmptyMessageBody()) {
  return null;
}

He's the rationale behind the response wrapper implementation:

  • a response has no body if its HTTP status or its Content-Length say so
  • otherwise the response has a body (that can be empty!), see hasEmptyMessageBody

Could you elaborate a bit more on your use case and why you think this behavior should be fixed?

Thanks for this report and PR!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 2, 2015

Alexey Ukhov commented

Ok, I'll perform full testing and inform you later.

My idea is - functions like hasMessageBody() should correctly inform in body REALLY exists. Correct?
Then have a looks here - class HttpMessageConverterExtractor:

protected boolean hasMessageBody(ClientHttpResponse response) throws IOException {
     HttpStatus responseStatus = response.getStatusCode();
     if (responseStatus == HttpStatus.NO_CONTENT ||
               responseStatus == HttpStatus.NOT_MODIFIED) {
          return false;
     }
     long contentLength = response.getHeaders().getContentLength(); // problem here!!!
     return contentLength != 0;
}

see my comment // problem here.

In case in we have no body and no content-length header in code -> long contentLength = -1. And result of this function is true. This is wrong.

Now class MessageBodyClientHttpResponseWrapper:

public boolean hasMessageBody() throws IOException {
     HttpStatus responseStatus = this.getStatusCode();
     if (responseStatus.is1xxInformational() || responseStatus == HttpStatus.NO_CONTENT ||
               responseStatus == HttpStatus.NOT_MODIFIED) {
          return false;
     }
     else if(this.getHeaders().getContentLength() == 0) { // problem here!!!
          return false;
     }
     return true;
}

Again same problem, this.getHeaders().getContentLength() returns -1 and we decide that body exists.
This is wrong.

Please correct my if I wrong.

Alexey

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 2, 2015

Brian Clozel commented

The Content-Length HTTP response header is optional; having none does not mean the body does not exist.
Using return this.getHeaders().getContentLength() > -1 would break all responses with no Content-Length header and a chunked Transfer-Encoding.

See rfc7230 section-3.3.3 on how the message body length is resolved. TL;DR; reading the body itself is the only way to know if there's a body, hence the need for this ResponseWrapper class.

@spring-projects-issues
Copy link
Collaborator Author

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

Juergen Hoeller commented

Reopened for considertion of a recent GitHub comment:
#868 (comment)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 3, 2018

Brian Clozel commented

Closing due to comment.

@spring-projects-issues spring-projects-issues added type: bug status: declined in: web labels Jan 11, 2019
@spring-projects-issues spring-projects-issues removed the type: bug label Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web status: declined
Projects
None yet
Development

No branches or pull requests

2 participants