From 334323f3dffdd0b6d102a5c9bec996140e63a9f5 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 17 Oct 2024 18:17:31 +0200 Subject: [PATCH] Feature: option to retrieve original json body if parse exception occurred (#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 <153528055+l-trotta@users.noreply.github.com> Co-authored-by: Sylvain Wallez --- .../release-notes/release-highlights.asciidoc | 20 +++++ .../transport/DefaultTransportOptions.java | 27 +++++- .../transport/ElasticsearchTransportBase.java | 7 +- .../clients/transport/TransportOptions.java | 14 +++ .../http/RepeatableBodyResponse.java | 84 ++++++++++++++++++ .../rest_client/RestClientHttpClient.java | 7 +- .../rest_client/RestClientOptions.java | 22 ++++- .../documentation/DocTestsTransport.java | 5 ++ .../clients/transport/TransportTest.java | 86 ++++++++++++++++++- .../rest_client/RestClientOptionsTest.java | 2 +- 10 files changed, 262 insertions(+), 12 deletions(-) create mode 100644 java-client/src/main/java/co/elastic/clients/transport/http/RepeatableBodyResponse.java diff --git a/docs/release-notes/release-highlights.asciidoc b/docs/release-notes/release-highlights.asciidoc index f76a6c1c8..49ee99ed8 100644 --- a/docs/release-notes/release-highlights.asciidoc +++ b/docs/release-notes/release-highlights.asciidoc @@ -8,6 +8,26 @@ For a list of detailed changes, including bug fixes, please see the https://gith [discrete] ==== Version 8.16 * `ElasticsearchClient` is now `Closeable`. Closing a client object also closes the underlying transport - https://github.com/elastic/elasticsearch-java/pull/851[#851] +* Added option to make the response body available in case of deserialization error- https://github.com/elastic/elasticsearch-java/pull/886[#886]. + +** While it has always been possible to set the log level to `trace` and have the client print both the json bodies of the requests and responses, it's often not the best solution because of the large amount of information printed. +** To enable the feature: + + RestClientOptions options = new RestClientOptions(RequestOptions.DEFAULT, true); + ElasticsearchTransport transport = new RestClientTransport(restClient, new JacksonJsonpMapper(), options); + ElasticsearchClient esClientWithOptions = new ElasticsearchClient(transport); + +** To retrieve the original body from the TransportException that gets thrown in case of deserialization errors: + + try{ + // some code that returns faulty json + } + catch (TransportException ex){ + try (RepeatableBodyResponse repeatableResponse = (RepeatableBodyResponse) ex.response()) { + BinaryData body = repeatableResponse.body(); + } + } + [discrete] ==== Version 8.15 diff --git a/java-client/src/main/java/co/elastic/clients/transport/DefaultTransportOptions.java b/java-client/src/main/java/co/elastic/clients/transport/DefaultTransportOptions.java index f199705c1..cdc35639e 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/DefaultTransportOptions.java +++ b/java-client/src/main/java/co/elastic/clients/transport/DefaultTransportOptions.java @@ -37,6 +37,7 @@ public class DefaultTransportOptions implements TransportOptions { private final HeaderMap headers; private final Map parameters; private final Function, Boolean> onWarnings; + private boolean keepResponseBodyOnException; public static final DefaultTransportOptions EMPTY = new DefaultTransportOptions(); @@ -44,6 +45,16 @@ public DefaultTransportOptions() { this(new HeaderMap(), Collections.emptyMap(), null); } + public DefaultTransportOptions( + @Nullable HeaderMap headers, + @Nullable Map parameters, + @Nullable Function, Boolean> onWarnings, + boolean keepResponseBodyOnException + ) { + this(headers,parameters,onWarnings); + this.keepResponseBodyOnException = keepResponseBodyOnException; + } + public DefaultTransportOptions( @Nullable HeaderMap headers, @Nullable Map parameters, @@ -53,10 +64,11 @@ public DefaultTransportOptions( this.parameters = (parameters == null || parameters.isEmpty()) ? Collections.emptyMap() : Collections.unmodifiableMap(parameters); this.onWarnings = onWarnings; + this.keepResponseBodyOnException = false; } protected DefaultTransportOptions(AbstractBuilder builder) { - this(builder.headers, builder.parameters, builder.onWarnings); + this(builder.headers, builder.parameters, builder.onWarnings, builder.keepResponseBodyOnException); } public static DefaultTransportOptions of(@Nullable TransportOptions options) { @@ -88,6 +100,11 @@ public Function, Boolean> onWarnings() { return onWarnings; } + @Override + public boolean keepResponseBodyOnException() { + return keepResponseBodyOnException; + } + @Override public Builder toBuilder() { return new Builder(this); @@ -111,6 +128,7 @@ public abstract static class AbstractBuilder parameters; private Function, Boolean> onWarnings; + private boolean keepResponseBodyOnException; public AbstractBuilder() { } @@ -119,10 +137,17 @@ public AbstractBuilder(DefaultTransportOptions options) { this.headers = new HeaderMap(options.headers); this.parameters = copyOrNull(options.parameters); this.onWarnings = options.onWarnings; + this.keepResponseBodyOnException = options.keepResponseBodyOnException; } protected abstract BuilderT self(); + @Override + public BuilderT keepResponseBodyOnException(boolean value) { + this.keepResponseBodyOnException = value; + return self(); + } + @Override public BuilderT addHeader(String name, String value) { if (name.equalsIgnoreCase(HeaderMap.CLIENT_META)) { diff --git a/java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java b/java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java index 7d1e98608..1d422eb66 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java +++ b/java-client/src/main/java/co/elastic/clients/transport/ElasticsearchTransportBase.java @@ -29,14 +29,15 @@ import co.elastic.clients.transport.endpoints.BooleanEndpoint; import co.elastic.clients.transport.endpoints.BooleanResponse; import co.elastic.clients.transport.http.HeaderMap; +import co.elastic.clients.transport.http.RepeatableBodyResponse; import co.elastic.clients.transport.http.TransportHttpClient; import co.elastic.clients.transport.instrumentation.Instrumentation; import co.elastic.clients.transport.instrumentation.NoopInstrumentation; import co.elastic.clients.transport.instrumentation.OpenTelemetryForElasticsearch; +import co.elastic.clients.util.ByteArrayBinaryData; import co.elastic.clients.util.LanguageRuntimeVersions; import co.elastic.clients.util.ApiTypeHelper; import co.elastic.clients.util.BinaryData; -import co.elastic.clients.util.ByteArrayBinaryData; import co.elastic.clients.util.ContentType; import co.elastic.clients.util.MissingRequiredPropertyException; import co.elastic.clients.util.NoCopyByteArrayOutputStream; @@ -306,6 +307,9 @@ private ResponseT getApiResponse( int statusCode = clientResp.statusCode(); + if(options().keepResponseBodyOnException()){ + clientResp = RepeatableBodyResponse.of(clientResp); + } try { if (statusCode == 200) { checkProductHeader(clientResp, endpoint); @@ -377,6 +381,7 @@ private ResponseT decodeTransportResponse( ) throws IOException { if (endpoint instanceof JsonEndpoint) { + @SuppressWarnings("unchecked") JsonEndpoint jsonEndpoint = (JsonEndpoint) endpoint; // Successful response diff --git a/java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java b/java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java index d6c41f490..9cbbdd40d 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java +++ b/java-client/src/main/java/co/elastic/clients/transport/TransportOptions.java @@ -38,6 +38,13 @@ public interface TransportOptions { Function, Boolean> onWarnings(); + /** + * If {@code true}, the response body in {@code TransportException.response().body()} is guaranteed to be + * replayable (i.e. buffered), even if the original response was streamed. This allows inspecting the + * response body in case of error. + */ + boolean keepResponseBodyOnException(); + Builder toBuilder(); default TransportOptions with(Consumer fn) { @@ -59,5 +66,12 @@ interface Builder extends ObjectBuilder { Builder removeParameter(String name); Builder onWarnings(Function, Boolean> listener); + + /** + * Should the response body be buffered and made available in {@code TransportException.response().body()}? + * This setting guarantees that the response body is buffered for inspection if parsing fails, even if originally + * streamed by the http library. + */ + Builder keepResponseBodyOnException(boolean value); } } diff --git a/java-client/src/main/java/co/elastic/clients/transport/http/RepeatableBodyResponse.java b/java-client/src/main/java/co/elastic/clients/transport/http/RepeatableBodyResponse.java new file mode 100644 index 000000000..d578cdcec --- /dev/null +++ b/java-client/src/main/java/co/elastic/clients/transport/http/RepeatableBodyResponse.java @@ -0,0 +1,84 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package co.elastic.clients.transport.http; + +import co.elastic.clients.util.BinaryData; +import co.elastic.clients.util.ByteArrayBinaryData; + +import javax.annotation.Nullable; +import java.io.IOException; +import java.util.List; + +public class RepeatableBodyResponse implements TransportHttpClient.Response { + + private final TransportHttpClient.Response response; + private final BinaryData body; + + public static TransportHttpClient.Response of(TransportHttpClient.Response response) throws IOException { + BinaryData body = response.body(); + if (body == null || body.isRepeatable()) { + return response; + } + return new RepeatableBodyResponse(response); + } + + public RepeatableBodyResponse(TransportHttpClient.Response response) throws IOException { + this.response = response; + this.body = new ByteArrayBinaryData(response.body()); + } + + @Override + public TransportHttpClient.Node node() { + return response.node(); + } + + @Override + public int statusCode() { + return response.statusCode(); + } + + @Nullable + @Override + public String header(String name) { + return response.header(name); + } + + @Override + public List headers(String name) { + return response.headers(name); + } + + @Nullable + @Override + public BinaryData body() throws IOException { + return this.body; + } + + @Nullable + @Override + public Object originalResponse() { + return response.originalResponse(); + } + + @Override + public void close() throws IOException { + response.close(); + } +} diff --git a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java index 64b5aa08a..1bcc06b05 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java +++ b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientHttpClient.java @@ -85,7 +85,8 @@ public RestClientOptions createOptions(@Nullable TransportOptions options) { } @Override - public Response performRequest(String endpointId, @Nullable Node node, Request request, TransportOptions options) throws IOException { + public Response performRequest(String endpointId, @Nullable Node node, Request request, + TransportOptions options) throws IOException { RestClientOptions rcOptions = RestClientOptions.of(options); org.elasticsearch.client.Request restRequest = createRestRequest(request, rcOptions); org.elasticsearch.client.Response restResponse = restClient.performRequest(restRequest); @@ -103,7 +104,7 @@ public CompletableFuture performRequestAsync( try { RestClientOptions rcOptions = RestClientOptions.of(options); restRequest = createRestRequest(request, rcOptions); - } catch(Throwable thr) { + } catch (Throwable thr) { // Terminate early future.completeExceptionally(thr); return future; @@ -166,7 +167,7 @@ private org.elasticsearch.client.Request createRestRequest(Request request, Rest if (body != null) { ContentType ct = null; String ctStr; - if (( ctStr = requestHeaders.get(HeaderMap.CONTENT_TYPE)) != null) { + if ((ctStr = requestHeaders.get(HeaderMap.CONTENT_TYPE)) != null) { ct = ContentTypeCache.computeIfAbsent(ctStr, ContentType::parse); } clientReq.setEntity(new MultiBufferEntity(body, ct)); diff --git a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java index 9de6da07e..842a45c62 100644 --- a/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java +++ b/java-client/src/main/java/co/elastic/clients/transport/rest_client/RestClientOptions.java @@ -42,6 +42,8 @@ public class RestClientOptions implements TransportOptions { private final RequestOptions options; + boolean keepResponseBodyOnException; + @VisibleForTesting static final String CLIENT_META_VALUE = getClientMeta(); @VisibleForTesting @@ -63,7 +65,8 @@ static RestClientOptions of(@Nullable TransportOptions options) { return builder.build(); } - public RestClientOptions(RequestOptions options) { + public RestClientOptions(RequestOptions options, boolean keepResponseBodyOnException) { + this.keepResponseBodyOnException = keepResponseBodyOnException; this.options = addBuiltinHeaders(options.toBuilder()).build(); } @@ -99,6 +102,11 @@ public Function, Boolean> onWarnings() { return warnings -> options.getWarningsHandler().warningsShouldFailRequest(warnings); } + @Override + public boolean keepResponseBodyOnException() { + return this.keepResponseBodyOnException; + } + @Override public Builder toBuilder() { return new Builder(options.toBuilder()); @@ -108,6 +116,8 @@ public static class Builder implements TransportOptions.Builder { private RequestOptions.Builder builder; + private boolean keepResponseBodyOnException; + public Builder(RequestOptions.Builder builder) { this.builder = builder; } @@ -181,14 +191,20 @@ public TransportOptions.Builder onWarnings(Function, Boolean> liste return this; } + @Override + public TransportOptions.Builder keepResponseBodyOnException(boolean value) { + this.keepResponseBodyOnException = value; + return this; + } + @Override public RestClientOptions build() { - return new RestClientOptions(addBuiltinHeaders(builder).build()); + return new RestClientOptions(addBuiltinHeaders(builder).build(), keepResponseBodyOnException); } } static RestClientOptions initialOptions() { - return new RestClientOptions(SafeResponseConsumer.DEFAULT_REQUEST_OPTIONS); + return new RestClientOptions(SafeResponseConsumer.DEFAULT_REQUEST_OPTIONS, false); } private static RequestOptions.Builder addBuiltinHeaders(RequestOptions.Builder builder) { diff --git a/java-client/src/test/java/co/elastic/clients/documentation/DocTestsTransport.java b/java-client/src/test/java/co/elastic/clients/documentation/DocTestsTransport.java index 29ea6a629..6d1c41eb5 100644 --- a/java-client/src/test/java/co/elastic/clients/documentation/DocTestsTransport.java +++ b/java-client/src/test/java/co/elastic/clients/documentation/DocTestsTransport.java @@ -64,6 +64,11 @@ public Function, Boolean> onWarnings() { return null; } + @Override + public boolean keepResponseBodyOnException() { + return false; + } + @Override public Builder toBuilder() { return null; diff --git a/java-client/src/test/java/co/elastic/clients/transport/TransportTest.java b/java-client/src/test/java/co/elastic/clients/transport/TransportTest.java index b79b03893..d25466bbd 100644 --- a/java-client/src/test/java/co/elastic/clients/transport/TransportTest.java +++ b/java-client/src/test/java/co/elastic/clients/transport/TransportTest.java @@ -21,24 +21,34 @@ import co.elastic.clients.elasticsearch.ElasticsearchClient; import co.elastic.clients.json.jackson.JacksonJsonpMapper; +import co.elastic.clients.transport.http.RepeatableBodyResponse; +import co.elastic.clients.transport.rest_client.RestClientOptions; import co.elastic.clients.transport.rest_client.RestClientTransport; +import co.elastic.clients.util.BinaryData; import com.sun.net.httpserver.HttpServer; import org.apache.http.HttpHost; +import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.client.RestClient; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.io.BufferedReader; +import java.io.InputStreamReader; import java.io.OutputStream; import java.net.InetAddress; import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; +import java.util.Collections; + +import static co.elastic.clients.util.ContentType.APPLICATION_JSON; public class TransportTest extends Assertions { @Test public void testXMLResponse() throws Exception { - HttpServer httpServer = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + HttpServer httpServer = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), + 0), 0); httpServer.createContext("/_cat/indices", exchange -> { exchange.sendResponseHeaders(401, 0); @@ -56,7 +66,8 @@ public void testXMLResponse() throws Exception { .builder(new HttpHost(address.getHostString(), address.getPort(), "http")) .build(); - ElasticsearchClient esClient = new ElasticsearchClient(new RestClientTransport(restClient, new JacksonJsonpMapper())); + ElasticsearchClient esClient = new ElasticsearchClient(new RestClientTransport(restClient, + new JacksonJsonpMapper())); TransportException ex = Assertions.assertThrows( TransportException.class, @@ -69,7 +80,76 @@ public void testXMLResponse() throws Exception { assertEquals("es/cat.indices", ex.endpointId()); // Original response is transport-dependent - Response restClientResponse = (Response)ex.response().originalResponse(); + Response restClientResponse = (Response) ex.response().originalResponse(); assertEquals(401, restClientResponse.getStatusLine().getStatusCode()); } + + + @Test + public void testOriginalJsonBodyRetrievalException() throws Exception { + HttpServer httpServer = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), + 0), 0); + + httpServer.createContext("/_cat/indices", exchange -> { + exchange.getResponseHeaders().put("Content-Type", Collections.singletonList(APPLICATION_JSON)); + exchange.getResponseHeaders().put("X-Elastic-Product", Collections.singletonList("Elasticsearch" + )); + exchange.sendResponseHeaders(200, 0); + OutputStream out = exchange.getResponseBody(); + out.write( + "definitely not json".getBytes(StandardCharsets.UTF_8) + ); + out.close(); + }); + + httpServer.start(); + InetSocketAddress address = httpServer.getAddress(); + + RestClient restClient = RestClient + .builder(new HttpHost(address.getHostString(), address.getPort(), "http")) + .build(); + + // no transport options, response is not RepeatableBodyResponse, original body cannot be retrieved + ElasticsearchClient esClient = new ElasticsearchClient(new RestClientTransport(restClient, + new JacksonJsonpMapper())); + + TransportException ex = Assertions.assertThrows( + TransportException.class, + () -> esClient.cat().indices() + ); + + assertEquals(200, ex.statusCode()); + assertNotEquals(RepeatableBodyResponse.class, ex.response().getClass()); + + // setting transport option + RestClientOptions options = new RestClientOptions(RequestOptions.DEFAULT, true); + + ElasticsearchTransport transport = new RestClientTransport( + restClient, new JacksonJsonpMapper(), options); + + ElasticsearchClient esClientOptions = new ElasticsearchClient(transport); + + ex = Assertions.assertThrows( + TransportException.class, + () -> esClientOptions.cat().indices() + ); + + httpServer.stop(0); + + assertEquals(200, ex.statusCode()); + assertEquals(RepeatableBodyResponse.class, ex.response().getClass()); + + try (RepeatableBodyResponse repeatableResponse = (RepeatableBodyResponse) ex.response()){ + BinaryData body = repeatableResponse.body(); + StringBuilder sb = new StringBuilder(); + BufferedReader br = new BufferedReader(new InputStreamReader(body.asInputStream())); + String read; + + while ((read = br.readLine()) != null) { + sb.append(read); + } + br.close(); + assertEquals("definitely not json",sb.toString()); + } + } } diff --git a/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java b/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java index cd6558a4f..cf8995944 100644 --- a/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java +++ b/java-client/src/test/java/co/elastic/clients/transport/rest_client/RestClientOptionsTest.java @@ -192,7 +192,7 @@ void testRequestOptionsOverridingBuiltin() throws Exception { new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort(), "http") ).build(); - ElasticsearchTransport transport = newRestClientTransport(llrc, new SimpleJsonpMapper(), new RestClientOptions(options)); + ElasticsearchTransport transport = newRestClientTransport(llrc, new SimpleJsonpMapper(), new RestClientOptions(options,false)); ElasticsearchClient esClient = new ElasticsearchClient(transport); // Should not override client meta String id = checkHeaders(esClient);