Skip to content

Commit

Permalink
[8.x] Add a deprecation warning that the JSON format of non-detailed …
Browse files Browse the repository at this point in the history 
…errors is changing in v9 (elastic#116330) (elastic#114739)
  • Loading branch information
@thecoop
thecoop authored Nov 13, 2024
1 parent e1af8cc commit c441ada
Show file tree
Hide file tree
Showing 29 changed files with 108 additions and 57 deletions.
20 changes: 20 additions & 0 deletions docs/changelog/114739.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
pr: 114739
summary: Add a basic deprecation warning that the JSON format for non-detailed error responses is changing in v9
area: Infra/REST API
type: deprecation
issues: [89387]
deprecation:
title: The format of non-detailed error responses is changing in v9
area: REST API
details: |-
When an error occurs when processing a request, Elasticsearch returns information on that error in the REST response.
If `http:detailed_errors.enabled: false` is specified in node settings with the v8 REST API and below,
the format of this response changes significantly.
Starting with the v9 REST API, the JSON structure of responses with errors when the `http.detailed_errors.enabled: false` option is set
will be the same as when detailed errors are enabled (which is the default).
To keep using the existing format for non-detailed error responses, use the v8 REST API.
impact: |-
If you have set `http.detailed_errors.enabled: false` (the default is `true`)
the structure of JSON when any exceptions occur will change with the v9 REST API.
To keep using the existing format, use the v8 REST API.
notable: false
13 changes: 13 additions & 0 deletions server/src/main/java/org/elasticsearch/rest/RestResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Releasable;
Expand Down Expand Up @@ -43,6 +45,7 @@ public final class RestResponse implements Releasable {
static final String STATUS = "status";

private static final Logger SUPPRESSED_ERROR_LOGGER = LogManager.getLogger("rest.suppressed");
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(AbstractRestChannel.class);

private final RestStatus status;

Expand Down Expand Up @@ -142,6 +145,16 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws
if (params.paramAsBoolean("error_trace", false) && status != RestStatus.UNAUTHORIZED) {
params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
}

if (channel.detailedErrorsEnabled() == false) {
deprecationLogger.warn(
DeprecationCategory.API,
"http_detailed_errors",
"The JSON format of non-detailed errors will change in Elasticsearch 9.0 to match the JSON structure"
+ " used for detailed errors. To keep using the existing format, use the V8 REST API."
);
}

try (XContentBuilder builder = channel.newErrorBuilder()) {
build(builder, params, status, channel.detailedErrorsEnabled(), e);
this.content = BytesReference.bytes(builder);
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/synonyms/PutSynonymRuleActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void testEmptyRequestBody() throws Exception {
.withParams(Map.of("synonymsSet", "testSet", "synonymRuleId", "testRule"))
.build();

FakeRestChannel channel = new FakeRestChannel(request, false, 0);
FakeRestChannel channel = new FakeRestChannel(request, true, 0);
try (var threadPool = createThreadPool()) {
final var nodeClient = new NoOpNodeClient(threadPool);
expectThrows(IllegalArgumentException.class, () -> action.handleRequest(request, channel, nodeClient));
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/synonyms/PutSynonymsActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void testEmptyRequestBody() throws Exception {
.withParams(Map.of("synonymsSet", "test"))
.build();

FakeRestChannel channel = new FakeRestChannel(request, false, 0);
FakeRestChannel channel = new FakeRestChannel(request, true, 0);
try (var threadPool = createThreadPool()) {
final var nodeClient = new NoOpNodeClient(threadPool);
expectThrows(IllegalArgumentException.class, () -> action.handleRequest(request, channel, nodeClient));
Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/org/elasticsearch/http/AbstractHttpServerTransportTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th
final RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).build();
final RestControllerTests.AssertingChannel channel = new RestControllerTests.AssertingChannel(
fakeRequest,
false,
true,
RestStatus.BAD_REQUEST
);

Expand Down Expand Up @@ -361,7 +361,7 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th
Map<String, List<String>> restHeaders = new HashMap<>();
restHeaders.put(Task.TRACE_PARENT_HTTP_HEADER, Collections.singletonList(traceParentValue));
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).build();
RestControllerTests.AssertingChannel channel = new RestControllerTests.AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
RestControllerTests.AssertingChannel channel = new RestControllerTests.AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);

try (
AbstractHttpServerTransport transport = new AbstractHttpServerTransport(
Expand Down
18 changes: 9 additions & 9 deletions server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public List<Route> routes() {
params.put("consumed", randomAlphaOfLength(8));
params.put("unconsumed", randomAlphaOfLength(8));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> handler.handleRequest(request, channel, mockClient)
Expand Down Expand Up @@ -108,7 +108,7 @@ public List<Route> routes() {
params.put("unconsumed-first", randomAlphaOfLength(8));
params.put("unconsumed-second", randomAlphaOfLength(8));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> handler.handleRequest(request, channel, mockClient)
Expand Down Expand Up @@ -155,7 +155,7 @@ public List<Route> routes() {
params.put("very_close_to_parametre", randomAlphaOfLength(8));
params.put("very_far_from_every_consumed_parameter", randomAlphaOfLength(8));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> handler.handleRequest(request, channel, mockClient)
Expand Down Expand Up @@ -206,7 +206,7 @@ public List<Route> routes() {
params.put("consumed", randomAlphaOfLength(8));
params.put("response_param", randomAlphaOfLength(8));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
handler.handleRequest(request, channel, mockClient);
assertTrue(restChannelConsumer.executed);
assertTrue(restChannelConsumer.closed);
Expand Down Expand Up @@ -238,7 +238,7 @@ public List<Route> routes() {
params.put("human", null);
params.put("error_trace", randomFrom("true", "false", null));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
handler.handleRequest(request, channel, mockClient);
assertTrue(restChannelConsumer.executed);
assertTrue(restChannelConsumer.closed);
Expand Down Expand Up @@ -283,7 +283,7 @@ public List<Route> routes() {
params.put("size", randomAlphaOfLength(8));
params.put("time", randomAlphaOfLength(8));
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build();
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
RestChannel channel = new FakeRestChannel(request, true, 1);
handler.handleRequest(request, channel, mockClient);
assertTrue(restChannelConsumer.executed);
assertTrue(restChannelConsumer.closed);
Expand Down Expand Up @@ -314,7 +314,7 @@ public List<Route> routes() {
new BytesArray(builder.toString()),
XContentType.JSON
).build();
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
final RestChannel channel = new FakeRestChannel(request, true, 1);
handler.handleRequest(request, channel, mockClient);
assertTrue(restChannelConsumer.executed);
assertTrue(restChannelConsumer.closed);
Expand All @@ -341,7 +341,7 @@ public List<Route> routes() {
};

final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build();
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
final RestChannel channel = new FakeRestChannel(request, true, 1);
handler.handleRequest(request, channel, mockClient);
assertTrue(restChannelConsumer.executed);
assertTrue(restChannelConsumer.closed);
Expand Down Expand Up @@ -371,7 +371,7 @@ public List<Route> routes() {
new BytesArray(builder.toString()),
XContentType.JSON
).build();
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
final RestChannel channel = new FakeRestChannel(request, true, 1);
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> handler.handleRequest(request, channel, mockClient)
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/rest/ChunkedRestResponseBodyPartTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void testEncodesChunkedXContentCorrectly() throws IOException {
ToXContent.EMPTY_PARAMS,
new FakeRestChannel(
new FakeRestRequest.Builder(xContentRegistry()).withContent(BytesArray.EMPTY, randomXContent.type()).build(),
randomBoolean(),
true,
1
)
);
Expand Down
24 changes: 12 additions & 12 deletions server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public void testApplyProductSpecificResponseHeaders() {
final ThreadContext threadContext = client.threadPool().getThreadContext();
final RestController restController = new RestController(null, null, circuitBreakerService, usageService, telemetryProvider);
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).build();
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRequest, channel, threadContext);
// the rest controller relies on the caller to stash the context, so we should expect these values here as we didn't stash the
// context in this test
Expand All @@ -177,7 +177,7 @@ public void testRequestWithDisallowedMultiValuedHeader() {
restHeaders.put("header.1", Collections.singletonList("boo"));
restHeaders.put("header.2", List.of("foo", "bar"));
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).build();
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRequest, channel, threadContext);
assertTrue(channel.getSendResponseCalled());
}
Expand Down Expand Up @@ -208,7 +208,7 @@ public String getName() {
});
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.OK);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(
eq(1L),
Expand All @@ -232,7 +232,7 @@ public MethodHandlers next() {
return null;
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 400)));
}
Expand All @@ -254,7 +254,7 @@ public MethodHandlers next() {
}
});

AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 400)));
}
Expand All @@ -277,7 +277,7 @@ public String getName() {
}));
when(spyRestController.getAllHandlers(any(), eq(fakeRequest.rawPath()))).thenAnswer(x -> handlers.iterator());

AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.METHOD_NOT_ALLOWED);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.METHOD_NOT_ALLOWED);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 405)));
}
Expand All @@ -287,7 +287,7 @@ public void testDispatchBadRequestEmitsMetric() {
final RestController restController = new RestController(null, null, circuitBreakerService, usageService, telemetryProvider);
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).build();

AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
restController.dispatchBadRequest(channel, threadContext, new Exception());
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 400)));
}
Expand All @@ -311,7 +311,7 @@ public MethodHandlers next() {
return new MethodHandlers("/").addMethod(GET, RestApiVersion.current(), (request, channel, client) -> {});
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRequest, channel, threadContext);
verify(tracer).startTrace(
eq(threadContext),
Expand All @@ -337,7 +337,7 @@ public void testRequestWithDisallowedMultiValuedHeaderButSameValues() {
new RestResponse(RestStatus.OK, RestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)
)
);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.OK);
restController.dispatchRequest(fakeRequest, channel, threadContext);
assertTrue(channel.getSendResponseCalled());
}
Expand Down Expand Up @@ -801,7 +801,7 @@ public void testFavicon() {
final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withMethod(GET)
.withPath("/favicon.ico")
.build();
final AssertingChannel channel = new AssertingChannel(fakeRestRequest, false, RestStatus.OK);
final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
assertTrue(channel.getSendResponseCalled());
assertThat(channel.getRestResponse().contentType(), containsString("image/x-icon"));
Expand Down Expand Up @@ -1085,7 +1085,7 @@ public void testApiProtectionWithServerlessDisabled() {
List<String> accessiblePaths = List.of("/public", "/internal", "/hidden");
accessiblePaths.forEach(path -> {
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath(path).build();
AssertingChannel channel = new AssertingChannel(request, false, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(request, true, RestStatus.OK);
restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY));
});
}
Expand All @@ -1107,7 +1107,7 @@ public void testApiProtectionWithServerlessEnabledAsEndUser() {

final Consumer<List<String>> checkUnprotected = paths -> paths.forEach(path -> {
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath(path).build();
AssertingChannel channel = new AssertingChannel(request, false, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(request, true, RestStatus.OK);
restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY));
});
final Consumer<List<String>> checkProtected = paths -> paths.forEach(path -> {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void testUnsupportedMethodResponseHttpHeader() throws Exception {
RestRequest restRequest = fakeRestRequestBuilder.build();

// Send the request and verify the response status code
FakeRestChannel restChannel = new FakeRestChannel(restRequest, false, 1);
FakeRestChannel restChannel = new FakeRestChannel(restRequest, true, 1);
restController.dispatchRequest(restRequest, restChannel, new ThreadContext(Settings.EMPTY));
assertThat(restChannel.capturedResponse().status().getStatus(), is(405));

Expand Down
Loading

0 comments on commit c441ada

Please sign in to comment.