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

HttpComponentsClientHttpRequestFactory does not set Content-Length: 0 #32678

Closed
tholinka opened this issue Apr 19, 2024 · 19 comments
Closed

HttpComponentsClientHttpRequestFactory does not set Content-Length: 0 #32678

tholinka opened this issue Apr 19, 2024 · 19 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@tholinka
Copy link

tholinka commented Apr 19, 2024

Affects: 6.1.6

I have an API that requires sending a Content-Length: 0 when the body is null/empty.

When we previously upgraded to Spring Boot 3 / Spring 6, we changed our RestTemplate to use new BufferingClientHttpRequestFactory(new HttpComponentsClientHttpRequestFactory()) as the request factory in order to ensure that a Content-Length was set.

This worked on 6.1.5, but on upgrading to 6.1.6 we are no longer sending the Content-Length header.


Stepping through the code with a debugger, this changed in 019ce44 / gh-32612.

I can force the old behavior if I do the following, which leads me to believe it is that commit (effectively, I'm trying to just skip over the new if):

  • Set a breakpoint at line 58 and at line 59 of BufferingClientHttpRequestWrapper
  • (on line 58 breakpoint) in the evaluation expression window run bufferedOutput = new byte[1];
  • Continue to line 59 breakpoint
  • (on line 59 breakpoint) in the evaluation expression window run bufferedOutput = new byte[0];

Doing this then causes the Content-Length: 0 header to be sent as expected.


I tried also manually setting Content-Length: 0 as a header, but HttpComponentsClientHttpRequest does not allow that, as is explicitly ignores Content-Length as a header.

else if (!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(headerName) &&


This still works as expected (Content-Length: 0 is set) when using SimpleClientHttpRequestFactory, so for now I can work around this by using new BufferingClientHttpRequestFactory(new SimpleClientHttpRequestFactory()) instead. However, I would prefer to use HttpComponentsClientHttpRequestFactory instead of Simple if possible.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 19, 2024
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Apr 19, 2024
@tholinka
Copy link
Author

tholinka commented Apr 19, 2024

Looking into it some more, in org.apache.hc.core5.http.protocol.RequestContent.process() there is a null check for if entity is null. With the previous version, this is empty, but with the change in the BufferingClientHttpRequestWrapper it is now null.

This is because of this check:

In 6.1.5, this check would be true when using BufferingClientHttpRequestFactory (body would be empty, but not null). However, in 6.1.6 because of 019ce44 it is now null, so the entity is null.

@bclozel
Copy link
Member

bclozel commented Apr 19, 2024

I have an API that requires sending a Content-Length: 0 when the body is null/empty.

Before investigating more we should question this requirement. The previous behavior was changed because it was considered invalid with regards to the spec. Is there anything supporting this behavior in clients in general?

@tholinka
Copy link
Author

tholinka commented Apr 19, 2024

Sadly this isn't changeable from my team. We're trying to call an external API that is very resistant to change, and they've always required sending Content-Length as a header.

e.g. this curl works:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN' --header 'Content-Length: 0'
# good response

and so does this one:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN' --data-raw ''
# good response

but this one does not:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN'
# replies with "Content-Length is missing"

So having some way to send Content-Length when it's 0 would be nice. From my understanding of the spec, it's just negative numbers that aren't valid. 0 is a valid Content-Length, I believe.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Apr 19, 2024
@tholinka
Copy link
Author

tholinka commented Apr 19, 2024

We're trying to call in our code with the following currently

HttpEntity<Object> entity = new HttpEntity<>("", headers);
ResponseEntity<Response> response = restTemplate.exchange(url, HttpMethod.POST, entity, Response.class);

I could understand not sending Content-Length: 0 if the body is null, but we're explicitly trying to send an empty body, because we do want the Content-Length header.

Also, for what it's worth, we do get a Content-Length: 0 header as expected if we use new BufferingClientHttpRequestFactory(new SimpleClientHttpRequestFactory()), or if we use new JdkClientHttpRequestFactory().

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 19, 2024
@poutsma
Copy link
Contributor

poutsma commented Apr 22, 2024

This seems related to, or perhaps even in contradiction of, #32650.

@poutsma poutsma added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided status: waiting-for-feedback We need additional information before we can continue labels Apr 22, 2024
@poutsma poutsma self-assigned this Apr 22, 2024
@tholinka
Copy link
Author

tholinka commented Apr 22, 2024

I don't think this is contradicted by #32650, for instance, in #32650 (comment) @nhmarujo lays out almost exactly the same as what I would expect. Thought that is somewhat contradicted by #32650 (comment), so maybe not...

in #32650, the conclusion seems to me (and it seems to me that it works this way with SimpleClientHttpRequestFactory and JdkClientHttpRequestFactory):

  • does NOT set header:
    • null
  • DOES set header:
    • empty (but not null, e.g. "")
    • explicitly set
    • non-empty (body has an actual value)

Whereas currently with BufferingClientHttpRequestFactory + HttpComponentsClientHttpRequestFactory, this is what currently happens from my testing:

  • does NOT set header:
    • null
    • empty (but not null, e.g. "")
    • explicitly set (this value is completely ignored, because httpclient5 throws when Content-Length is set, unless the override config value is set)
  • DOES set header:
    • non-empty (body has an actual value)

Hopefully those that worked on #32650 can comment, as maybe I'm misunderstanding something here.

@Mario-Eis
Copy link

Mario-Eis commented Apr 24, 2024

Sadly this isn't changeable from my team. We're trying to call an external API that is very resistant to change, and they've always required sending Content-Length as a header.

e.g. this curl works:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN' --header 'Content-Length: 0'
# good response

and so does this one:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN' --data-raw ''
# good response

but this one does not:

$ curl --location --request POST 'https://example.com/endpoint' --header 'Authorization: TOKEN'
# replies with "Content-Length is missing"

So having some way to send Content-Length when it's 0 would be nice. From my understanding of the spec, it's just negative numbers that aren't valid. 0 is a valid Content-Length, I believe.

Same here. The changes in the content-length headers behaviour were breaking our system several times now for the last month. Not being able to set a guaranteed content-length header would be a huge deal for us. Because our central NoSql Database expects a content-length set for requests. Even when no body is contained. With the current behaviour we are not able to update spring from now on. Or refactor the whole system to move away from using the rest template. Meaning to exchange the whole data access layer. Thats really a big deal.

@onjik
Copy link
Contributor

onjik commented Apr 24, 2024

Hi @tholinka @Mario-Eis

There is no problem with the Content-Length being 0 with Empty Body. (That doesn't mean you should set) The RFC document defines a value greater than or equal to zero as a valid value.

In my opinion, this change (#32650 , b3a4567) has the following meaning:

Do not force "Content-Length" to all requests.
Give the user a choice.

(In previous behavior, there was no way to not attach Content-Length)

//on prev code
if ( headers.getContentLength() < 0) { // check if there is no content-length header
	headers.setContentLength(bytes.length);
}

I think there is no problem with that change itself.

But if "content-length = 0" cannot be set explicitly, I think this is a problem and a point to be corrected.

I'm not sure because I didn't see the full detailed code flow, but if I read the first comment, I somewhat don't understand why these codes were written. link

I think @tholinka 's concept is correct.

does NOT set header:

  • null

DOES set header:

  • empty (but not null, e.g. "")
  • explicitly set
  • non-empty (body has an actual value)

Hi @nhmarujo!! Have any Idea??

@onjik
Copy link
Contributor

onjik commented Apr 24, 2024

I think this code seems to have been written on the assumption that the Content-Length will be recalculated and added unconditionally in this part that has been changed (#32650 , b3a4567 )

It will no longer be added unconditionally, it seems necessary to make appropriate changes.

I think there are some ways. (Again, adding the content-length header automatically to all requests doesn't seem right.)

  1. Trust (i.e. reflect all) the Content-Length headers given as Arguments
  2. If there is content-length in the header, recalculate it (which doesn't seem like a very good way to do it) (and in this case, you should also consider the transfer encoding header)

It just came to my mind immediately and I need other's opinions.

@jbretsch
Copy link

jbretsch commented Apr 24, 2024

I, too, suffer from this regression because a legacy system which is called by a service I’m helping to maintain requires Content-Length: 0 for empty POST requests.

I think, for empty POST/PUT/PATCH requests, the Content-Length header should be set to 0. This is supported by the HTTP RFC#9110. It says:

A user agent SHOULD send Content-Length in a request when the method defines a meaning for enclosed content and it is not sending Transfer-Encoding. For example, a user agent normally sends Content-Length in a POST request even when the value is 0 (indicating empty content). A user agent SHOULD NOT send a Content-Length header field when the request message does not contain content and the method semantics do not anticipate such data.

Note that, „when the method defines a meaning for enclosed content“ in the first sentence refers to e.g. POST/PUT/PATCH requests. And „the method semantics do not anticipate such data“ in the last sentence refers to e.g. GET requests.

PS: We can also dig into the history of the HTTP RFCs.

For instance, RFC#2616 said about the Content-Length header:

Note that the meaning of this field is significantly different from the corresponding definition in MIME, where it is an optional field used within the "message/external-body" content-type. In HTTP, it SHOULD be sent whenever the message's length can be determined prior to being transferred, unless this is prohibited by the rules in section 4.4.

And section 4.4 said:

For compatibility with HTTP/1.0 applications, HTTP/1.1 requests containing a message-body MUST include a valid Content-Length header field unless the server is known to be HTTP/1.1 compliant. If a request contains a message-body and a Content-Length is not given, the server SHOULD respond with 400 (bad request) if it cannot determine the length of the message, or with 411 (length required) if it wishes to insist on receiving a valid Content-Length.

@nhmarujo
Copy link

nhmarujo commented Apr 24, 2024

@jbretsch note however that what you just posted speaks in favour of #32650 , b3a4567

A user agent SHOULD NOT send a Content-Length header field when the request message does not contain content and the method semantics do not anticipate such data.

That issue was specifically about a POST without a body (not empty, but absent!). 🙂

@jbretsch
Copy link

jbretsch commented Apr 24, 2024

@jbretsch note however that what you just posted speaks in favour of #32650 , b3a4567

A user agent SHOULD NOT send a Content-Length header field when the request message does not contain content and the method semantics do not anticipate such data.

No, it does not. That is because with "the method semantics do not anticipate such data" they mean e.g. GET requests which normally have no body. POST/PUT/PATCH requests fall under "when the method defines a meaning for enclosed content". When the RFC speaks about "method", it does not speak about different resources and their specific semantics given by the implementing service. It speaks about HTTP methods, i.e. HTTP verbs.

PS: Also, from the perspective of the HTTP RFC, there is no difference between a POST request with an empty body and POST request with an absent body.

@nhmarujo
Copy link

nhmarujo commented Apr 24, 2024

So say I have 2 POSTS with no content, but one with Content-Type set and the other not. Are they the same to you?

@jbretsch
Copy link

jbretsch commented Apr 24, 2024

So say I have 2 POSTS with no content, but one with Content-Type set and the other not. Are they the same to you?

I see nothing in section 8.3 Content-Type that suggests that those two requests should have different semantics. I would also still maintain the claim that from the perspective of the HTTP RFC, there is no difference between a POST request with an empty body and a POST request with an absent body. Do you see where the HTTP RFC makes a distinction between empty and absent body?

@nhmarujo
Copy link

I think this comment on stackoverflow does shed some light with some RFC quotes https://stackoverflow.com/a/78182957 Let me know your thoughts 🙂

@bclozel bclozel self-assigned this Apr 24, 2024
@jbretsch
Copy link

I think this comment on stackoverflow does shed some light with some RFC quotes https://stackoverflow.com/a/78182957 Let me know your thoughts 🙂

The argument in https://stackoverflow.com/a/78182957 assumes that a client is free to choose whether to include the Content-Length header or not in POST requests. One can argue that this is indeed true because only SHOULD instead of MUST is used in "A user agent SHOULD send Content-Length in a request when the method defines a meaning for enclosed content and it is not sending Transfer-Encoding." even if one believes that all POST requests are meant with "the method defines a meaning for enclosed content".

I'm not convinced that it would be a good idea to actually implement a POST resource which behaves differently for absent and empty requests. But I'm fine with settling on: The HTTP RFC allows that.

The question remains how Spring should behave. I agree with #32678 (comment) which expects a Content-Length: 0 header being present for:

HttpEntity<Object> entity = new HttpEntity<>("", headers);
ResponseEntity<Response> response = restTemplate.exchange(url, HttpMethod.POST, entity, Response.class);

I think, it would be pragmatic to restore this behavior while simultaneously allowing people to differentiate between absent and empty request bodies by having Spring exclude the Content-Length header for:

HttpEntity<Object> entity = new HttpEntity<>(null, headers);
ResponseEntity<Response> response = restTemplate.exchange(url, HttpMethod.POST, entity, Response.class);

@nhmarujo
Copy link

Yes, that is the suggestion I was trying to make - to avoid the second one from having the Content-Length.

@bclozel
Copy link
Member

bclozel commented Apr 29, 2024

I had another look and tested the following code snippet with various combinations of request factories and checking the "Content-Length" header value received by the server:

ResponseEntity<String> response = restClient.post().uri("http://localhost:8080/test").retrieve().toEntity(String.class);

Spring Framework 6.1.5

We can see two inconsistencies, one with HttpComponentsClientHttpRequestFactory and another one with SimpleClientHttpRequestFactory.

Client Factory Content-Length value
HttpComponentsClientHttpRequestFactory null
BufferingClientHttpRequestFactory + HttpComponentsClientHttpRequestFactory 0
JdkClientHttpRequestFactory 0
BufferingClientHttpRequestFactory + JdkClientHttpRequestFactory 0
JettyClientHttpRequestFactory 0
BufferingClientHttpRequestFactory + JettyClientHttpRequestFactory 0
SimpleClientHttpRequestFactory null
BufferingClientHttpRequestFactory + SimpleClientHttpRequestFactory 0

Spring Framework 6.1.6

The HttpComponentsClientHttpRequestFactory inconsistency is fixed, but the SimpleClientHttpRequestFactory one remains.

Client Factory Content-Length value
HttpComponentsClientHttpRequestFactory null
BufferingClientHttpRequestFactory + HttpComponentsClientHttpRequestFactory null
JdkClientHttpRequestFactory 0
BufferingClientHttpRequestFactory + JdkClientHttpRequestFactory 0
JettyClientHttpRequestFactory 0
BufferingClientHttpRequestFactory + JettyClientHttpRequestFactory 0
SimpleClientHttpRequestFactory null
BufferingClientHttpRequestFactory + SimpleClientHttpRequestFactory 0

I have tested a patch in AbstractBufferingClientHttpRequest that fixes the last one.

Empty vs missing request body

I think the use case for BufferingClientHttpRequestFactory is to buffer the request and response body contents to allow for repeated writes. It should be as much transparent as possible for any other use case. Here, it seems that some of you are using this to work around a server behavior with missing/empty request bodies and the "Content-Length" header.

Several contracts in Spring Framework do not make that difference, for example the ClientHttpRequestInterceptor should always receive a non-null byte[] body when intercepting requests. My opinion is that we should not interfere with the libraries here, and instead the BufferingClientHttpRequestFactory should be as transparent as possible. If a particular behavior is required, then this should guide the choice of client library.

org.springframework.http.client.HttpComponentsClientHttpRequest.addHeaders will not write Content-Length headers to the actual request and will delegate that to the library itself. Maybe we should relax that in the case of the "0" value? This would allow a much more robust mechanism for applications with a custom interceptor:

class ContentLengthInterceptor implements ClientHttpRequestInterceptor {

	@Override
	public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
		if (body.length == 0) {
			request.getHeaders().setContentLength(0);
		}
		return execution.execute(request, body);
	}
}

@bclozel
Copy link
Member

bclozel commented Apr 30, 2024

We have discussed this as a team and we think that the issue might not be in the BufferingClientHttpRequestFactory, but a bug in our implementation of the HttpComponents client. In the HttpComponentsClientHttpRequest, we should probably set a NullEntity instead of skipping it entirely. This should result in setting the "Content-Length" header and align the behavior with other clients.

FYI we just reopened #32650 and we might revert the change there.

@bclozel bclozel changed the title BufferingClientHttpRequestFactory + HttpComponentsClientHttpRequestFactory does not set Content-Length: 0 in 6.1.6 HttpComponentsClientHttpRequestFactory does not set Content-Length: 0 May 2, 2024
@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 2, 2024
@bclozel bclozel added this to the 6.1.7 milestone May 2, 2024
@bclozel bclozel closed this as completed in 47c5cd2 May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

9 participants