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.

Sign up for GitHub

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

Content Length Missing Fix #1088

Merged
merged 16 commits into from
Feb 29, 2024
Merged

Content Length Missing Fix #1088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0c608ee
Fix File Upload by setting ContentLength property
ramsessanchez Feb 16, 2024
2dd6776
Merge branch 'main' into rsh/contentLengthUploadFix
baywet Feb 21, 2024
13ae404
Refactor to utilize okhttp class RequestBody.create() rather than imp…
ramsessanchez Feb 21, 2024
72af825
spotless apply
ramsessanchez Feb 21, 2024
dd05912
agent handler version
ramsessanchez Feb 21, 2024
c8c3b66
refactor logic
ramsessanchez Feb 22, 2024
b711049
Refactor contentLength() method
ramsessanchez Feb 22, 2024
d5a22ed
spotless apply
ramsessanchez Feb 22, 2024
3894fcf
Update components/http/okHttp/src/main/java/com/microsoft/kiota/http/…
ramsessanchez Feb 23, 2024
06c3e31
Merge branch 'main' into rsh/contentLengthUploadFix
baywet Feb 26, 2024
7ba31e1
- bumps patch version
baywet Feb 26, 2024
b243088
json and file payload tests
ramsessanchez Feb 28, 2024
c350ac1
merge
ramsessanchez Feb 28, 2024
6c621fa
v1.0.5
ramsessanchez Feb 28, 2024
8f0b05e
fix failing spotbugs tests
ramsessanchez Feb 28, 2024
826ad00
fix file path
ramsessanchez Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

## [1.0.5] - 2023-02-28

### Changed

- Added contentLength property to RequestInformation to facilitate in setting the content length of the Okhttp3 RequestBody object within the OkhttpRequestAdapter.

## [1.0.4] - 2024-02-26

