fix: resolved handling of larger batch request message

Andrew Omondi · Andrew Omondi · commit c78b39d44e7e · 2024-09-10T11:47:31.000+03:00
diff --git a/src/Microsoft.Graph.Core/Exceptions/ErrorConstants.cs b/src/Microsoft.Graph.Core/Exceptions/ErrorConstants.cs
@@ -83,6 +83,8 @@ internal static class Messages
 
             internal static string PageIteratorRequestError = "Error occured when making a request with the page iterator. See inner exception for more details.";
 
+            internal static string BatchRequestError = "Error occured when making the batch request. See inner exception for more details.";
+
             public static string InvalidProxyArgument = "Proxy cannot be set more once. Proxy can only be set on the proxy or defaultHttpHandler argument and not both.";
         }
     }
diff --git a/src/Microsoft.Graph.Core/Extensions/IParseNodeExtensions.cs b/src/Microsoft.Graph.Core/Extensions/IParseNodeExtensions.cs
@@ -0,0 +1,16 @@
+﻿using Microsoft.Kiota.Abstractions.Serialization;
+
+namespace Microsoft.Graph;
+
+/// <summary>
+/// Extension helpers for the <see cref="IParseNode"/>
+/// </summary>
+public static class ParseNodeExtensions
+{
+    internal static string GetErrorMessage(this IParseNode responseParseNode)
+    {
+        var errorParseNode = responseParseNode.GetChildNode("error");
+        // concatenate the error code and message
+        return $"{errorParseNode?.GetChildNode("code")?.GetStringValue()} : {errorParseNode?.GetChildNode("message")?.GetStringValue()}";
+    }
+}
diff --git a/src/Microsoft.Graph.Core/Requests/BatchRequestBuilder.cs b/src/Microsoft.Graph.Core/Requests/BatchRequestBuilder.cs
@@ -7,10 +7,12 @@ namespace Microsoft.Graph.Core.Requests
     using System;
     using System.Collections.Generic;
     using System.Net.Http;
+    using System.Text.Json;
     using System.Threading;
     using System.Threading.Tasks;
     using Microsoft.Kiota.Abstractions;
     using Microsoft.Kiota.Abstractions.Serialization;
+    using Microsoft.Kiota.Serialization.Json;
 
     /// <summary>
     /// The type BatchRequestBuilder
@@ -58,7 +60,9 @@ public async Task<BatchResponseContent> PostAsync(BatchRequestContent batchReque
             var nativeResponseHandler = new NativeResponseHandler();
             requestInfo.SetResponseHandler(nativeResponseHandler);
             await this.RequestAdapter.SendNoContentAsync(requestInfo, cancellationToken: cancellationToken);
-            return new BatchResponseContent(nativeResponseHandler.Value as HttpResponseMessage, errorMappings);
+            var httpResponseMessage = nativeResponseHandler.Value as HttpResponseMessage;
+            await ThrowIfFailedResponseAsync(httpResponseMessage);
+            return new BatchResponseContent(httpResponseMessage, errorMappings);
         }
 
         /// <summary>
@@ -99,5 +103,26 @@ public async Task<RequestInformation> ToPostRequestInformationAsync(BatchRequest
             requestInfo.Headers.Add("Content-Type", CoreConstants.MimeTypeNames.Application.Json);
             return requestInfo;
         }
+
+        private static async Task ThrowIfFailedResponseAsync(HttpResponseMessage httpResponseMessage)
+        {
+            if (httpResponseMessage.IsSuccessStatusCode) return;
+
+            if (CoreConstants.MimeTypeNames.Application.Json.Equals(httpResponseMessage.Content?.Headers?.ContentType?.MediaType, StringComparison.OrdinalIgnoreCase))
+            {
+                using var responseContent = await httpResponseMessage.Content.ReadAsStreamAsync().ConfigureAwait(false);
+                using var document = await JsonDocument.ParseAsync(responseContent).ConfigureAwait(false);
+                var parsable = new JsonParseNode(document.RootElement);
+                throw new ServiceException(ErrorConstants.Messages.BatchRequestError, httpResponseMessage.Headers, (int)httpResponseMessage.StatusCode, new Exception(parsable.GetErrorMessage()));
+            }
+
+            var responseStringContent = string.Empty;
+            if (httpResponseMessage.Content != null)
+            {
+                responseStringContent = await httpResponseMessage.Content.ReadAsStringAsync().ConfigureAwait(false);
+            }
+
+            throw new ServiceException(ErrorConstants.Messages.BatchRequestError, httpResponseMessage.Headers, (int)httpResponseMessage.StatusCode, responseStringContent);
+        }
     }
 }
