From 97f70e7f66a24b59a59843868f412af84d17d0a6 Mon Sep 17 00:00:00 2001 From: Joshua Flanagan Date: Fri, 28 Jun 2024 14:54:24 -0500 Subject: [PATCH 1/3] Handle unexpected HTTP responses If the response cannot be parsed as JSON, we will throw a ShipEngineException, and include the actual response. --- ShipEngine.Tests/ShipEngineClientTests.cs | 115 ++++++++++++++++++++++ ShipEngine/ShipEngineClient.cs | 42 ++++---- 2 files changed, 138 insertions(+), 19 deletions(-) create mode 100644 ShipEngine.Tests/ShipEngineClientTests.cs diff --git a/ShipEngine.Tests/ShipEngineClientTests.cs b/ShipEngine.Tests/ShipEngineClientTests.cs new file mode 100644 index 00000000..27c0aae5 --- /dev/null +++ b/ShipEngine.Tests/ShipEngineClientTests.cs @@ -0,0 +1,115 @@ +namespace ShipEngineTest +{ + using ShipEngineSDK; + using ShipEngineSDK.VoidLabelWithLabelId; + using System; + using System.Net.Http; + using System.Threading.Tasks; + using Xunit; + + public class ShipEngineClientTests + { + [Fact] + public async Task FailureWithShipengineResponseThrowsPopulatedShipEngineException() + { + var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); + var mockShipEngineFixture = new MockShipEngineFixture(config); + var shipengine = mockShipEngineFixture.ShipEngine; + + + string requestId = "12345"; + string message = "Request body cannot be empty."; + var responseBody = string.Format( + @" + {{ + ""request_id"": ""{0}"", + ""errors"": [ + {{ + ""error_source"": ""shipengine"", + ""error_type"": ""validation"", + ""error_code"": ""request_body_required"", + ""message"": ""{1}"" + }} + ] + }}", requestId, message); + + + mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.BadRequest, + responseBody); + var ex = await Assert.ThrowsAsync( + async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", + mockShipEngineFixture.HttpClient, config) + ); + + Assert.Equal(requestId, ex.RequestId); + Assert.Equal(message, ex.Message); + Assert.Equal(ErrorSource.Shipengine, ex.ErrorSource); + Assert.Equal(ErrorType.Validation, ex.ErrorType); + Assert.Equal(ErrorCode.RequestBodyRequired, ex.ErrorCode); + Assert.NotNull(ex.ResponseMessage); + Assert.Equal(400, (int)ex.ResponseMessage.StatusCode); + } + + [Fact] + public async Task FailureWithoutShipengineResponseThrowsHttpException() + { + var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); + var mockShipEngineFixture = new MockShipEngineFixture(config); + var shipengine = mockShipEngineFixture.ShipEngine; + + var responseBody = @"

Bad Gateway

"; + mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.BadGateway, + responseBody); + var ex = await Assert.ThrowsAsync( + async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", + mockShipEngineFixture.HttpClient, config) + ); + + Assert.Contains("502", ex.Message); + } + + [Fact] + public async Task FailureToParseSuccessResponseThrowsExceptionWithUnparsedResponse() + { + var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); + var mockShipEngineFixture = new MockShipEngineFixture(config); + var shipengine = mockShipEngineFixture.ShipEngine; + + var responseBody = @"Unexpected response - not JSON"; + mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, + responseBody); + var ex = await Assert.ThrowsAsync( + async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", + mockShipEngineFixture.HttpClient, config) + ); + mockShipEngineFixture.AssertRequest(HttpMethod.Post, "/v1/something"); + + Assert.NotNull(ex.ResponseMessage); + Assert.Equal(200, (int)ex.ResponseMessage.StatusCode); + Assert.Equal(responseBody, await ex.ResponseMessage.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task NullStringResponseThrowsExceptionWithUnparsedResponse() + { + var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); + var mockShipEngineFixture = new MockShipEngineFixture(config); + var shipengine = mockShipEngineFixture.ShipEngine; + + // this scenario is similar to unparseable JSON - except that it is valid JSON + var responseBody = @"null"; + mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, + responseBody); + var ex = await Assert.ThrowsAsync( + async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", + mockShipEngineFixture.HttpClient, config) + ); + mockShipEngineFixture.AssertRequest(HttpMethod.Post, "/v1/something"); + + Assert.NotNull(ex.ResponseMessage); + Assert.Equal("Unexpected null response", ex.Message); + Assert.Equal(200, (int)ex.ResponseMessage.StatusCode); + Assert.Equal(responseBody, await ex.ResponseMessage.Content.ReadAsStringAsync()); + } + } +} \ No newline at end of file diff --git a/ShipEngine/ShipEngineClient.cs b/ShipEngine/ShipEngineClient.cs index b6a25afb..96d7e02b 100644 --- a/ShipEngine/ShipEngineClient.cs +++ b/ShipEngine/ShipEngineClient.cs @@ -91,7 +91,15 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) if (!response.IsSuccessStatusCode) { - var deserializedError = JsonSerializer.Deserialize(contentString, JsonSerializerOptions); + ShipEngineAPIError? deserializedError = null; + try + { + deserializedError = + JsonSerializer.Deserialize(contentString, JsonSerializerOptions); + } + catch (JsonException) + { + } // Throw Generic HttpClient Error if unable to deserialize to a ShipEngineException if (deserializedError == null) @@ -115,14 +123,23 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) } - var result = JsonSerializer.Deserialize(contentString, JsonSerializerOptions); + T? result; + try + { + result = JsonSerializer.Deserialize(contentString, JsonSerializerOptions); + } + catch (JsonException) + { + throw new ShipEngineException("Unable to parse response", responseMessage: response); + } + if (result != null) { return result; } - throw new ShipEngineException(message: "Unexpected Error"); + throw new ShipEngineException(message: "Unexpected null response", responseMessage: response); } @@ -148,8 +165,7 @@ public virtual async Task SendHttpRequestAsync(HttpMethod method, string p try { var request = BuildRequest(method, path, jsonContent); - var streamTask = client.SendAsync(request, CancellationToken); - response = await streamTask; + response = await client.SendAsync(request, CancellationToken); var deserializedResult = await DeserializedResultOrThrow(response); @@ -159,17 +175,12 @@ public virtual async Task SendHttpRequestAsync(HttpMethod method, string p { if (e.ErrorCode != ErrorCode.RateLimitExceeded) { - throw e; + throw; } requestException = e; } - catch (Exception e) - { - throw e; - } - if (!ShouldRetry(retry, response?.StatusCode, response?.Headers, config)) { @@ -180,14 +191,7 @@ public virtual async Task SendHttpRequestAsync(HttpMethod method, string p await WaitAndRetry(response, config, requestException); } - if (requestException != null) - { - throw requestException; - } - else - { - throw new ShipEngineException(message: "Unexpected Error"); - } + throw requestException; } private async Task WaitAndRetry(HttpResponseMessage? response, Config config, ShipEngineException ex) From b4aec9ac7dace3ad058e3a5ecb4fb29d4c80c1f2 Mon Sep 17 00:00:00 2001 From: Joshua Flanagan Date: Fri, 28 Jun 2024 16:14:38 -0500 Subject: [PATCH 2/3] Always throw exception for non-success status code If error details weren't found, the user would get back an empty deserialized object. --- ShipEngine.Tests/ShipEngineClientTests.cs | 32 ++++++++++++++++++----- ShipEngine/ShipEngineClient.cs | 28 +++++++++----------- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/ShipEngine.Tests/ShipEngineClientTests.cs b/ShipEngine.Tests/ShipEngineClientTests.cs index 27c0aae5..dc78305d 100644 --- a/ShipEngine.Tests/ShipEngineClientTests.cs +++ b/ShipEngine.Tests/ShipEngineClientTests.cs @@ -10,7 +10,7 @@ namespace ShipEngineTest public class ShipEngineClientTests { [Fact] - public async Task FailureWithShipengineResponseThrowsPopulatedShipEngineException() + public async Task FailureStatusWithShipengineContentThrowsPopulatedShipEngineException() { var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); var mockShipEngineFixture = new MockShipEngineFixture(config); @@ -51,7 +51,26 @@ public async Task FailureWithShipengineResponseThrowsPopulatedShipEngineExceptio } [Fact] - public async Task FailureWithoutShipengineResponseThrowsHttpException() + public async Task FailureStatusWithoutShipEngineDetailsThrowsShipEngineExceptionWithOriginalResponse() + { + var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); + var mockShipEngineFixture = new MockShipEngineFixture(config); + var shipengine = mockShipEngineFixture.ShipEngine; + + var responseBody = @"{""description"": ""valid JSON, but not what you expect""}"; + mockShipEngineFixture.StubRequest(HttpMethod.Get, "/v1/something", System.Net.HttpStatusCode.NotFound, + responseBody); + var ex = await Assert.ThrowsAsync( + async () => await shipengine.SendHttpRequestAsync(HttpMethod.Get, "/v1/something", null, + mockShipEngineFixture.HttpClient, config) + ); + + Assert.NotNull(ex.ResponseMessage); + Assert.Equal(404, (int) ex.ResponseMessage.StatusCode); + } + + [Fact] + public async Task FailureStatusWithoutJsonContentThrowsShipEngineExceptionWithOriginalResponse() { var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); var mockShipEngineFixture = new MockShipEngineFixture(config); @@ -60,16 +79,17 @@ public async Task FailureWithoutShipengineResponseThrowsHttpException() var responseBody = @"

Bad Gateway

"; mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.BadGateway, responseBody); - var ex = await Assert.ThrowsAsync( + var ex = await Assert.ThrowsAsync( async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", mockShipEngineFixture.HttpClient, config) ); - Assert.Contains("502", ex.Message); + Assert.NotNull(ex.ResponseMessage); + Assert.Equal(502, (int) ex.ResponseMessage.StatusCode); } [Fact] - public async Task FailureToParseSuccessResponseThrowsExceptionWithUnparsedResponse() + public async Task SuccessResponseThatCannotBeParsedThrowsExceptionWithUnparsedResponse() { var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); var mockShipEngineFixture = new MockShipEngineFixture(config); @@ -90,7 +110,7 @@ public async Task FailureToParseSuccessResponseThrowsExceptionWithUnparsedRespon } [Fact] - public async Task NullStringResponseThrowsExceptionWithUnparsedResponse() + public async Task SuccessResponseWithNullContentThrowsShipEngineExceptionWithUnparsedResponse() { var config = new Config(apiKey: "test", timeout: TimeSpan.FromSeconds(0.5)); var mockShipEngineFixture = new MockShipEngineFixture(config); diff --git a/ShipEngine/ShipEngineClient.cs b/ShipEngine/ShipEngineClient.cs index 96d7e02b..99559774 100644 --- a/ShipEngine/ShipEngineClient.cs +++ b/ShipEngine/ShipEngineClient.cs @@ -101,26 +101,22 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) { } - // Throw Generic HttpClient Error if unable to deserialize to a ShipEngineException if (deserializedError == null) { - response.EnsureSuccessStatusCode(); - } - - var error = deserializedError?.Errors[0]; - - if (error != null && error.Message != null && deserializedError?.RequestId != null) - { - throw new ShipEngineException( - error.Message, - error.ErrorSource, - error.ErrorType, - error.ErrorCode, - deserializedError.RequestId, - response - ); + // in this case, the response body was not parseable JSON + throw new ShipEngineException("Unexpected HTTP status", responseMessage: response); } + var error = deserializedError.Errors?.FirstOrDefault(e => e.Message != null); + // if error is null, it means we got back a JSON response, but it wasn't the format we expected + throw new ShipEngineException( + error?.Message ?? response.ReasonPhrase, + error?.ErrorSource ?? ErrorSource.Shipengine, + error?.ErrorType ?? ErrorType.System, + error?.ErrorCode ?? ErrorCode.Unspecified, + deserializedError.RequestId, + response + ); } T? result; From 5997aafe9ba866eecf01ec13dcf1406108d7ae7b Mon Sep 17 00:00:00 2001 From: Joshua Flanagan Date: Fri, 28 Jun 2024 16:41:37 -0500 Subject: [PATCH 3/3] Populate request ID on ShipEngineException Capture the request ID from the response headers, in case the body does not include it. --- ShipEngine.Tests/Helpers/MockShipEngineFixture.cs | 7 ++++++- ShipEngine.Tests/ShipEngineClientTests.cs | 12 ++++++++---- ShipEngine/ShipEngineClient.cs | 14 ++++++++++---- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/ShipEngine.Tests/Helpers/MockShipEngineFixture.cs b/ShipEngine.Tests/Helpers/MockShipEngineFixture.cs index bfd91425..d5fca799 100644 --- a/ShipEngine.Tests/Helpers/MockShipEngineFixture.cs +++ b/ShipEngine.Tests/Helpers/MockShipEngineFixture.cs @@ -3,6 +3,7 @@ namespace ShipEngineTest using Moq; using Moq.Protected; using ShipEngineSDK; + using System; using System.Net; using System.Net.Http; using System.Text.Json; @@ -74,10 +75,13 @@ public void AssertRequest(HttpMethod method, string path, int numberOfCalls = 1) /// The HTTP path. /// The status code to return. /// The response body to return. - public void StubRequest(HttpMethod method, string path, HttpStatusCode status, string response) + public string StubRequest(HttpMethod method, string path, HttpStatusCode status, string response) { + var requestId = Guid.NewGuid().ToString(); var responseMessage = new HttpResponseMessage(status); responseMessage.Content = new StringContent(response); + responseMessage.Headers.Add("x-shipengine-requestid", requestId); + responseMessage.Headers.Add("request-id", requestId); MockHandler.Protected() .Setup>( @@ -87,6 +91,7 @@ public void StubRequest(HttpMethod method, string path, HttpStatusCode status, s m.RequestUri.AbsolutePath == path), ItExpr.IsAny()) .Returns(Task.FromResult(responseMessage)); + return requestId; } } } \ No newline at end of file diff --git a/ShipEngine.Tests/ShipEngineClientTests.cs b/ShipEngine.Tests/ShipEngineClientTests.cs index dc78305d..844b0e93 100644 --- a/ShipEngine.Tests/ShipEngineClientTests.cs +++ b/ShipEngine.Tests/ShipEngineClientTests.cs @@ -58,7 +58,7 @@ public async Task FailureStatusWithoutShipEngineDetailsThrowsShipEngineException var shipengine = mockShipEngineFixture.ShipEngine; var responseBody = @"{""description"": ""valid JSON, but not what you expect""}"; - mockShipEngineFixture.StubRequest(HttpMethod.Get, "/v1/something", System.Net.HttpStatusCode.NotFound, + var requestId = mockShipEngineFixture.StubRequest(HttpMethod.Get, "/v1/something", System.Net.HttpStatusCode.NotFound, responseBody); var ex = await Assert.ThrowsAsync( async () => await shipengine.SendHttpRequestAsync(HttpMethod.Get, "/v1/something", null, @@ -67,6 +67,7 @@ public async Task FailureStatusWithoutShipEngineDetailsThrowsShipEngineException Assert.NotNull(ex.ResponseMessage); Assert.Equal(404, (int) ex.ResponseMessage.StatusCode); + Assert.Equal(requestId, ex.RequestId); } [Fact] @@ -77,7 +78,7 @@ public async Task FailureStatusWithoutJsonContentThrowsShipEngineExceptionWithOr var shipengine = mockShipEngineFixture.ShipEngine; var responseBody = @"

