From cb660fa872d702a11f9ed88cf6cc39955f8ec3c9 Mon Sep 17 00:00:00 2001 From: Florian Bernd Date: Mon, 27 Apr 2026 14:57:50 +0200 Subject: [PATCH] Align ThrowExceptions decision with ProductRegistration.IsValidResponse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The throw decision in `RequestPipeline.CreateClientException` previously used `HasSuccessfulStatusCodeAndExpectedContentType`, which doesn't account for product-specific semantics like Elasticsearch's "404 with no extracted server error is a valid missing-entity response". Move the predicate into a new virtual method on `ProductRegistration` so each product encodes its own semantics, and consult it from both the throw path and the per-response `IsValidResponse` properties — single source of truth. `ElasticsearchProductRegistration.IsValidResponse` carries the verbatim logic that lived in the static `ElasticsearchResponseHelper.IsValidResponse` (now removed); the response-level `IsValidResponse` properties on the six Elasticsearch response types now read it through `ApiCallDetails.TransportConfiguration.ProductRegistration`. The default `ProductRegistration.IsValidResponse` keeps the previous transport behavior (`HasSuccessfulStatusCodeAndExpectedContentType`) so non-product calls are unchanged. Behavior change for users with `ThrowExceptions=true`: - Real-error 4xx/5xx still throw (unchanged for non-Elasticsearch users; newly correct for Elasticsearch users where the now-removed client- side `HttpStatusCodeClassifier` override had been suppressing throws on real-error 404s). - Valid 404s — e.g. `client.GetAsync(idx, missing-id)` returning `{"_index":"…","_id":"…","found":false}` — no longer throw and instead return a response with `IsValidResponse=true` and `Found=false`. Fixes elastic/elastic-transport-net#206. Pairs with elasticsearch-net's removal of `ElasticsearchClientProductRegistration` overrides. --- .../Components/Pipeline/RequestPipeline.cs | 2 +- .../ElasticsearchProductRegistration.cs | 26 +++++- .../Responses/ElasticsearchDynamicResponse.cs | 3 +- .../Responses/ElasticsearchJsonResponse.cs | 3 +- .../Responses/ElasticsearchPipeResponse.cs | 3 +- .../Responses/ElasticsearchResponse.cs | 2 +- .../Responses/ElasticsearchResponseHelper.cs | 18 ---- .../Responses/ElasticsearchStreamResponse.cs | 3 +- .../Responses/ElasticsearchStringResponse.cs | 3 +- .../Products/ProductRegistration.cs | 12 +++ ...ProductRegistrationIsValidResponseTests.cs | 85 +++++++++++++++++++ 11 files changed, 134 insertions(+), 26 deletions(-) create mode 100644 tests/Elastic.Transport.Tests/Products/ProductRegistrationIsValidResponseTests.cs diff --git a/src/Elastic.Transport/Components/Pipeline/RequestPipeline.cs b/src/Elastic.Transport/Components/Pipeline/RequestPipeline.cs index ce1fe389..fee7f1bf 100644 --- a/src/Elastic.Transport/Components/Pipeline/RequestPipeline.cs +++ b/src/Elastic.Transport/Components/Pipeline/RequestPipeline.cs @@ -206,7 +206,7 @@ private async ValueTask CallProductEndpointCoreAsync( ) where TResponse : TransportResponse, new() { - if (callDetails?.HasSuccessfulStatusCodeAndExpectedContentType ?? false) + if (_productRegistration.IsValidResponse(callDetails)) return null; var pipelineFailure = callDetails?.HttpStatusCode != null ? PipelineFailure.BadResponse : PipelineFailure.BadRequest; diff --git a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchProductRegistration.cs b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchProductRegistration.cs index ac9281b7..18d33a56 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchProductRegistration.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/ElasticsearchProductRegistration.cs @@ -111,11 +111,35 @@ public override bool NodePredicate(Node node) => /// We consider all status codes >= 200 and < 300 valid by default. /// Elasticsearch might return 404 for valid responses in some cases (e.g. `GET /my-index/_doc/missing-doc-id`) but also for actual error cases like /// missing endpoints, missing indices (e.g. `GET /missing-index/_mapping`), etc. - /// The 404 case is handled on a per-request basis (see for details). + /// The 404 case is handled on a per-request basis (see for details). /// public override bool HttpStatusCodeClassifier(HttpMethod method, int statusCode) => statusCode is >= 200 and < 300; + /// + /// + /// A response is valid when: + /// + /// The content-type matches what the caller asked for. + /// For 404 responses: there is no extracted Elasticsearch server error + /// (404s without an error body — e.g. GET /my-index/_doc/missing-doc-id — represent + /// a legitimate "missing entity" rather than a failure). + /// Otherwise: the status code is in the success range (or explicitly allowed via + /// ). + /// + /// + public override bool IsValidResponse(ApiCallDetails? details) + { + if (details is null || !details.HasExpectedContentType) + return false; + + var serverError = details.ProductError as ElasticsearchServerError; + if (details.HttpStatusCode is 404) + return !serverError?.HasError() ?? true; + + return details.HasSuccessfulStatusCode; + } + /// > public override bool TryGetServerErrorReason(TResponse response, out string? reason) { diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchDynamicResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchDynamicResponse.cs index 8e014536..5620772e 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchDynamicResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchDynamicResponse.cs @@ -24,7 +24,8 @@ public ElasticsearchDynamicResponse(DynamicDictionary dictionary) : base(diction public ElasticsearchServerError? ElasticsearchServerError => ElasticsearchResponseHelper.GetElasticsearchError(ApiCallDetails); /// - public bool IsValidResponse => ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + public bool IsValidResponse => + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// public IEnumerable ElasticsearchWarnings => ElasticsearchResponseHelper.GetElasticsearchWarnings(ApiCallDetails); diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchJsonResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchJsonResponse.cs index 13e42904..1149ef32 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchJsonResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchJsonResponse.cs @@ -25,7 +25,8 @@ public ElasticsearchJsonResponse(JsonNode node) : base(node) { } public ElasticsearchServerError? ElasticsearchServerError => ElasticsearchResponseHelper.GetElasticsearchError(ApiCallDetails); /// - public bool IsValidResponse => ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + public bool IsValidResponse => + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// public IEnumerable ElasticsearchWarnings => ElasticsearchResponseHelper.GetElasticsearchWarnings(ApiCallDetails); diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchPipeResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchPipeResponse.cs index e815e9c5..6bba6046 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchPipeResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchPipeResponse.cs @@ -35,7 +35,8 @@ public ElasticsearchPipeResponse(Stream responseStream, string? contentType) : b public ElasticsearchServerError? ElasticsearchServerError => ElasticsearchResponseHelper.GetElasticsearchError(ApiCallDetails); /// - public bool IsValidResponse => ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + public bool IsValidResponse => + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// public IEnumerable ElasticsearchWarnings => ElasticsearchResponseHelper.GetElasticsearchWarnings(ApiCallDetails); diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponse.cs index a777f6f3..757b15e4 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponse.cs @@ -27,7 +27,7 @@ public abstract class ElasticsearchResponse : TransportResponse, IElasticsearchR /// [JsonIgnore] public virtual bool IsValidResponse => - ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// [JsonIgnore] diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponseHelper.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponseHelper.cs index 3f14b98f..328e8c9f 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponseHelper.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchResponseHelper.cs @@ -15,24 +15,6 @@ namespace Elastic.Transport.Products.Elasticsearch; /// internal static class ElasticsearchResponseHelper { - public static bool IsValidResponse(ApiCallDetails? apiCallDetails) - { - if (apiCallDetails is null || !apiCallDetails.HasExpectedContentType) - return false; - - // Elasticsearch returns 404 for valid responses in some cases (e.g. `GET /my-index/_doc/missing-doc-id`) but also for actual error cases like - // missing endpoints, missing indices (e.g. `GET /missing-index/_mapping`), etc. - // We consider all status codes >= 200 and < 300 valid by default. For 404, we assume "invalid" and try to parse the Elasticsearch - // error response from the body. - // A 404 status code without an error body indicates a valid response. - - var serverError = GetElasticsearchError(apiCallDetails); - if (apiCallDetails.HttpStatusCode is 404) - return !serverError?.HasError() ?? true; - - return apiCallDetails.HasSuccessfulStatusCode; - } - public static ElasticsearchServerError? GetElasticsearchError(ApiCallDetails? apiCallDetails) => apiCallDetails?.ProductError as ElasticsearchServerError; diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStreamResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStreamResponse.cs index 71a11a19..63bad229 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStreamResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStreamResponse.cs @@ -33,7 +33,8 @@ public ElasticsearchStreamResponse(Stream body, string? contentType) : base(body public ElasticsearchServerError? ElasticsearchServerError => ElasticsearchResponseHelper.GetElasticsearchError(ApiCallDetails); /// - public bool IsValidResponse => ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + public bool IsValidResponse => + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// public IEnumerable ElasticsearchWarnings => ElasticsearchResponseHelper.GetElasticsearchWarnings(ApiCallDetails); diff --git a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStringResponse.cs b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStringResponse.cs index 6a6f79ae..4cfa35d1 100644 --- a/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStringResponse.cs +++ b/src/Elastic.Transport/Products/Elasticsearch/Responses/ElasticsearchStringResponse.cs @@ -24,7 +24,8 @@ public ElasticsearchStringResponse(string body) : base(body) { } public ElasticsearchServerError? ElasticsearchServerError => ElasticsearchResponseHelper.GetElasticsearchError(ApiCallDetails); /// - public bool IsValidResponse => ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails); + public bool IsValidResponse => + ApiCallDetails?.TransportConfiguration?.ProductRegistration?.IsValidResponse(ApiCallDetails) ?? false; /// public IEnumerable ElasticsearchWarnings => ElasticsearchResponseHelper.GetElasticsearchWarnings(ApiCallDetails); diff --git a/src/Elastic.Transport/Products/ProductRegistration.cs b/src/Elastic.Transport/Products/ProductRegistration.cs index 849881fc..ecbf48c0 100644 --- a/src/Elastic.Transport/Products/ProductRegistration.cs +++ b/src/Elastic.Transport/Products/ProductRegistration.cs @@ -162,4 +162,16 @@ public abstract class ProductRegistration /// A seekable stream containing the response body. /// An if one was successfully extracted; otherwise null. public virtual ErrorResponse? TryExtractError(BoundConfiguration boundConfiguration, Stream responseStream) => null; + + /// + /// Whether the response should be treated as valid for the caller — meaning no exception is + /// raised under , and the product-level + /// IsValidResponse reads true. + /// Default: a successful HTTP status code with the expected content type. Products override + /// to encode their own semantics (e.g. Elasticsearch treats a 404 with no extracted server error + /// as valid because it represents a missing entity rather than a true failure). + /// + /// The for the response. + public virtual bool IsValidResponse(ApiCallDetails? details) => + details?.HasSuccessfulStatusCodeAndExpectedContentType ?? false; } diff --git a/tests/Elastic.Transport.Tests/Products/ProductRegistrationIsValidResponseTests.cs b/tests/Elastic.Transport.Tests/Products/ProductRegistrationIsValidResponseTests.cs new file mode 100644 index 00000000..4b4722ec --- /dev/null +++ b/tests/Elastic.Transport.Tests/Products/ProductRegistrationIsValidResponseTests.cs @@ -0,0 +1,85 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information + +using Elastic.Transport.Products; +using Elastic.Transport.Products.Elasticsearch; +using FluentAssertions; +using Xunit; + +namespace Elastic.Transport.Tests.Products; + +public class ProductRegistrationIsValidResponseTests +{ + [Fact] + public void DefaultRegistrationReturnsFalseForNullDetails() => + DefaultProductRegistration.Default.IsValidResponse(null).Should().BeFalse(); + + [Fact] + public void DefaultRegistrationReturnsTrueForSuccessAndExpectedContentType() => + DefaultProductRegistration.Default + .IsValidResponse(MakeDetails(200, hasSuccess: true)) + .Should().BeTrue(); + + [Fact] + public void DefaultRegistrationReturnsFalseForNonSuccess() => + DefaultProductRegistration.Default + .IsValidResponse(MakeDetails(404, hasSuccess: false)) + .Should().BeFalse(); + + [Fact] + public void DefaultRegistrationReturnsFalseForUnexpectedContentType() + { + var details = MakeDetails(200, hasSuccess: true); + details.HasExpectedContentType = false; + DefaultProductRegistration.Default.IsValidResponse(details).Should().BeFalse(); + } + + [Fact] + public void ElasticsearchReturnsTrueFor404WithNoExtractedServerError() => + ElasticsearchProductRegistration.Default + .IsValidResponse(MakeDetails(404, hasSuccess: false)) + .Should().BeTrue(); + + [Fact] + public void ElasticsearchReturnsFalseFor404WithExtractedServerError() + { + var details = MakeDetails(404, hasSuccess: false); + details.ProductError = new ElasticsearchServerError( + new Error { Type = "index_not_found_exception", Reason = "no such index" }, + statusCode: 404); + ElasticsearchProductRegistration.Default.IsValidResponse(details).Should().BeFalse(); + } + + [Fact] + public void ElasticsearchReturnsFalseFor500WithExtractedServerError() + { + var details = MakeDetails(500, hasSuccess: false); + details.ProductError = new ElasticsearchServerError( + new Error { Type = "internal", Reason = "boom" }, + statusCode: 500); + ElasticsearchProductRegistration.Default.IsValidResponse(details).Should().BeFalse(); + } + + [Fact] + public void ElasticsearchReturnsTrueForSuccess() => + ElasticsearchProductRegistration.Default + .IsValidResponse(MakeDetails(200, hasSuccess: true)) + .Should().BeTrue(); + + [Fact] + public void ElasticsearchReturnsFalseFor404WithUnexpectedContentType() + { + var details = MakeDetails(404, hasSuccess: false); + details.HasExpectedContentType = false; + ElasticsearchProductRegistration.Default.IsValidResponse(details).Should().BeFalse(); + } + + private static ApiCallDetails MakeDetails(int statusCode, bool hasSuccess) => + new() + { + HttpStatusCode = statusCode, + HasSuccessfulStatusCode = hasSuccess, + HasExpectedContentType = true, + }; +}