diff --git a/src/Microsoft.Graph.Core/Requests/Content/BatchResponseContent.cs b/src/Microsoft.Graph.Core/Requests/Content/BatchResponseContent.cs
@@ -32,7 +32,9 @@ public class BatchResponseContent
         public BatchResponseContent(HttpResponseMessage httpResponseMessage, Dictionary<string, ParsableFactory<IParsable>> errorMappings = null)
         {
             this.batchResponseMessage = httpResponseMessage ?? throw new ArgumentNullException(nameof(httpResponseMessage));
-            this.apiErrorMappings = errorMappings ?? new();
+            this.apiErrorMappings = errorMappings ?? new Dictionary<string, ParsableFactory<IParsable>>(StringComparer.OrdinalIgnoreCase) {
+                {"XXX", (parsable) => new ServiceException(ErrorConstants.Messages.BatchRequestError, new Exception(parsable.GetErrorMessage())) }
+            };
         }
 
         /// <summary>
diff --git a/src/Microsoft.Graph.Core/Tasks/PageIterator.cs b/src/Microsoft.Graph.Core/Tasks/PageIterator.cs
@@ -115,8 +115,7 @@ public static PageIterator<TEntity, TCollectionPage> CreatePageIterator(IRequest
                 _processPageItemCallback = callback,
                 _requestConfigurator = requestConfigurator,
                 _errorMapping = errorMapping ?? new Dictionary<string, ParsableFactory<IParsable>>(StringComparer.OrdinalIgnoreCase) {
-                    {"4XX", (parsable) => new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(GetErrorMessageFromParsable(parsable))) },
-                    {"5XX", (parsable) => new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(GetErrorMessageFromParsable(parsable))) }
+                    {"XXX", (parsable) => new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(parsable.GetErrorMessage())) }
                 },
                 State = PagingState.NotStarted
             };
@@ -172,15 +171,14 @@ public static PageIterator<TEntity, TCollectionPage> CreatePageIterator(IRequest
                 _asyncProcessPageItemCallback = asyncCallback,
                 _requestConfigurator = requestConfigurator,
                 _errorMapping = errorMapping ?? new Dictionary<string, ParsableFactory<IParsable>>(StringComparer.OrdinalIgnoreCase) {
-                    {"4XX", (parsable) => new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(GetErrorMessageFromParsable(parsable))) },
-                    {"5XX", (parsable) =>new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(GetErrorMessageFromParsable(parsable))) },
+                    {"XXX", (parsable) => new ServiceException(ErrorConstants.Messages.PageIteratorRequestError,new Exception(parsable.GetErrorMessage())) }
                 },
                 State = PagingState.NotStarted
             };
         }
 
         /// <summary>
-        /// Iterate across the content of a a single results page with the callback.
+        /// Iterate across the content of a single results page with the callback.
         /// </summary>
         /// <returns>A boolean value that indicates whether the callback cancelled 
         /// iterating across the page results or whether there are more pages to page. 
@@ -393,13 +391,6 @@ private static string ExtractNextLinkFromParsable(TCollectionPage parsableCollec
             // the next link property may not be defined in the response schema so we also check its presence in the additional data bag
             return parsableCollection.AdditionalData.TryGetValue(CoreConstants.OdataInstanceAnnotations.NextLink, out var nextLink) ? nextLink.ToString() : string.Empty;
         }
-
-        private static string GetErrorMessageFromParsable(IParseNode responseParseNode)
-        {
-            var errorParseNode = responseParseNode.GetChildNode("error");
-            // concatenate the error code and message
-            return $"{errorParseNode?.GetChildNode("code")?.GetStringValue()} : {errorParseNode?.GetChildNode("message")?.GetStringValue()}";
-        }
     }
 
     /// <summary>
diff --git a/tests/Microsoft.Graph.DotnetCore.Core.Test/Requests/BatchRequestBuilderTests.cs b/tests/Microsoft.Graph.DotnetCore.Core.Test/Requests/BatchRequestBuilderTests.cs
@@ -6,10 +6,15 @@ namespace Microsoft.Graph.DotnetCore.Core.Test.Requests
 {
     using System.Collections.Generic;
     using System.Net.Http;
+    using System.Text;
+    using System.Threading;
     using System.Threading.Tasks;
     using Microsoft.Graph.Core.Requests;
     using Microsoft.Graph.DotnetCore.Core.Test.Mocks;
+    using Microsoft.Kiota.Abstractions;
     using Microsoft.Kiota.Abstractions.Authentication;
+    using Microsoft.Kiota.Abstractions.Serialization;
+    using Moq;
     using Xunit;
 
     public class BatchRequestBuilderTests
@@ -42,5 +47,119 @@ public async Task BatchRequestBuilderAsync()
             Assert.Equal("{+baseurl}/$batch", requestInformation.UrlTemplate);
             Assert.Equal(baseClient.RequestAdapter, batchRequestBuilder.RequestAdapter);
         }
