solving for content-length request header

[baywet]

Hey @Ndiritu @calebkiage @andreaTP @ramsessanchez @araneolus @ffcdf (I'm probably missing others, feel free to mention them as well)
We're having conversations about the content-length all over the place, and I think this is not helping solving the issues for good.

Here are the different contexts where the content-length is critical from an HTTP standpoint:

• When the length is known in advance (small payloads, large file upload implementation), it MUST be set.
• When the length is NOT known in advance (streaming) it MUST NOT be set.

From there, we have a couple of additional requirements/constraints:

• OkHttp will set the content-length header automatically, once the writeTo method is called on the body, depending on the media type and the stream type from what I can tell. But the rules are unclear.
• Request information instances need to be able to override the Content-Length header (large file upload implementation mostly)
• The content length value needs to be passed to the span attribute for tracing when it's available.

@andreaTP rightfully pointed out that we should not be using the available method since implementations behaviour vary. I think that part of the confusion here is that streams in dotnet behave a bit differently w.r.t. that aspect.

kiota-java/components/http/okHttp/src/main/java/com/microsoft/kiota/http/OkHttpRequestAdapter.java

Line 914 in 41bafdb

length = contentStream.available();

So one first step we probably need to take is to remove that if block body. (and yes, I know I'm the one who made that suggestion, maybe I should've taken more sick days)

I already fixed a regression to restore the default okhttp behaviour when the content length header is not overridden by the request in #1109 (maybe the code here needs to be updated to be a strict rather than lower or equal comparison, and set the initial value to -1)

This block most likely needs to be moved back to after the body is built because we won't get the native writeTo behaviour otherwise.

kiota-java/components/http/okHttp/src/main/java/com/microsoft/kiota/http/OkHttpRequestAdapter.java

Line 923 in 41bafdb

if (length > 0) {

(but it needs to be tested thoroughly)

Finally, before merging any PRs further, and adding to the confusion, thorough integration end to end testing needs to be done for both large file upload and regular post with structured bodies as I pointed out. And to avoid any false negative/positive, it needs to be done in a way that all dependencies (kiota, graph core, graph) are locally linked all the way to the test project.

@araneolus nicely provided test code for ODSP LFU here, we need the same for Exchange LFU since the service-side behaviour differs a little from what I can remember.

Between medical down-time for the last couple of weeks, and upcoming travel for the next couple of weeks, I can't be the one handling this. I'm volunteering @Ndiritu , but of course we need to consider other priorities internally first.

I hope this recap was helpful in clarifying the confusion. Thank you everyone for the help, input, and feedback! Please call me out if any of my analysis points are incorrect, this is the starting point of a discussion to figure out what the required changes are.

[baywet]

And now thanks to @dvag-joerg-winter we also have a repro for exchange

[ramsessanchez]

microsoftgraph/msgraph-sdk-java#1818 (comment)

As described here, the upload is succeeding, but the handling within the UploadResponseHandler is throwing an exception. I tested this with kiota-java-okhttp 1.0.6, @baywet I believe this includes your regression work. It is not that the headers are missing, in this particular case it is that we are expecting a response.Body object with values, however, the api is actually not returning a body object within the response itself, it is only returning the response code. We are currently attempting to read the values in an empty object which is leading to an exception.

The upload is successful, our handling of the response is flawed at the moment. This is an improvement from prior issues where the upload was always returning a 400 error due to missing content-length values in the request.Body() object

[ramsessanchez]

try (final ResponseBody body = response.body())
This line, L54, on graph-core UploadResponseHandler, should likely be changed. According to the documentation,
"Returns a non-null value if this response was passed to Callback.onResponse or returned from Call.execute(). "
We use Call.Execute when making these calls so this try statement will always pass and leads to the false assumption that the responseBody is not null when it very well might be. This should likely be changed. There is a property called NetworkResponse which would return the raw response made by the network and has null on the body object if there is no body, however documentation suggests we don't read the body on the network response so that's a bit of an issue. Perhaps we should check the response code before doing any prior handling and then decide what we do regarding empty response bodies.

[baywet]

Thank you for your analysis.
You could check for a content length and content type header in the response before opening the body.
I think that's what we do for the regular request adapter.