Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions docs/release-notes/release-highlights.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,24 @@ public class DefaultTransportOptions implements TransportOptions {
private final HeaderMap headers;
private final Map<String, String> parameters;
private final Function<List<String>, Boolean> onWarnings;
private boolean keepResponseBodyOnException;

public static final DefaultTransportOptions EMPTY = new DefaultTransportOptions();

public DefaultTransportOptions() {
this(new HeaderMap(), Collections.emptyMap(), null);
}

public DefaultTransportOptions(
@Nullable HeaderMap headers,
@Nullable Map<String, String> parameters,
@Nullable Function<List<String>, Boolean> onWarnings,
boolean keepResponseBodyOnException
) {
this(headers,parameters,onWarnings);
this.keepResponseBodyOnException = keepResponseBodyOnException;
}

public DefaultTransportOptions(
@Nullable HeaderMap headers,
@Nullable Map<String, String> parameters,
Expand All @@ -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) {
Expand Down Expand Up @@ -88,6 +100,11 @@ public Function<List<String>, Boolean> onWarnings() {
return onWarnings;
}

@Override
public boolean keepResponseBodyOnException() {
return keepResponseBodyOnException;
}

@Override
public Builder toBuilder() {
return new Builder(this);
Expand All @@ -111,6 +128,7 @@ public abstract static class AbstractBuilder<BuilderT extends AbstractBuilder<Bu
private HeaderMap headers;
private Map<String, String> parameters;
private Function<List<String>, Boolean> onWarnings;
private boolean keepResponseBodyOnException;

public AbstractBuilder() {
}
Expand All @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -306,6 +307,9 @@ private <ResponseT, ErrorT> ResponseT getApiResponse(

int statusCode = clientResp.statusCode();

if(options().keepResponseBodyOnException()){
clientResp = RepeatableBodyResponse.of(clientResp);
}
try {
if (statusCode == 200) {
checkProductHeader(clientResp, endpoint);
Expand Down Expand Up @@ -377,6 +381,7 @@ private <ResponseT> ResponseT decodeTransportResponse(
) throws IOException {

if (endpoint instanceof JsonEndpoint) {

@SuppressWarnings("unchecked")
JsonEndpoint<?, ResponseT, ?> jsonEndpoint = (JsonEndpoint<?, ResponseT, ?>) endpoint;
// Successful response
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ public interface TransportOptions {

Function<List<String>, 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();
l-trotta marked this conversation as resolved.
Show resolved Hide resolved

Builder toBuilder();

default TransportOptions with(Consumer<Builder> fn) {
Expand All @@ -59,5 +66,12 @@ interface Builder extends ObjectBuilder<TransportOptions> {
Builder removeParameter(String name);

Builder onWarnings(Function<List<String>, Boolean> listener);

l-trotta marked this conversation as resolved.
Show resolved Hide resolved
/**
* 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);
}
}
Original file line number Diff line number Diff line change
@@ -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<String> 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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -103,7 +104,7 @@ public CompletableFuture<Response> performRequestAsync(
try {
RestClientOptions rcOptions = RestClientOptions.of(options);
restRequest = createRestRequest(request, rcOptions);
} catch(Throwable thr) {
} catch (Throwable thr) {
// Terminate early
future.completeExceptionally(thr);
return future;
Expand Down Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public class RestClientOptions implements TransportOptions {

private final RequestOptions options;

boolean keepResponseBodyOnException;

@VisibleForTesting
static final String CLIENT_META_VALUE = getClientMeta();
@VisibleForTesting
Expand All @@ -63,7 +65,8 @@ static RestClientOptions of(@Nullable TransportOptions options) {
return builder.build();
}

public RestClientOptions(RequestOptions options) {
public RestClientOptions(RequestOptions options, boolean keepResponseBodyOnException) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.keepResponseBodyOnException = keepResponseBodyOnException;
this.options = addBuiltinHeaders(options.toBuilder()).build();
}

Expand Down Expand Up @@ -99,6 +102,11 @@ public Function<List<String>, Boolean> onWarnings() {
return warnings -> options.getWarningsHandler().warningsShouldFailRequest(warnings);
}

@Override
public boolean keepResponseBodyOnException() {
return this.keepResponseBodyOnException;
}

@Override
public Builder toBuilder() {
return new Builder(options.toBuilder());
Expand All @@ -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;
}
Expand Down Expand Up @@ -181,14 +191,20 @@ public TransportOptions.Builder onWarnings(Function<List<String>, Boolean> liste
return this;
}

@Override
public TransportOptions.Builder keepResponseBodyOnException(boolean value) {
this.keepResponseBodyOnException = value;
return this;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it cannot set a boolean flag in the builder, because the builder is in the RestClient code

}

@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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ public Function<List<String>, Boolean> onWarnings() {
return null;
}

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

@Override
public Builder toBuilder() {
return null;
Expand Down
Loading
Loading