From 0f7b4021cc878f34c6e174baee6ccb0c8b6f24e9 Mon Sep 17 00:00:00 2001 From: Steve Gordon Date: Thu, 10 Nov 2022 14:59:36 +0000 Subject: [PATCH] Refactoring responses (#53) --- Directory.Build.props | 4 +-- global.json | 2 +- .../Audit/Auditor.cs | 4 +-- .../Pipeline/DefaultRequestPipeline.cs | 14 ++++----- .../Pipeline/DefaultResponseBuilder.cs | 2 +- .../Configuration/ITransportConfiguration.cs | 2 +- .../Elastic.Transport.csproj | 2 ++ .../Elasticsearch/ElasticsearchResponse.cs | 19 ++++++------ .../ElasticsearchResponseBuilder.cs | 2 +- .../Products/ProductRegistration.cs | 2 +- .../Responses/HttpDetails/ApiCallDetails.cs | 6 ++-- .../Responses/ResponseFactory.cs | 29 ++----------------- .../Examples/ControllerIntegrationTests.cs | 2 +- .../Examples/EndpointIntegrationTests.cs | 2 +- .../VirtualClusterTests.cs | 6 ++-- 15 files changed, 38 insertions(+), 60 deletions(-) diff --git a/Directory.Build.props b/Directory.Build.props index 6d672ea..bcbb92c 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -1,4 +1,4 @@ - + @@ -10,7 +10,7 @@ latest true False - + false true $(SolutionRoot)\build\keys\keypair.snk diff --git a/global.json b/global.json index c88d990..4b23320 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { - "version": "6.0.202", + "version": "6.0.403", "rollForward": "latestFeature", "allowPrerelease": false } diff --git a/src/Elastic.Transport.VirtualizedCluster/Audit/Auditor.cs b/src/Elastic.Transport.VirtualizedCluster/Audit/Auditor.cs index 5a61bfd..b216891 100644 --- a/src/Elastic.Transport.VirtualizedCluster/Audit/Auditor.cs +++ b/src/Elastic.Transport.VirtualizedCluster/Audit/Auditor.cs @@ -128,7 +128,7 @@ public async Task TraceElasticsearchExceptionOnResponse(ClientCall call Action call = () => { Response = _cluster.ClientCall(callTrace?.RequestOverrides); }; call(); - if (Response.ApiCallDetails.Success) throw new Exception("Expected call to not be valid"); + if (Response.ApiCallDetails.HasSuccessfulStatusCode) throw new Exception("Expected call to not be valid"); if (Response.ApiCallDetails.OriginalException is not TransportException exception) throw new Exception("OriginalException on response is not expected TransportException"); @@ -142,7 +142,7 @@ public async Task TraceElasticsearchExceptionOnResponse(ClientCall call _clusterAsync.ClientThrows(false); Func callAsync = async () => { ResponseAsync = await _clusterAsync.ClientCallAsync(callTrace?.RequestOverrides).ConfigureAwait(false); }; await callAsync().ConfigureAwait(false); - if (Response.ApiCallDetails.Success) throw new Exception("Expected call to not be valid"); + if (Response.ApiCallDetails.HasSuccessfulStatusCode) throw new Exception("Expected call to not be valid"); exception = ResponseAsync.ApiCallDetails.OriginalException as TransportException; if (exception == null) throw new Exception("OriginalException on response is not expected TransportException"); assert(exception); diff --git a/src/Elastic.Transport/Components/Pipeline/DefaultRequestPipeline.cs b/src/Elastic.Transport/Components/Pipeline/DefaultRequestPipeline.cs index ef053f3..9aecadb 100644 --- a/src/Elastic.Transport/Components/Pipeline/DefaultRequestPipeline.cs +++ b/src/Elastic.Transport/Components/Pipeline/DefaultRequestPipeline.cs @@ -187,7 +187,7 @@ public override TResponse CallProductEndpoint(RequestData requestData apiCallDetails.AuditTrail = AuditTrail; ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails, response); - if (!response.ApiCallDetails.Success) audit.Event = requestData.OnFailureAuditEvent; + if (!response.ApiCallDetails.HasSuccessfulStatusCode) audit.Event = requestData.OnFailureAuditEvent; return response; } catch (Exception e) @@ -215,7 +215,7 @@ public override async Task CallProductEndpointAsync(Reques apiCallDetails.AuditTrail = AuditTrail; ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails, response); - if (!response.ApiCallDetails.Success) audit.Event = requestData.OnFailureAuditEvent; + if (!response.ApiCallDetails.HasSuccessfulStatusCode) audit.Event = requestData.OnFailureAuditEvent; return response; } catch (Exception e) @@ -230,7 +230,7 @@ public override async Task CallProductEndpointAsync(Reques public override TransportException CreateClientException( TResponse response, ApiCallDetails callDetails, RequestData data, List pipelineExceptions) { - if (callDetails?.Success ?? false) return null; + if (callDetails?.HasSuccessfulStatusCode ?? false) return null; var pipelineFailure = data.OnFailurePipelineFailure; var innerException = callDetails?.OriginalException; @@ -424,7 +424,7 @@ public override void Ping(Node node) d.EndState = response.ApiCallDetails; ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails); //ping should not silently accept bad but valid http responses - if (!response.ApiCallDetails.Success) + if (!response.ApiCallDetails.HasSuccessfulStatusCode) throw new PipelineException(pingData.OnFailurePipelineFailure, response.ApiCallDetails.OriginalException) { Response = response }; } catch (Exception e) @@ -454,7 +454,7 @@ public override async Task PingAsync(Node node, CancellationToken cancellationTo d.EndState = response.ApiCallDetails; ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails); //ping should not silently accept bad but valid http responses - if (!response.ApiCallDetails.Success) + if (!response.ApiCallDetails.HasSuccessfulStatusCode) throw new PipelineException(pingData.OnFailurePipelineFailure, response.ApiCallDetails.OriginalException) { Response = response }; } catch (Exception e) @@ -488,7 +488,7 @@ public override void Sniff() ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails); //sniff should not silently accept bad but valid http responses - if (!response.ApiCallDetails.Success) + if (!response.ApiCallDetails.HasSuccessfulStatusCode) throw new PipelineException(requestData.OnFailurePipelineFailure, response.ApiCallDetails.OriginalException) { Response = response }; _nodePool.Reseed(nodes); @@ -528,7 +528,7 @@ public override async Task SniffAsync(CancellationToken cancellationToken) ThrowBadAuthPipelineExceptionWhenNeeded(response.ApiCallDetails); //sniff should not silently accept bad but valid http responses - if (!response.ApiCallDetails.Success) + if (!response.ApiCallDetails.HasSuccessfulStatusCode) throw new PipelineException(requestData.OnFailurePipelineFailure, response.ApiCallDetails.OriginalException) { Response = response }; _nodePool.Reseed(nodes); diff --git a/src/Elastic.Transport/Components/Pipeline/DefaultResponseBuilder.cs b/src/Elastic.Transport/Components/Pipeline/DefaultResponseBuilder.cs index e097a66..318b4a3 100644 --- a/src/Elastic.Transport/Components/Pipeline/DefaultResponseBuilder.cs +++ b/src/Elastic.Transport/Components/Pipeline/DefaultResponseBuilder.cs @@ -131,7 +131,7 @@ IReadOnlyDictionary tcpStats var details = new ApiCallDetails { - Success = success, + HasSuccessfulStatusCode = success, OriginalException = exception, HttpStatusCode = statusCode, RequestBodyInBytes = requestData.PostData?.WrittenBytes, diff --git a/src/Elastic.Transport/Configuration/ITransportConfiguration.cs b/src/Elastic.Transport/Configuration/ITransportConfiguration.cs index 6ba5926..5a06c5a 100644 --- a/src/Elastic.Transport/Configuration/ITransportConfiguration.cs +++ b/src/Elastic.Transport/Configuration/ITransportConfiguration.cs @@ -247,7 +247,7 @@ public interface ITransportConfiguration : IDisposable UserAgent UserAgent { get; } /// - /// Allow you to override the status code inspection that sets + /// Allow you to override the status code inspection that sets /// /// Defaults to validating the statusCode is greater or equal to 200 and less then 300 /// diff --git a/src/Elastic.Transport/Elastic.Transport.csproj b/src/Elastic.Transport/Elastic.Transport.csproj index bfb0c44..10d4164 100644 --- a/src/Elastic.Transport/Elastic.Transport.csproj +++ b/src/Elastic.Transport/Elastic.Transport.csproj @@ -21,6 +21,8 @@ + + diff --git a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponse.cs index 6cac0a2..d4e1c16 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponse.cs @@ -34,17 +34,16 @@ public IEnumerable ElasticsearchWarnings /// /// /// - /// [JsonIgnore] public string DebugInformation { get { var sb = new StringBuilder(); - sb.Append($"{(!IsValid ? "Inv" : "V")}alid Elastic.Clients.Elasticsearch response built from a "); + sb.Append($"{(!IsValidResponse ? "Inv" : "V")}alid Elasticsearch response built from a "); sb.AppendLine(ApiCallDetails?.ToString().ToCamelCase() ?? "null ApiCall which is highly exceptional, please open a bug if you see this"); - if (!IsValid) + if (!IsValidResponse) DebugIsValid(sb); if (ApiCallDetails.ParsedHeaders is not null && ApiCallDetails.ParsedHeaders.TryGetValue("warning", out var warnings)) @@ -57,26 +56,26 @@ public string DebugInformation if (ApiCallDetails != null) Diagnostics.ResponseStatics.DebugInformationBuilder(ApiCallDetails, sb); + return sb.ToString(); } } /// - /// + /// Shortcut to test if the response is considered to be successful. /// - /// + /// A indicating success or failure. [JsonIgnore] - public virtual bool IsValid + public virtual bool IsValidResponse { get { var statusCode = ApiCallDetails?.HttpStatusCode; - // TODO - Review this on a request by reqeust basis if (statusCode == 404) return false; - return (ApiCallDetails?.Success ?? false) && (!ServerError?.HasError() ?? true); + return (ApiCallDetails?.HasSuccessfulStatusCode ?? false) && (!ElasticsearchServerError?.HasError() ?? true); } } @@ -84,7 +83,7 @@ public virtual bool IsValid /// /// [JsonIgnore] - public ElasticsearchServerError ServerError { get; internal set; } + public ElasticsearchServerError ElasticsearchServerError { get; internal set; } /// /// @@ -112,5 +111,5 @@ protected virtual void DebugIsValid(StringBuilder sb) { } /// /// public override string ToString() => - $"{(!IsValid ? "Inv" : "V")}alid Elastic.Clients.Elasticsearch response built from a {ApiCallDetails?.ToString().ToCamelCase()}"; + $"{(!IsValidResponse ? "Inv" : "V")}alid Elastic.Clients.Elasticsearch response built from a {ApiCallDetails?.ToString().ToCamelCase()}"; } diff --git a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponseBuilder.cs b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponseBuilder.cs index f155fb5..77fc157 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponseBuilder.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponseBuilder.cs @@ -14,7 +14,7 @@ protected override void SetErrorOnResponse(TResponse response, Elasti { if (response is ElasticsearchResponse elasticResponse) { - elasticResponse.ServerError = error; + elasticResponse.ElasticsearchServerError = error; } } diff --git a/src/Elastic.Transport/Products/ProductRegistration.cs b/src/Elastic.Transport/Products/ProductRegistration.cs index 27e397f..4ba366a 100644 --- a/src/Elastic.Transport/Products/ProductRegistration.cs +++ b/src/Elastic.Transport/Products/ProductRegistration.cs @@ -92,7 +92,7 @@ public abstract RequestData CreateSniffRequestData(Node node, IRequestConfigurat /// /// Used by to determine if it needs to return true or false for - /// + /// /// public abstract bool HttpStatusCodeClassifier(HttpMethod method, int statusCode); diff --git a/src/Elastic.Transport/Responses/HttpDetails/ApiCallDetails.cs b/src/Elastic.Transport/Responses/HttpDetails/ApiCallDetails.cs index 09d7394..0977582 100644 --- a/src/Elastic.Transport/Responses/HttpDetails/ApiCallDetails.cs +++ b/src/Elastic.Transport/Responses/HttpDetails/ApiCallDetails.cs @@ -87,13 +87,13 @@ public string DebugInformation /// /// /// - public bool Success { get; internal set; } + public bool HasSuccessfulStatusCode { get; internal set; } /// /// /// internal bool SuccessOrKnownError => - Success || HttpStatusCode >= 400 + HasSuccessfulStatusCode || HttpStatusCode >= 400 && HttpStatusCode < 599 && HttpStatusCode != 504 //Gateway timeout needs to be retried && HttpStatusCode != 503 //service unavailable needs to be retried @@ -134,5 +134,5 @@ public bool TryGetHeader(string key, out IEnumerable headerValues) => /// A string summarising the API call. /// public override string ToString() => - $"{(Success ? "S" : "Uns")}uccessful ({HttpStatusCode}) low level call on {HttpMethod.GetStringValue()}: {(Uri is not null ? Uri.PathAndQuery: "UNKNOWN URI")}"; + $"{(HasSuccessfulStatusCode ? "S" : "Uns")}uccessful ({HttpStatusCode}) low level call on {HttpMethod.GetStringValue()}: {(Uri is not null ? Uri.PathAndQuery: "UNKNOWN URI")}"; } diff --git a/src/Elastic.Transport/Responses/ResponseFactory.cs b/src/Elastic.Transport/Responses/ResponseFactory.cs index 20d6909..ecb1ccd 100644 --- a/src/Elastic.Transport/Responses/ResponseFactory.cs +++ b/src/Elastic.Transport/Responses/ResponseFactory.cs @@ -7,39 +7,16 @@ namespace Elastic.Transport; /// /// TODO /// -public static class ResponseFactory +public static class TestableResponseFactory { /// /// /// /// /// - public static T CreateResponse(T response, int statusCode) where T : TransportResponse => - CreateResponse(response, statusCode, true); - - /// - /// - /// - /// - /// - /// - /// - /// - public static T CreateResponse(T response, int statusCode, bool success) where T : TransportResponse => - CreateResponse(response, statusCode, HttpMethod.GET, success); - - /// - /// - /// - /// - /// - /// - /// - /// - /// - public static T CreateResponse(T response, int statusCode, HttpMethod httpMethod, bool success) where T : TransportResponse + public static T CreateResponse(T response, int httpStatusCode) where T : TransportResponse { - var apiCallDetails = new ApiCallDetails { HttpStatusCode = statusCode, Success = success, HttpMethod = httpMethod }; + var apiCallDetails = new ApiCallDetails { HttpStatusCode = httpStatusCode }; return CreateResponse(response, apiCallDetails); } diff --git a/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/ControllerIntegrationTests.cs b/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/ControllerIntegrationTests.cs index 903731e..22b37bf 100644 --- a/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/ControllerIntegrationTests.cs +++ b/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/ControllerIntegrationTests.cs @@ -27,7 +27,7 @@ public ControllerIntegrationTests(TransportTestServer instance) : base(instance) public async Task CanCallIntoController() { var response = await Transport.GetAsync("/dummy/20"); - response.ApiCallDetails.Success.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation); + response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation); } } diff --git a/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/EndpointIntegrationTests.cs b/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/EndpointIntegrationTests.cs index 693c8cb..ce6859b 100644 --- a/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/EndpointIntegrationTests.cs +++ b/tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/EndpointIntegrationTests.cs @@ -25,7 +25,7 @@ public EndpointIntegrationTests(TransportTestServer instance) : ba public async Task CanCallIntoEndpoint() { var response = await Transport.GetAsync(DummyStartup.Endpoint); - response.ApiCallDetails.Success.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation); + response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation); } } diff --git a/tests/Elastic.Transport.Tests/VirtualClusterTests.cs b/tests/Elastic.Transport.Tests/VirtualClusterTests.cs index 80a2694..b217d17 100644 --- a/tests/Elastic.Transport.Tests/VirtualClusterTests.cs +++ b/tests/Elastic.Transport.Tests/VirtualClusterTests.cs @@ -41,7 +41,7 @@ [Fact] public async Task ThrowsExceptionAfterDepleedingRules() { HealthyResponse, 9200, response => { - response.ApiCallDetails.Success.Should().BeTrue(); + response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue(); response.ApiCallDetails.HttpStatusCode.Should().Be(200); response.ApiCallDetails.DebugInformation.Should().Contain("x\":1"); } }, @@ -86,7 +86,7 @@ [Fact] public async Task RulesAreIgnoredAfterBeingExecuted() { HealthyResponse, 9200, response => { - response.ApiCallDetails.Success.Should().BeTrue(); + response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue(); response.ApiCallDetails.HttpStatusCode.Should().Be(200); response.ApiCallDetails.DebugInformation.Should().Contain("x\":1"); } }, @@ -95,7 +95,7 @@ [Fact] public async Task RulesAreIgnoredAfterBeingExecuted() { BadResponse, 9200, response => { - response.ApiCallDetails.Success.Should().BeFalse(); + response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeFalse(); response.ApiCallDetails.HttpStatusCode.Should().Be(500); response.ApiCallDetails.DebugInformation.Should().Contain("x\":2"); } },