Merge pull request #1548 from microsoftgraph/fix/lfu

Fix large file upload

Author: baywet

CHANGELOG.md

@@ -11,6 +11,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+## [3.1.8]
+
+### Added
+
+### Changed
+- Changed chunkInputStream method in LargeFileUploadTask to resolve IndexOutOfBoundsException when uploading large files
+- Fix Large File Upload bug where exception was thrown for completed successful uploads
+
 ## [3.1.7] - 2024-03-28
 
 ### Changed
@@ -60,13 +68,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - Version bump for Java SDK GA release.
-- Bumps Kiota-Java abstractions, authentication, http, and serialization components for Java SDK 6.1.0 release. 
+- Bumps Kiota-Java abstractions, authentication, http, and serialization components for Java SDK 6.1.0 release.
 
 ## [3.0.12] - 2023-12-15
 
-### Fixed 
+### Fixed
 
-- Fixes a bug where a null collection for allowedHosts would result in failure to initialize client. [#1411](https://github.com/microsoftgraph/msgraph-sdk-java-core/pull/1411) 
+- Fixes a bug where a null collection for allowedHosts would result in failure to initialize client. [#1411](https://github.com/microsoftgraph/msgraph-sdk-java-core/pull/1411)
 
 ## [3.0.11] - 2023-12-08
 
@@ -77,7 +85,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [3.0.10] - 2023-11-27
 
-### Changed 
+### Changed
 
 - Removed the usage of reflection for enum deserialization and reordered RequestAdapter method arguments. [Kiota-Java #840](https://github.com/microsoft/kiota-java/issues/840)
 
@@ -100,13 +108,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Removed
 
-- Removes 'SuppressFBWarnings' annotations and dependency.  
+- Removes 'SuppressFBWarnings' annotations and dependency.
 
 ## [3.0.7] - 2023-07-20
 
 ### Added
 
-- Adds graph-java-sdk implementation of the `UrlReplaceHandler` middleware including default replacement pairs. 
+- Adds graph-java-sdk implementation of the `UrlReplaceHandler` middleware including default replacement pairs.
 - Default replacement pair: '/users/TokenToReplace' -> '/me'
 
 ## [3.0.6] - 2023-07-11
@@ -123,11 +131,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [3.0.4] - 2023-05-03
 
-### Added 
- 
-- Added LargeFileUploadTask functionality for kiota generated service libraries. 
+### Added
+
+- Added LargeFileUploadTask functionality for kiota generated service libraries.
 
-### Fixed 
+### Fixed
 
 - Fixes formatting used in the headers added by the telemetry handler to align with the [msGraph sdk spec.](https://github.com/microsoftgraph/msgraph-sdk-design/blob/master/middleware/TelemetryHandler.md)
 
@@ -171,7 +179,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   private OkHttpClient createClient(@Nonnull final IAuthenticationProvider auth) {
     return HttpClients.createDefault(auth);
   }
-  
+
   // then create the GraphServiceClient
     IAuthenticationProvider authenticationProvider = ...;
     GraphServiceClient
@@ -183,16 +191,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [2.0.20] - 2023-10-23
 
-### Changed 
+### Changed
 
-- Updates Okhttp3 to avoid transient vulnerabilty. [#1038](https://github.com/microsoftgraph/msgraph-sdk-java-core/issues/1038) 
+- Updates Okhttp3 to avoid transient vulnerabilty. [#1038](https://github.com/microsoftgraph/msgraph-sdk-java-core/issues/1038)
 
 ## [2.0.19] - 2023-06-20
 
 ### Changed
 
 - Remove explicit logging of GraphServiceException in the CoreHttpProvider class. [#885](https://github.com/microsoftgraph/msgraph-sdk-java-core/issues/885)
-- Thank you to @MaHa6543 for the contribution. 
+- Thank you to @MaHa6543 for the contribution.
 
 ## [2.0.18] - 2023-04-06
 

gradle.properties

@@ -25,7 +25,7 @@ mavenGroupId         = com.microsoft.graph
 mavenArtifactId      = microsoft-graph-core
 mavenMajorVersion    = 3
 mavenMinorVersion    = 1
-mavenPatchVersion    = 7
+mavenPatchVersion    = 8
 mavenArtifactSuffix =
 
 #These values are used to run functional tests

gradle/dependencies.gradle

@@ -7,7 +7,6 @@ dependencies {
     testImplementation 'io.opentelemetry:opentelemetry-api:1.37.0'
     testImplementation 'io.opentelemetry:opentelemetry-context:1.37.0'
     testImplementation 'io.github.std-uritemplate:std-uritemplate:0.0.57'
-    
     implementation 'com.google.code.gson:gson:2.10.1'
 
     implementation 'jakarta.annotation:jakarta.annotation-api:3.0.0'

src/main/java/com/microsoft/graph/core/CoreConstants.java

@@ -14,7 +14,7 @@ private CoreConstants() {}
     private static class VersionValues {
         private static final int MAJOR = 3;
         private static final int MINOR = 1;
-        private static final int PATCH = 7;
+        private static final int PATCH = 8;
     }
 
     /**

src/main/java/com/microsoft/graph/core/requests/upload/UploadResponseHandler.java

@@ -52,36 +52,46 @@ public <T extends Parsable> UploadResult<T> handleResponse(@Nonnull final Respon
         Objects.requireNonNull(response);
         Objects.requireNonNull(factory);
         try (final ResponseBody body = response.body()) {
-            if (Objects.isNull(body)) {
+            UploadResult<T> uploadResult = new UploadResult<>();
+            String contentLengthHeader = response.headers().get("content-length");
+            // rely on content-type OR content-length headers to determine if response body is empty.
+            // Response body() may be non-null despite being empty in raw response https://square.github.io/okhttp/3.x/okhttp/okhttp3/Response.html#body--
+            // content-length header is not always present in Graph responses. Content-type is more reliable
+            if (Objects.isNull(body)
+                || Objects.isNull(body.contentType())
+                || (!Objects.isNull(contentLengthHeader) && Integer.parseInt(contentLengthHeader) == 0)
+            ) {
+                if (response.code() == HttpURLConnection.HTTP_CREATED) {
+                    final String location = response.headers().get("location");
+                    if(!Objects.isNull(location) && !location.isEmpty()) {
+                        uploadResult.location = new URI(location);
+                        return uploadResult;
+                    }
+                }
                 throw new ApiException(ErrorConstants.Messages.NO_RESPONSE_FOR_UPLOAD);
             }
             try(final InputStream in = body.byteStream()){
-                final String contentType = body.contentType().toString().split(";")[0]; //contentType.toString() returns in format <mediaType>;<charset>, we only want the mediaType.
                 if(!response.isSuccessful()) {
                     throw new ApiExceptionBuilder()
                             .withMessage(ErrorConstants.Codes.GENERAL_EXCEPTION)
                             .withResponseStatusCode(response.code())
                             .withResponseHeaders(HeadersCompatibility.getResponseHeaders(response.headers()))
                             .build();
                 }
-                UploadResult<T> uploadResult = new UploadResult<>();
-                if (response.code() == HttpURLConnection.HTTP_CREATED) {
-                    if (body.contentLength() > 0) {
-                        final ParseNode uploadTypeParseNode = parseNodeFactory.getParseNode(contentType, in);
-                        uploadResult.itemResponse = uploadTypeParseNode.getObjectValue(factory);
-                    }
-                    final String location = response.headers().get("location");
-                    if(!Objects.isNull(location) && !location.isEmpty()) {
-                        uploadResult.location = new URI(location);
-                    }
-                } else {
+                boolean canBeParsed = (!Objects.isNull(contentLengthHeader) && Integer.parseInt(contentLengthHeader) > 0) || !Objects.isNull(body.contentType());
+                String contentType = canBeParsed ? body.contentType().toString().split(";")[0] : null; //contentType.toString() returns in format <mediaType>;<charset>, we only want the mediaType.
+                if (canBeParsed) {
                     final ParseNode parseNode = parseNodeFactory.getParseNode(contentType, in);
-                    final UploadSession uploadSession = parseNode.getObjectValue(UploadSession::createFromDiscriminatorValue);
-                    final List<String> nextExpectedRanges = uploadSession.getNextExpectedRanges();
-                    if (!(nextExpectedRanges == null || nextExpectedRanges.isEmpty())) {
-                        uploadResult.uploadSession = uploadSession;
-                    } else {
+                    if (response.code() == HttpURLConnection.HTTP_CREATED) {
                         uploadResult.itemResponse = parseNode.getObjectValue(factory);
+                    } else {
+                        final UploadSession uploadSession = parseNode.getObjectValue(UploadSession::createFromDiscriminatorValue);
+                        final List<String> nextExpectedRanges = uploadSession.getNextExpectedRanges();
+                        if (!(nextExpectedRanges == null || nextExpectedRanges.isEmpty())) {
+                            uploadResult.uploadSession = uploadSession;
+                        } else {
+                            uploadResult.itemResponse = parseNode.getObjectValue(factory);
+                        }
                     }
                 }
                 return uploadResult;
@@ -91,6 +101,7 @@ public <T extends Parsable> UploadResult<T> handleResponse(@Nonnull final Respon
             throw new RuntimeException(ex);
         }
     }
+
 }
 
 

src/main/java/com/microsoft/graph/core/tasks/LargeFileUploadTask.java

@@ -201,7 +201,7 @@ public IUploadSession updateSessionStatus() {
         return session;
     }
     private UploadResult<T> uploadSlice(UploadSliceRequestBuilder<T> uploadSliceRequestBuilder, ArrayList<Throwable> exceptionsList) throws IOException {
-        byte[] buffer = chunkInputStream(uploadStream,(int) uploadSliceRequestBuilder.getRangeBegin(), (int)uploadSliceRequestBuilder.getRangeLength());
+        byte[] buffer = chunkInputStream(uploadStream, (int)uploadSliceRequestBuilder.getRangeLength());
         ByteArrayInputStream chunkStream = new ByteArrayInputStream(buffer);
         try {
             return uploadSliceRequestBuilder.put(chunkStream);
@@ -275,9 +275,9 @@ private long nextSliceSize(long rangeBegin, long rangeEnd)  {
         long size = rangeEnd - rangeBegin + 1;
         return Math.min(size, this.maxSliceSize);
     }
-    private byte[] chunkInputStream(InputStream stream, int begin, int length) throws IOException {
+    private byte[] chunkInputStream(InputStream stream, int length) throws IOException {
         byte[] buffer = new byte[length];
-        int lengthAssert = stream.read(buffer, begin, length);
+        int lengthAssert = stream.read(buffer);
         assert lengthAssert == length;
         return buffer;
     }

src/test/java/com/microsoft/graph/core/requests/upload/UploadResponseHandlerTest.java

@@ -11,11 +11,16 @@
 import okhttp3.*;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.mockito.stubbing.Answer;
 
 import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 import static com.microsoft.kiota.serialization.ParseNodeFactoryRegistry.defaultInstance;
 
+import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.time.OffsetDateTime;
 
@@ -24,7 +29,7 @@ class UploadResponseHandlerTest {
     ParseNodeFactoryRegistry registry = defaultInstance;
 
     @Test
-    void GetUploadItemOnCompletedUpload() {
+    void GetUploadItemOnCompletedUpload() throws IOException {
         registry.contentTypeAssociatedFactories.put(CoreConstants.MimeTypeNames.APPLICATION_JSON, new JsonParseNodeFactory());
 
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
@@ -41,8 +46,9 @@ void GetUploadItemOnCompletedUpload() {
             .body(body)
             .code(HttpURLConnection.HTTP_CREATED)
             .build();
+        OkHttpClient mockHttpClient = getMockClient(response);
         UploadResult<TestDriveItem> result = responseHandler
-            .handleResponse(response, TestDriveItem::createFromDiscriminatorValue);
+            .handleResponse(mockHttpClient.newCall(mock(Request.class)).execute(), TestDriveItem::createFromDiscriminatorValue);
         responseHandler.handleResponse(response, parseNode -> {return new TestDriveItem();});
         TestDriveItem item = result.itemResponse;
         assertTrue(result.isUploadSuccessful());
@@ -53,7 +59,7 @@ void GetUploadItemOnCompletedUpload() {
     }
 
     @Test
-    void GetUploadItemOnCompletedUpdate() {
+    void GetUploadItemOnCompletedUpdate() throws IOException {
         registry.contentTypeAssociatedFactories.put(CoreConstants.MimeTypeNames.APPLICATION_JSON, new JsonParseNodeFactory());
 
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
@@ -70,8 +76,9 @@ void GetUploadItemOnCompletedUpdate() {
             .code(HttpURLConnection.HTTP_OK)
             .message("OK")
             .build();
+        OkHttpClient mockHttpClient = getMockClient(response);
         UploadResult<TestDriveItem> result = responseHandler
-            .handleResponse(response, TestDriveItem::createFromDiscriminatorValue);
+            .handleResponse(mockHttpClient.newCall(mock(Request.class)).execute(), TestDriveItem::createFromDiscriminatorValue);
         responseHandler.handleResponse(response, parseNode -> {return new TestDriveItem();});
         TestDriveItem item = result.itemResponse;
         assertTrue(result.isUploadSuccessful());
@@ -82,28 +89,29 @@ void GetUploadItemOnCompletedUpdate() {
     }
 
     @Test
-    void getFileAttachmentLocationOnCompletedUpload() {
+    void getFileAttachmentLocationOnCompletedUpload() throws IOException {
         registry.contentTypeAssociatedFactories.put(CoreConstants.MimeTypeNames.APPLICATION_JSON, new JsonParseNodeFactory());
 
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
         Response response = new Response.Builder()
             .request(mock(Request.class))
             .protocol(mock(Protocol.class))
             .message("success")
-            .body(ResponseBody.create("", MediaType.parse(CoreConstants.MimeTypeNames.APPLICATION_JSON)))
             .code(HttpURLConnection.HTTP_CREATED)
             .header("location", "http://localhost")
             .build();
+        OkHttpClient mockClient = getMockClient(response);
         UploadResult<TestDriveItem> result = responseHandler
-            .handleResponse(response,TestDriveItem::createFromDiscriminatorValue);
+            .handleResponse(mockClient.newCall(mock(Request.class)).execute()
+            ,TestDriveItem::createFromDiscriminatorValue);
         TestDriveItem item = result.itemResponse;
 
         assertTrue(result.isUploadSuccessful());
         assertNull(item);
         assertEquals("http://localhost", result.location.toString());
     }
     @Test
-    void getUploadSessionOnProgressingUpload() {
+    void getUploadSessionOnProgressingUpload() throws IOException {
         registry.contentTypeAssociatedFactories.put(CoreConstants.MimeTypeNames.APPLICATION_JSON, new JsonParseNodeFactory());
 
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
@@ -123,8 +131,9 @@ void getUploadSessionOnProgressingUpload() {
             .body(body)
             .code(HttpURLConnection.HTTP_ACCEPTED)
             .build();
+        OkHttpClient mockHttpClient = getMockClient(response);
         UploadResult<TestDriveItem> result = responseHandler
-            .handleResponse(response, TestDriveItem::createFromDiscriminatorValue);
+            .handleResponse(mockHttpClient.newCall(mock(Request.class)).execute(), TestDriveItem::createFromDiscriminatorValue);
         UploadSession session = (UploadSession) result.uploadSession;
 
         assertFalse(result.isUploadSuccessful());
@@ -137,7 +146,7 @@ void getUploadSessionOnProgressingUpload() {
     }
 
     @Test
-    void throwsServiceExceptionOnErrorResponse() {
+    void throwsServiceExceptionOnErrorResponse() throws IOException {
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
         ResponseBody body = ResponseBody.create("{\n" +
             "   \"error\": {\n"+
@@ -157,17 +166,18 @@ void throwsServiceExceptionOnErrorResponse() {
             .body(body)
             .code(HttpURLConnection.HTTP_UNAUTHORIZED)
             .build();
+        OkHttpClient mockHttpClient = getMockClient(response);
 
         try {
             responseHandler
-                .handleResponse(response, TestDriveItem::createFromDiscriminatorValue);
+                .handleResponse(mockHttpClient.newCall(mock(Request.class)).execute(), TestDriveItem::createFromDiscriminatorValue);
         } catch (ApiException ex) {
             Assertions.assertEquals(ErrorConstants.Codes.GENERAL_EXCEPTION, ex.getMessage());
             assertEquals(HttpURLConnection.HTTP_UNAUTHORIZED, ex.getResponseStatusCode());
         }
     }
     @Test
-    void throwsSerializationErrorOnInvalidJson() {
+    void throwsSerializationErrorOnInvalidJson() throws IOException {
         UploadResponseHandler responseHandler = new UploadResponseHandler(null);
         String malformedResponse =
             "   \"error\": {\n"+
@@ -188,11 +198,31 @@ void throwsSerializationErrorOnInvalidJson() {
             .body(body)
             .code(HttpURLConnection.HTTP_UNAUTHORIZED)
             .build();
+        OkHttpClient mockHttpClient = getMockClient(response);
         try {
             responseHandler
-                .handleResponse(response, TestDriveItem::createFromDiscriminatorValue);
+                .handleResponse(mockHttpClient.newCall(mock(Request.class)).execute(), TestDriveItem::createFromDiscriminatorValue);
         } catch (ApiException ex) {
             assertEquals(ErrorConstants.Codes.GENERAL_EXCEPTION, ex.getMessage());
         }
     }
+
+    public static OkHttpClient getMockClient(final Response response) throws IOException {
+        final OkHttpClient mockClient = mock(OkHttpClient.class);
+        final Call remoteCall = mock(Call.class);
+        final Dispatcher dispatcher = new Dispatcher();
+        when(remoteCall.execute()).thenReturn(response);
+        doAnswer(
+                        (Answer<Void>)
+                                invocation -> {
+                                    Callback callback = invocation.getArgument(0);
+                                    callback.onResponse(null, response);
+                                    return null;
+                                })
+                .when(remoteCall)
+                .enqueue(any(Callback.class));
+        when(mockClient.dispatcher()).thenReturn(dispatcher);
+        when(mockClient.newCall(any())).thenReturn(remoteCall);
+        return mockClient;
+    }
 }