Feature: option to retrieve original json body if parse exception occurred #886

l-trotta · 2024-10-01T15:14:05Z

To help with issue debugging. The option can be set with:

RestClientOptions options = new RestClientOptions(RequestOptions.DEFAULT, true);

ElasticsearchTransport transport = new RestClientTransport(restClient, new JacksonJsonpMapper(), options);

ElasticsearchClient esClientOptions = new ElasticsearchClient(transport);

(the name retrieveOriginalJsonResponseOnException is clearly up to debate)

l-trotta · 2024-10-01T15:15:26Z

java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java

                    JsonParser parser = mapper.jsonProvider().createParser(content)
                ) {
                    response = responseParser.deserialize(parser, mapper);
                } catch (Exception e) {
-                    throw new TransportException(


custom exception to avoid changing too many lines around

swallez · 2024-10-04T09:43:32Z

java-client/src/test/java/co/elastic/clients/documentation/DocTestsTransport.java

@@ -64,6 +64,11 @@ public Function<List<String>, Boolean> onWarnings() {
            return null;
        }

+        @Override
+        public boolean retrieveOriginalJsonResponseOnException() {


Rename it to something shorter like keepResponseBodyOnException()? With a javadoc saying something like "Should the response body always be buffered and made available in TransportException.response.body()?"

swallez · 2024-10-04T10:03:25Z

java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java

+
+            // if the option to print the original body has been set, the body has to be
+            // copied first to another stream to be read again by the exception class
+            if(options().retrieveOriginalJsonResponseOnException()) {


We already have something similar for error responses:

elasticsearch-java/java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java

Lines 336 to 339 in 5d466ad

// We may have to replay it.

if (!entity.isRepeatable()) {

entity = new ByteArrayBinaryData(entity);

}

We could do the same here. Or even factorize it in a new method BinaryData ensureRepeatable() in BinaryData

swallez · 2024-10-04T10:08:47Z

java-client/src/main/java/co/elastic/clients/transport/TransportBodyResponseException.java

+     * The original response body, before json deserialization.
+     */
+    @Nullable
+    public String originalBody() {


Having a different kind of exception depending on the "capture body" flag is a bit confusing.

Another approach would be to add a new implementation of TransportHttpClient.Response, e.g. RepeatableBodyResponse that would wrap another response to ensure its body (if any) is repeatable, and delegate everything else to the wrapped response. This could even replace the entity = new ByteArrayBinaryData(entity) suggestion above.

swallez · 2024-10-04T10:11:53Z

java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java

@@ -181,6 +192,11 @@ public TransportOptions.Builder onWarnings(Function<List<String>, Boolean> liste
            return this;
        }

+        @Override
+        public TransportOptions.Builder retrieveOriginalJsonResponseOnException(boolean value) {
+            return this;


Why is it empty? It should set a boolean flag in the builder, and setRetrieveOriginalJsonResponseOnException on RestClientOptions should be removed to keep it immutable.

l-trotta · 2024-10-08T11:01:57Z

@swallez entire PRs has been refactored based on our latest discussion

swallez · 2024-10-15T15:55:45Z

java-client/src/main/java/co/elastic/clients/transport/DefaultTransportOptions.java

@@ -88,6 +88,11 @@ public Function<List<String>, Boolean> onWarnings() {
        return onWarnings;
    }

+    @Override
+    public boolean keepResponseBodyOnException() {
+        return false;


We should add a new field here, that is initialized from the builder's keepResponseBodyOnException.

this is the default one, shouldn't it be false by default?

java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java

swallez · 2024-10-15T16:15:59Z

java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java

+            return repeatableBody;
+        }
+
+        public String getOriginalBodyAsString() throws IOException {


Looks like this is used only in tests. Let's not add in the public API some things that aren't required to use the library.

I actually wanted to provide this as a utility method to let users get the string directly

swallez · 2024-10-15T16:18:13Z

java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java

@@ -112,6 +119,9 @@ public CompletableFuture<Response> performRequestAsync(
        future.cancellable = restClient.performRequestAsync(restRequest, new ResponseListener() {
            @Override
            public void onSuccess(org.elasticsearch.client.Response response) {
+                if (options.keepResponseBodyOnException()) {


This should be moved to ElasticsearchTransportBase, which will allow it to be independent of the actual HttpClient implementation, and also RepeatableBodyResponse which will then implement TransportHttpClient.Response.

swallez · 2024-10-15T16:27:56Z

java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java

@@ -63,7 +65,8 @@ static RestClientOptions of(@Nullable TransportOptions options) {
        return builder.build();
    }

-    public RestClientOptions(RequestOptions options) {
+    public RestClientOptions(RequestOptions options, boolean keepResponseBodyOnException) {


We should keep the existing constructor without the additional parameter to avoid a breaking change (and default the additional parameter to false).

Also, to be future proof if we add more options in the features, what about changing this constructor to RestClientOptions(RequestOptions.Builder builder) so that we can add more properties without impacting the constructor's signature?

I think the future proof constructor will make more sense when we'll have control over RequestOptions in the low level client!

…rtOptions.java Co-authored-by: Sylvain Wallez <[email protected]>

swallez

LGTM! We should also mention it in the release highlights and the troubleshooting docs.

…urred (#886) * base working impl * twr, remove unused constructor * unit test * license * complete refactor * reverting old changes * codestyle * leave ElasticsearchTransportBase untouched * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java Co-authored-by: Sylvain Wallez <[email protected]> * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java Co-authored-by: Sylvain Wallez <[email protected]> * custom generic response, checking in transport base * license header * asciidocs --------- Co-authored-by: Sylvain Wallez <[email protected]>

…urred (#886) (#898) * base working impl * twr, remove unused constructor * unit test * license * complete refactor * reverting old changes * codestyle * leave ElasticsearchTransportBase untouched * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java * custom generic response, checking in transport base * license header * asciidocs --------- Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Sylvain Wallez <[email protected]>

…urred (#886) (#897) * base working impl * twr, remove unused constructor * unit test * license * complete refactor * reverting old changes * codestyle * leave ElasticsearchTransportBase untouched * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java * Update java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java * custom generic response, checking in transport base * license header * asciidocs --------- Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Sylvain Wallez <[email protected]>

l-trotta added 3 commits October 1, 2024 15:54

base working impl

51976d1

twr, remove unused constructor

35c07e9

unit test

9835bc4

l-trotta requested a review from swallez October 1, 2024 15:14

l-trotta commented Oct 1, 2024

View reviewed changes

license

0e93afd

swallez requested changes Oct 4, 2024

View reviewed changes

swallez reviewed Oct 4, 2024

View reviewed changes

l-trotta added 2 commits October 8, 2024 12:52

complete refactor

d003cc6

reverting old changes

5bcf666

l-trotta and others added 4 commits October 8, 2024 13:04

codestyle

9832695

Merge branch 'main' into json-deser-exc

890a76b

leave ElasticsearchTransportBase untouched

cdeb192

Merge remote-tracking branch 'origin/json-deser-exc' into json-deser-exc

e7eee8a

swallez requested changes Oct 15, 2024

View reviewed changes

l-trotta and others added 4 commits October 16, 2024 12:42

Update java-client/src/main/java/co/elastic/clients/transport/Transpo…

30ee626

…rtOptions.java Co-authored-by: Sylvain Wallez <[email protected]>

Update java-client/src/main/java/co/elastic/clients/transport/Transpo…

435d2ef

…rtOptions.java Co-authored-by: Sylvain Wallez <[email protected]>

custom generic response, checking in transport base

0beeebb

license header

d93cd29

swallez approved these changes Oct 17, 2024

View reviewed changes

l-trotta added backport 8.16 backport 8.x labels Oct 17, 2024

l-trotta and others added 2 commits October 17, 2024 17:02

asciidocs

d416490

Merge branch 'main' into json-deser-exc

a7b422e

l-trotta merged commit d9cb8c5 into main Oct 17, 2024
7 checks passed

l-trotta deleted the json-deser-exc branch October 17, 2024 15:46

github-actions bot mentioned this pull request Oct 17, 2024

[Backport 8.x] Feature: option to retrieve original json body if parse exception occurred #897

Merged

github-actions bot mentioned this pull request Oct 17, 2024

[Backport 8.16] Feature: option to retrieve original json body if parse exception occurred #898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: option to retrieve original json body if parse exception occurred #886

Feature: option to retrieve original json body if parse exception occurred #886

l-trotta commented Oct 1, 2024 •

edited

Loading

l-trotta Oct 1, 2024

swallez Oct 4, 2024

swallez Oct 4, 2024

swallez Oct 4, 2024

swallez Oct 4, 2024

l-trotta commented Oct 8, 2024

swallez Oct 15, 2024

l-trotta Oct 16, 2024

swallez Oct 15, 2024

l-trotta Oct 16, 2024

swallez Oct 15, 2024

swallez Oct 15, 2024

l-trotta Oct 16, 2024

swallez left a comment

	// We may have to replay it.
	if (!entity.isRepeatable()) {
	entity = new ByteArrayBinaryData(entity);
	}

Feature: option to retrieve original json body if parse exception occurred #886

Feature: option to retrieve original json body if parse exception occurred #886

Conversation

l-trotta commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-trotta commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swallez left a comment

Choose a reason for hiding this comment

l-trotta commented Oct 1, 2024 •

edited

Loading