+
+
+        [Fact]
+        public async Task BatchRequestBuilderPostAsyncHandlesDoesNotThrowExceptionAsync()
+        {
+            // Arrange
+            var requestAdapter = new Mock<IRequestAdapter>();
+            IBaseClient baseClient = new BaseClient(requestAdapter.Object);
+
+            var errorResponseMessage = new HttpResponseMessage(System.Net.HttpStatusCode.OK)
+            {
+                Content = new StringContent("{}", Encoding.UTF8, "application/json"),//dummy content
+            };
+            requestAdapter
+                .Setup(requestAdapter => requestAdapter.SendNoContentAsync(It.IsAny<RequestInformation>(), It.IsAny<Dictionary<string, ParsableFactory<IParsable>>>(), It.IsAny<CancellationToken>()))
+                .Callback((RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>> errorMapping, CancellationToken cancellationToken) => ((NativeResponseHandler)requestInfo.GetRequestOption<ResponseHandlerOption>().ResponseHandler).Value = errorResponseMessage)
+                .Returns(Task.FromResult(0));
+
+            // Act
+            var batchRequestBuilder = new BatchRequestBuilder(baseClient.RequestAdapter);
+
+            // 4. Create batch request content to be sent out
+            // 4.1 Create HttpRequestMessages for the content
+            HttpRequestMessage httpRequestMessage1 = new HttpRequestMessage(HttpMethod.Get, "https://graph.microsoft.com/v1.0/me/");
+
+            // 4.2 Create batch request steps with request ids.
+            BatchRequestStep requestStep1 = new BatchRequestStep("1", httpRequestMessage1);
+
+            // 4.3 Add batch request steps to BatchRequestContent.
+#pragma warning disable CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            BatchRequestContent batchRequestContent = new BatchRequestContent(baseClient, requestStep1);
+#pragma warning restore CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            var responseContent = await batchRequestBuilder.PostAsync(batchRequestContent);
+
+            // Assert
+            Assert.NotNull(responseContent);
+        }
+
+        [Fact]
+        public async Task BatchRequestBuilderPostAsyncHandlesNonSuccessStatusWithJsonResponseAsync()
+        {
+            // Arrange
+            var requestAdapter = new Mock<IRequestAdapter>();
+            IBaseClient baseClient = new BaseClient(requestAdapter.Object);
+
+            var errorResponseMessage = new HttpResponseMessage(System.Net.HttpStatusCode.Unauthorized)
+            {
+                Content = new StringContent("{\"error\": {\"code\": \"20117\",\"message\": \"An item with this name already exists in this location.\",\"innerError\":{\"request-id\": \"nothing1b13-45cd-new-92be873c5781\",\"date\": \"2019-03-22T23:17:50\"}}}", Encoding.UTF8, "application/json"),
+            };
+            requestAdapter
+                .Setup(requestAdapter => requestAdapter.SendNoContentAsync(It.IsAny<RequestInformation>(), It.IsAny<Dictionary<string, ParsableFactory<IParsable>>>(), It.IsAny<CancellationToken>()))
+                .Callback((RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>> errorMapping, CancellationToken cancellationToken) => ((NativeResponseHandler)requestInfo.GetRequestOption<ResponseHandlerOption>().ResponseHandler).Value = errorResponseMessage)
+                .Returns(Task.FromResult(0));
+
+            // Act
+            var batchRequestBuilder = new BatchRequestBuilder(baseClient.RequestAdapter);
+
+            // 4. Create batch request content to be sent out
+            // 4.1 Create HttpRequestMessages for the content
+            HttpRequestMessage httpRequestMessage1 = new HttpRequestMessage(HttpMethod.Get, "https://graph.microsoft.com/v1.0/me/");
+
+            // 4.2 Create batch request steps with request ids.
+            BatchRequestStep requestStep1 = new BatchRequestStep("1", httpRequestMessage1);
+
+            // 4.3 Add batch request steps to BatchRequestContent.
+#pragma warning disable CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            BatchRequestContent batchRequestContent = new BatchRequestContent(baseClient, requestStep1);
+#pragma warning restore CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            var serviceException = await Assert.ThrowsAsync<ServiceException>(async () => await batchRequestBuilder.PostAsync(batchRequestContent));
+
+            // Assert
+            Assert.Equal(ErrorConstants.Messages.BatchRequestError, serviceException.Message);
+            Assert.Equal(401, serviceException.ResponseStatusCode);
+            Assert.NotNull(serviceException.InnerException);
+            Assert.Equal("20117 : An item with this name already exists in this location.", serviceException.InnerException.Message);
+        }
+
+        [Fact]
+        public async Task BatchRequestBuilderPostAsyncHandlesNonSuccessStatusWithNonJsonResponseAsync()
+        {
+            // Arrange
+            var requestAdapter = new Mock<IRequestAdapter>();
+            IBaseClient baseClient = new BaseClient(requestAdapter.Object);
+
+            var errorResponseMessage = new HttpResponseMessage(System.Net.HttpStatusCode.Conflict)
+            {
+                Content = new StringContent("<html>This is random html</html>", Encoding.UTF8, "text/plain"),
+            };
+            requestAdapter
+                .Setup(requestAdapter => requestAdapter.SendNoContentAsync(It.IsAny<RequestInformation>(), It.IsAny<Dictionary<string, ParsableFactory<IParsable>>>(), It.IsAny<CancellationToken>()))
+                .Callback((RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>> errorMapping, CancellationToken cancellationToken) => ((NativeResponseHandler)requestInfo.GetRequestOption<ResponseHandlerOption>().ResponseHandler).Value = errorResponseMessage)
+                .Returns(Task.FromResult(0));
+
+            // Act
+            var batchRequestBuilder = new BatchRequestBuilder(baseClient.RequestAdapter);
+
+            // 4. Create batch request content to be sent out
+            // 4.1 Create HttpRequestMessages for the content
+            HttpRequestMessage httpRequestMessage1 = new HttpRequestMessage(HttpMethod.Get, "https://graph.microsoft.com/v1.0/me/");
+
+            // 4.2 Create batch request steps with request ids.
+            BatchRequestStep requestStep1 = new BatchRequestStep("1", httpRequestMessage1);
+
+            // 4.3 Add batch request steps to BatchRequestContent.
+#pragma warning disable CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            BatchRequestContent batchRequestContent = new BatchRequestContent(baseClient, requestStep1);
+#pragma warning restore CS0618 // Type or member is obsolete use the BatchRequestContentCollection for making batch requests
+            var serviceException = await Assert.ThrowsAsync<ServiceException>(async () => await batchRequestBuilder.PostAsync(batchRequestContent));
+
+            // Assert
+            Assert.Equal(ErrorConstants.Messages.BatchRequestError, serviceException.Message);
+            Assert.Equal(409, serviceException.ResponseStatusCode);
+            Assert.Equal("<html>This is random html</html>", serviceException.RawResponseBody);
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -83,6 +83,8 @@ internal static class Messages`
`83`	`83`
`84`	`84`	`internal static string PageIteratorRequestError = "Error occured when making a request with the page iterator. See inner exception for more details.";`
`85`	`85`
	`86`	`+ internal static string BatchRequestError = "Error occured when making the batch request. See inner exception for more details.";`
	`87`	`+`
`86`	`88`	`public static string InvalidProxyArgument = "Proxy cannot be set more once. Proxy can only be set on the proxy or defaultHttpHandler argument and not both.";`
`87`	`89`	`}`
`88`	`90`	`}`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,9 @@ public class BatchResponseContent`
`32`	`32`	`public BatchResponseContent(HttpResponseMessage httpResponseMessage, Dictionary<string, ParsableFactory<IParsable>> errorMappings = null)`
`33`	`33`	`{`
`34`	`34`	`this.batchResponseMessage = httpResponseMessage ?? throw new ArgumentNullException(nameof(httpResponseMessage));`
`35`		`- this.apiErrorMappings = errorMappings ?? new();`
	`35`	`+ this.apiErrorMappings = errorMappings ?? new Dictionary<string, ParsableFactory<IParsable>>(StringComparer.OrdinalIgnoreCase) {`
	`36`	`+ {"XXX", (parsable) => new ServiceException(ErrorConstants.Messages.BatchRequestError, new Exception(parsable.GetErrorMessage())) }`
	`37`	`+ };`
`36`	`38`	`}`
`37`	`39`
`38`	`40`	`/// <summary>`