### Changed
Expand Down
57 changes: 36 additions & 21 deletions components/http/okHttp/src/main/java/com/microsoft/kiota/http/OkHttpRequestAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import okio.BufferedSink;
import okio.Okio;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.math.BigDecimal;
Expand All @@ -53,6 +54,7 @@
/** RequestAdapter implementation for OkHttp */
public class OkHttpRequestAdapter implements com.microsoft.kiota.RequestAdapter {
private static final String contentTypeHeaderKey = "Content-Type";
private static final String contentLengthHeaderKey = "Content-Length";
@Nonnull private final Call.Factory client;
@Nonnull private final AuthenticationProvider authProvider;
@Nonnull private final ObservabilityOptions obsOptions;
Expand Down Expand Up @@ -188,7 +190,7 @@ public void enableBackingStore(@Nullable final BackingStoreFactory backingStoreF
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
Objects.requireNonNull(factory, nullFactoryParameter);

final Span span = startSpan(requestInfo, "sendCollectionAsync");
final Span span = startSpan(requestInfo, "sendCollection");
try (final Scope scope = span.makeCurrent()) {
Response response = this.getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -269,7 +271,7 @@ private Span startSpan(
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
Objects.requireNonNull(factory, nullFactoryParameter);

final Span span = startSpan(requestInfo, "sendAsync");
final Span span = startSpan(requestInfo, "send");
try (final Scope scope = span.makeCurrent()) {
Response response = this.getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -331,7 +333,7 @@ private void closeResponse(boolean closeResponse, Response response) {
@Nonnull final Class<ModelType> targetClass) {
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
Objects.requireNonNull(targetClass, "parameter targetClass cannot be null");
final Span span = startSpan(requestInfo, "sendPrimitiveAsync");
final Span span = startSpan(requestInfo, "sendPrimitive");
try (final Scope scope = span.makeCurrent()) {
Response response = this.getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -425,7 +427,7 @@ private void closeResponse(boolean closeResponse, Response response) {
@Nonnull final ValuedEnumParser<ModelType> enumParser) {
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
Objects.requireNonNull(enumParser, nullEnumParserParameter);
final Span span = startSpan(requestInfo, "sendEnumAsync");
final Span span = startSpan(requestInfo, "sendEnum");
try (final Scope scope = span.makeCurrent()) {
Response response = this.getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -471,7 +473,7 @@ private void closeResponse(boolean closeResponse, Response response) {
@Nonnull final ValuedEnumParser<ModelType> enumParser) {
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
Objects.requireNonNull(enumParser, nullEnumParserParameter);
final Span span = startSpan(requestInfo, "sendEnumCollectionAsync");
final Span span = startSpan(requestInfo, "sendEnumCollection");
try (final Scope scope = span.makeCurrent()) {
Response response = this.getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -518,7 +520,7 @@ private void closeResponse(boolean closeResponse, Response response) {
@Nonnull final Class<ModelType> targetClass) {
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);

final Span span = startSpan(requestInfo, "sendPrimitiveCollectionAsync");
final Span span = startSpan(requestInfo, "sendPrimitiveCollection");
try (final Scope scope = span.makeCurrent()) {
Response response = getHttpResponseMessage(requestInfo, span, span, null);
final ResponseHandler responseHandler = getResponseHandler(requestInfo);
Expand Down Expand Up @@ -835,7 +837,7 @@ private void setBaseUrlForRequestInformation(@Nonnull final RequestInformation r
@SuppressWarnings("unchecked")
@Nonnull public <T> T convertToNativeRequest(@Nonnull final RequestInformation requestInfo) {
Objects.requireNonNull(requestInfo, nullRequestInfoParameter);
final Span span = startSpan(requestInfo, "convertToNativeRequestAsync");
final Span span = startSpan(requestInfo, "convertToNativeRequest");
try (final Scope scope = span.makeCurrent()) {
this.authProvider.authenticateRequest(requestInfo, null);
return (T) getRequestFromRequestInformation(requestInfo, span, span);
Expand Down Expand Up @@ -885,9 +887,8 @@ private void setBaseUrlForRequestInformation(@Nonnull final RequestInformation r
@Override
public MediaType contentType() {
final Set<String> contentTypes =
requestInfo.headers.containsKey(contentTypeHeaderKey)
? requestInfo.headers.get(contentTypeHeaderKey)
: new HashSet<>();
requestInfo.headers.getOrDefault(
contentTypeHeaderKey, new HashSet<>());
if (contentTypes.isEmpty()) {
return null;
} else {
Expand All @@ -899,6 +900,30 @@ public MediaType contentType() {
}
}

@Override
public long contentLength() {
long length;
final Set<String> contentLength =
requestInfo.headers.getOrDefault(
contentLengthHeaderKey, new HashSet<>());
if (contentLength.isEmpty()
&& requestInfo.content
instanceof ByteArrayInputStream) {
final ByteArrayInputStream contentStream =
(ByteArrayInputStream) requestInfo.content;
length = contentStream.available();
Copy link
Contributor

@andreaTP andreaTP Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so sorry I have not followed up on this subject, but, as noticed here: #1088 (comment)

available is NOT the API you need to use in this context since it has very very different semantics from the expected behavior.
Have you explored different solutions?
Do we have a reproducer of the original issue that triggered those changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, due to the support and semantics, I'm using workarounds in the Apicurio Registry testsuite to properly perform file uploads.
But I'm looking forward to a real resolution of the issue.

If you need me to chip in I would love a minimal repoducer, maybe @araneolus would be able to provide one 🙏

} else {
length =
Long.parseLong(
contentLength.toArray(new String[] {})[0]);
}
if (length > 0) {
spanForAttributes.setAttribute(
SemanticAttributes.HTTP_REQUEST_BODY_SIZE, length);
}
return length;
}

@Override
public void writeTo(@Nonnull BufferedSink sink) throws IOException {
sink.writeAll(Okio.source(requestInfo.content));
Expand Down Expand Up @@ -933,17 +958,7 @@ public void writeTo(@Nonnull BufferedSink sink) throws IOException {
requestBuilder.tag(obsOptions.getType(), obsOptions);
}
requestBuilder.tag(Span.class, parentSpan);
final Request request = requestBuilder.build();
final List<String> contentLengthHeader = request.headers().values("Content-Length");
if (contentLengthHeader != null && !contentLengthHeader.isEmpty()) {
final String firstEntryValue = contentLengthHeader.get(0);
if (firstEntryValue != null && !firstEntryValue.isEmpty()) {
spanForAttributes.setAttribute(
SemanticAttributes.HTTP_REQUEST_BODY_SIZE,
Long.parseLong(firstEntryValue));
}
}
return request;
return requestBuilder.build();
} finally {
span.end();
}
Expand Down
2 changes: 1 addition & 1 deletion ...ttp/src/main/java/com/microsoft/kiota/http/middleware/options/UserAgentHandlerOption.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public UserAgentHandlerOption() {}

private boolean enabled = true;
@Nonnull private String productName = "kiota-java";
@Nonnull private String productVersion = "1.0.0";
@Nonnull private String productVersion = "1.0.5";

/**
* Gets the product name to be used in the user agent header
Expand Down
59 changes: 56 additions & 3 deletions components/http/okHttp/src/test/java/com/microsoft/kiota/http/OkHttpRequestAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,15 @@

import okio.Okio;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.stubbing.Answer;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.*;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
Expand Down Expand Up @@ -293,6 +292,60 @@ void throwsAPIException(
assertTrue(exception.getResponseHeaders().containsKey("request-id"));
}

@Test
void getRequestFromRequestInformationHasCorrectContentLength_JsonPayload() throws Exception {
final var authenticationProviderMock = mock(AuthenticationProvider.class);
final var requestInformation = new RequestInformation();
requestInformation.setUri(new URI("https://localhost"));
ByteArrayInputStream content =
new ByteArrayInputStream(
"{\"name\":\"value\",\"array\":[\"1\",\"2\",\"3\"]}"
.getBytes(StandardCharsets.UTF_8));
requestInformation.setStreamContent(content, "application/octet-stream");
requestInformation.httpMethod = HttpMethod.PUT;
requestInformation.headers.tryAdd("Content-Length", String.valueOf(content.available()));

final var adapter = new OkHttpRequestAdapter(authenticationProviderMock);
final var request =
adapter.getRequestFromRequestInformation(
requestInformation, mock(Span.class), mock(Span.class));

assertEquals(
String.valueOf(requestInformation.content.available()),
request.headers().get("Content-Length"));
assertEquals("application/octet-stream", request.headers().get("Content-Type"));
assertNotNull(request.body());
assertEquals(request.body().contentLength(), requestInformation.content.available());
assertEquals(request.body().contentType(), MediaType.parse("application/octet-stream"));
}

@Test
void getRequestFromRequestInformationIncludesContentLength_FilePayload() throws Exception {
final var authenticationProviderMock = mock(AuthenticationProvider.class);
final var testFile = new File("./src/test/resources/helloWorld.txt");
final var requestInformation = new RequestInformation();

requestInformation.setUri(new URI("https://localhost"));
requestInformation.httpMethod = HttpMethod.PUT;
requestInformation.headers.add("Content-Length", String.valueOf(testFile.length()));
try (FileInputStream content = new FileInputStream(testFile)) {
requestInformation.setStreamContent(content, "application/octet-stream");

final var adapter = new OkHttpRequestAdapter(authenticationProviderMock);
final var request =
adapter.getRequestFromRequestInformation(
requestInformation, mock(Span.class), mock(Span.class));

assertEquals(
String.valueOf(requestInformation.content.available()),
request.headers().get("Content-Length"));
assertEquals("application/octet-stream", request.headers().get("Content-Type"));
assertNotNull(request.body());
assertEquals(request.body().contentLength(), requestInformation.content.available());
assertEquals(request.body().contentType(), MediaType.parse("application/octet-stream"));
}
}

public static OkHttpClient getMockClient(final Response response) throws IOException {
final OkHttpClient mockClient = mock(OkHttpClient.class);
final Call remoteCall = mock(Call.class);
Expand Down
25 changes: 25 additions & 0 deletions components/http/okHttp/src/test/resources/helloWorld.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
HelloWorld!
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ org.gradle.caching=true
mavenGroupId = com.microsoft.kiota
mavenMajorVersion = 1
mavenMinorVersion = 0
mavenPatchVersion = 4
mavenPatchVersion = 5
mavenArtifactSuffix =

#These values are used to run functional tests
Expand Down
Loading