Align ThrowExceptions decision with ProductRegistration.IsValidResponse#208
Merged
Conversation
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<MyDoc>(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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The throw decision in
RequestPipeline.CreateClientExceptionpreviously usedHasSuccessfulStatusCodeAndExpectedContentType, which doesn't account for product-specific semantics like Elasticsearch's "404 with no extracted server error is a valid missing-entity response". This PR moves the predicate into a new virtual onProductRegistrationso each product encodes its own semantics, and consults it from both the throw path and the per-responseIsValidResponseproperties — single source of truth.ElasticsearchProductRegistration.IsValidResponsenow carries the verbatim logic that lived in the staticElasticsearchResponseHelper.IsValidResponse(now removed); the response-levelIsValidResponseproperties on the six Elasticsearch response types read it throughApiCallDetails.TransportConfiguration.ProductRegistration. The defaultProductRegistration.IsValidResponsekeeps the previous transport behavior (HasSuccessfulStatusCodeAndExpectedContentType) so non-product calls are unchanged.Fixes #206. Pairs with the corresponding removal of
ElasticsearchClientProductRegistrationoverrides inelasticsearch-net.What changed
public virtual bool IsValidResponse(ApiCallDetails?)onProductRegistration. Default:details?.HasSuccessfulStatusCodeAndExpectedContentType ?? false.ElasticsearchProductRegistration.IsValidResponseoverride carries the verbatim 404-lenient logic moved from the helper.RequestPipeline.CreateClientExceptionconsults_productRegistration.IsValidResponse(callDetails)instead of the previous predicate.ElasticsearchResponse,ElasticsearchDynamicResponse,ElasticsearchJsonResponse,ElasticsearchPipeResponse,ElasticsearchStreamResponse,ElasticsearchStringResponse) read the predicate viaApiCallDetails.TransportConfiguration.ProductRegistration.ElasticsearchResponseHelper.IsValidResponse(ApiCallDetails?)deleted; the four other helpers stay.Behavior change
Under
ThrowExceptions=true:HttpStatusCodeClassifieroverride had been suppressing throws on real-error 404s).client.GetAsync<MyDoc>(idx, missing-id)returning{"_index":"…","_id":"…","found":false}— no longer throw and instead return a response withIsValidResponse=trueandFound=false.Under the default
ThrowExceptions=false, behavior is unchanged.Tests
tests/Elastic.Transport.Tests/Products/ProductRegistrationIsValidResponseTests.cs— nine focused tests coveringDefaultProductRegistrationandElasticsearchProductRegistrationacross 200/404/500 with/without extractedProductErrorand with/without expected content type.