Bad Gateway

"; - mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.BadGateway, + var requestId = mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.BadGateway, responseBody); var ex = await Assert.ThrowsAsync( async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", @@ -86,6 +87,7 @@ public async Task FailureStatusWithoutJsonContentThrowsShipEngineExceptionWithOr Assert.NotNull(ex.ResponseMessage); Assert.Equal(502, (int) ex.ResponseMessage.StatusCode); + Assert.Equal(requestId, ex.RequestId); } [Fact] @@ -96,7 +98,7 @@ public async Task SuccessResponseThatCannotBeParsedThrowsExceptionWithUnparsedRe var shipengine = mockShipEngineFixture.ShipEngine; var responseBody = @"Unexpected response - not JSON"; - mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, + var requestId = mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, responseBody); var ex = await Assert.ThrowsAsync( async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", @@ -107,6 +109,7 @@ public async Task SuccessResponseThatCannotBeParsedThrowsExceptionWithUnparsedRe Assert.NotNull(ex.ResponseMessage); Assert.Equal(200, (int)ex.ResponseMessage.StatusCode); Assert.Equal(responseBody, await ex.ResponseMessage.Content.ReadAsStringAsync()); + Assert.Equal(requestId, ex.RequestId); } [Fact] @@ -118,7 +121,7 @@ public async Task SuccessResponseWithNullContentThrowsShipEngineExceptionWithUnp // this scenario is similar to unparseable JSON - except that it is valid JSON var responseBody = @"null"; - mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, + var requestId = mockShipEngineFixture.StubRequest(HttpMethod.Post, "/v1/something", System.Net.HttpStatusCode.OK, responseBody); var ex = await Assert.ThrowsAsync( async () => await shipengine.SendHttpRequestAsync(HttpMethod.Post, "/v1/something", "", @@ -130,6 +133,7 @@ public async Task SuccessResponseWithNullContentThrowsShipEngineExceptionWithUnp Assert.Equal("Unexpected null response", ex.Message); Assert.Equal(200, (int)ex.ResponseMessage.StatusCode); Assert.Equal(responseBody, await ex.ResponseMessage.Content.ReadAsStringAsync()); + Assert.Equal(requestId, ex.RequestId); } } } \ No newline at end of file diff --git a/ShipEngine/ShipEngineClient.cs b/ShipEngine/ShipEngineClient.cs index 99559774..9a7ee8b9 100644 --- a/ShipEngine/ShipEngineClient.cs +++ b/ShipEngine/ShipEngineClient.cs @@ -88,6 +88,12 @@ public static HttpClient ConfigureHttpClient(HttpClient client, string apiKey, U private async Task DeserializedResultOrThrow(HttpResponseMessage response) { var contentString = await response.Content.ReadAsStringAsync(); + string? requestId = null; + if (response.Headers.TryGetValues("x-shipengine-requestid", out var requestIdValues)) + { + requestId = requestIdValues.FirstOrDefault(); + } + if (!response.IsSuccessStatusCode) { @@ -104,7 +110,7 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) if (deserializedError == null) { // in this case, the response body was not parseable JSON - throw new ShipEngineException("Unexpected HTTP status", responseMessage: response); + throw new ShipEngineException("Unexpected HTTP status", requestID: requestId, responseMessage: response); } var error = deserializedError.Errors?.FirstOrDefault(e => e.Message != null); @@ -114,7 +120,7 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) error?.ErrorSource ?? ErrorSource.Shipengine, error?.ErrorType ?? ErrorType.System, error?.ErrorCode ?? ErrorCode.Unspecified, - deserializedError.RequestId, + deserializedError.RequestId ?? requestId, response ); } @@ -126,7 +132,7 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) } catch (JsonException) { - throw new ShipEngineException("Unable to parse response", responseMessage: response); + throw new ShipEngineException("Unable to parse response", requestID: requestId, responseMessage: response); } @@ -135,7 +141,7 @@ private async Task DeserializedResultOrThrow(HttpResponseMessage response) return result; } - throw new ShipEngineException(message: "Unexpected null response", responseMessage: response); + throw new ShipEngineException(message: "Unexpected null response", requestID: requestId, responseMessage: response); }