Refactoring responses (#53)

Directory.Build.props

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
 
@@ -10,7 +10,7 @@
     <LangVersion>latest</LangVersion>
     <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
     <IsPackable>False</IsPackable>
-
+    <CheckEolTargetFramework>false</CheckEolTargetFramework>
     <SignAssembly>true</SignAssembly>
     <AssemblyOriginatorKeyFile>$(SolutionRoot)\build\keys\keypair.snk</AssemblyOriginatorKeyFile>
 

global.json

@@ -1,6 +1,6 @@
 {
   "sdk": {
-    "version": "6.0.202",
+    "version": "6.0.403",
     "rollForward": "latestFeature",
     "allowPrerelease": false
   }

src/Elastic.Transport.VirtualizedCluster/Audit/Auditor.cs

@@ -128,7 +128,7 @@ public async Task<Auditor> 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<Auditor> TraceElasticsearchExceptionOnResponse(ClientCall call
 		_clusterAsync.ClientThrows(false);
 		Func<Task> 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);

src/Elastic.Transport/Components/Pipeline/DefaultRequestPipeline.cs

@@ -187,7 +187,7 @@ public override TResponse CallProductEndpoint<TResponse>(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<TResponse> CallProductEndpointAsync<TResponse>(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<TResponse> CallProductEndpointAsync<TResponse>(Reques
 	public override TransportException CreateClientException<TResponse>(
 		TResponse response, ApiCallDetails callDetails, RequestData data, List<PipelineException> 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);

src/Elastic.Transport/Components/Pipeline/DefaultResponseBuilder.cs

@@ -131,7 +131,7 @@ IReadOnlyDictionary<TcpState, int> tcpStats
 
 		var details = new ApiCallDetails
 		{
-			Success = success,
+			HasSuccessfulStatusCode = success,
 			OriginalException = exception,
 			HttpStatusCode = statusCode,
 			RequestBodyInBytes = requestData.PostData?.WrittenBytes,

src/Elastic.Transport/Configuration/ITransportConfiguration.cs

@@ -247,7 +247,7 @@ public interface ITransportConfiguration : IDisposable
 	UserAgent UserAgent { get; }
 
 	/// <summary>
-	/// Allow you to override the status code inspection that sets <see cref="ApiCallDetails.Success"/>
+	/// Allow you to override the status code inspection that sets <see cref="ApiCallDetails.HasSuccessfulStatusCode"/>
 	/// <para>
 	/// Defaults to validating the statusCode is greater or equal to 200 and less then 300
 	/// </para>

src/Elastic.Transport/Elastic.Transport.csproj

@@ -21,6 +21,8 @@
   <ItemGroup>
     <InternalsVisibleTo Include="Elastic.Transport.VirtualizedCluster" Key="$(ExposedPublicKey)" />
     <InternalsVisibleTo Include="Elastic.Transport.Tests" Key="$(ExposedPublicKey)" />
+    <InternalsVisibleTo Include="Elastic.Transport.Benchmarks" Key="$(ExposedPublicKey)" />
+    <InternalsVisibleTo Include="Elastic.Transport.IntegrationTests" Key="$(ExposedPublicKey)" />    
   </ItemGroup>
 
   <ItemGroup>

src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponse.cs

@@ -34,17 +34,16 @@ public IEnumerable<string> ElasticsearchWarnings
 	/// <summary>
 	/// 
 	/// </summary>
-	/// <returns></returns>
 	[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,34 +56,34 @@ public string DebugInformation
 
 			if (ApiCallDetails != null)
 				Diagnostics.ResponseStatics.DebugInformationBuilder(ApiCallDetails, sb);
+
 			return sb.ToString();
 		}
 	}
 
 	/// <summary>
-	/// 
+	/// Shortcut to test if the response is considered to be successful.
 	/// </summary>
-	/// <returns></returns>
+	/// <returns>A <see cref="bool"/> indicating success or failure.</returns>
 	[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);
 		}
 	}
 
 	/// <summary>
 	/// 
 	/// </summary>
 	[JsonIgnore]
-	public ElasticsearchServerError ServerError { get; internal set; }
+	public ElasticsearchServerError ElasticsearchServerError { get; internal set; }
 
 	/// <summary>
 	/// 
@@ -112,5 +111,5 @@ protected virtual void DebugIsValid(StringBuilder sb) { }
 	/// </summary>
 	/// <returns></returns>
 	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()}";
 }

src/Elastic.Transport/Products/Elasticsearch/ElasticsearchResponseBuilder.cs

@@ -14,7 +14,7 @@ protected override void SetErrorOnResponse<TResponse>(TResponse response, Elasti
 	{
 		if (response is ElasticsearchResponse elasticResponse)
 		{
-			elasticResponse.ServerError = error;
+			elasticResponse.ElasticsearchServerError = error;
 		}
 	}
 

src/Elastic.Transport/Products/ProductRegistration.cs

@@ -92,7 +92,7 @@ public abstract RequestData CreateSniffRequestData(Node node, IRequestConfigurat
 
 	/// <summary>
 	/// Used by <see cref="ResponseBuilder"/> to determine if it needs to return true or false for
-	/// <see cref="ApiCallDetails.Success"/>
+	/// <see cref="ApiCallDetails.HasSuccessfulStatusCode"/>
 	/// </summary>
 	public abstract bool HttpStatusCodeClassifier(HttpMethod method, int statusCode);
 

src/Elastic.Transport/Responses/HttpDetails/ApiCallDetails.cs

@@ -87,13 +87,13 @@ public string DebugInformation
 	/// <summary>
 	/// 
 	/// </summary>
-	public bool Success { get; internal set; }
+	public bool HasSuccessfulStatusCode { get; internal set; }
 
 	/// <summary>
 	/// 
 	/// </summary>
 	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<string> headerValues) =>
 	/// A string summarising the API call.
 	/// </summary>
 	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")}";
 }

src/Elastic.Transport/Responses/ResponseFactory.cs

@@ -7,39 +7,16 @@ namespace Elastic.Transport;
 /// <summary>
 /// TODO
 /// </summary>
-public static class ResponseFactory
+public static class TestableResponseFactory
 {
 	/// <summary>
 	/// 
 	/// </summary>
 	/// <typeparam name="T"></typeparam>
 	/// <returns></returns>
-	public static T CreateResponse<T>(T response, int statusCode) where T : TransportResponse =>
-		CreateResponse<T>(response, statusCode, true);
-
-	/// <summary>
-	/// 
-	/// </summary>
-	/// <typeparam name="T"></typeparam>
-	/// <param name="response"></param>
-	/// <param name="statusCode"></param>
-	/// <param name="success"></param>
-	/// <returns></returns>
-	public static T CreateResponse<T>(T response, int statusCode, bool success) where T : TransportResponse =>
-		CreateResponse<T>(response, statusCode, HttpMethod.GET, success);
-
-	/// <summary>
-	/// 
-	/// </summary>
-	/// <typeparam name="T"></typeparam>
-	/// <param name="response"></param>
-	/// <param name="statusCode"></param>
-	/// <param name="httpMethod"></param>
-	/// <param name="success"></param>
-	/// <returns></returns>
-	public static T CreateResponse<T>(T response, int statusCode, HttpMethod httpMethod, bool success) where T : TransportResponse
+	public static T CreateResponse<T>(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<T>(response, apiCallDetails);
 	}
 

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<StringResponse>("/dummy/20");
-			response.ApiCallDetails.Success.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation);
+			response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation);
 		}
 	}
 

tests/Elastic.Transport.IntegrationTests/Plumbing/Examples/EndpointIntegrationTests.cs

@@ -25,7 +25,7 @@ public EndpointIntegrationTests(TransportTestServer<DummyStartup> instance) : ba
 		public async Task CanCallIntoEndpoint()
 		{
 			var response = await Transport.GetAsync<StringResponse>(DummyStartup.Endpoint);
-			response.ApiCallDetails.Success.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation);
+			response.ApiCallDetails.HasSuccessfulStatusCode.Should().BeTrue("{0}", response.ApiCallDetails.DebugInformation);
 		}
 	}
 

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");
 					} },