Skip to content

Output a consistent format when generating error json #90529

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

Merged
merged 39 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b0c0682
Output a consistent format when generating error json
thecoop Sep 29, 2022
4cd9a4e
Update docs/changelog/90529.yaml
thecoop Sep 29, 2022
a731fa6
Update changelog with issue number
thecoop Sep 29, 2022
cec444a
Update RestResponseTests
thecoop Sep 29, 2022
f3d60c8
PR comments
thecoop Sep 30, 2022
6bd5e56
Merge remote-tracking branch 'upstream/main' into exception-json
thecoop Sep 30, 2022
744626c
spotless
thecoop Sep 30, 2022
45b9806
Update docs/changelog/90529.yaml
thecoop Sep 30, 2022
7532693
Update docs/changelog/90529.yaml
thecoop Sep 30, 2022
1e75fce
Update docs/changelog/90529.yaml
thecoop Sep 30, 2022
750eb7d
Update changelog with change details
thecoop Sep 30, 2022
93dc953
Update docs/changelog/90529.yaml
thecoop Oct 3, 2022
d293dcb
Match the method parameter types
thecoop Oct 3, 2022
309b076
Merge remote-tracking branch 'upstream/main' into exception-json
thecoop Nov 17, 2022
ab3041c
Update docs
thecoop Nov 17, 2022
f185846
Merge branch 'main' into exception-json
thecoop Nov 17, 2022
459acf4
Update docs/changelog/90529.yaml
thecoop Feb 8, 2023
32a9f9e
Merge branch 'main' into exception-json
thecoop Feb 10, 2023
02862e9
Revert "Update docs/changelog/90529.yaml"
thecoop Feb 10, 2023
14b55f9
Update docs/changelog/90529.yaml
thecoop Apr 26, 2023
dca6d31
Merge branch 'main' into exception-json
thecoop Oct 10, 2024
a2548ab
Revert "Update docs/changelog/90529.yaml"
thecoop Oct 10, 2024
8396ba3
Merge branch 'main' into exception-json
elasticmachine Oct 10, 2024
798be45
Merge remote-tracking branch 'upstream/main' into exception-json
thecoop Oct 18, 2024
6f253c4
Add V8 rest API formats for compatibility
thecoop Oct 18, 2024
c77abde
Merge branch 'main' into exception-json
thecoop Nov 6, 2024
93da694
Add a basic deprecation warning that the JSON format is changing in v9
thecoop Oct 14, 2024
872699c
Merge branch 'main' into exception-json
thecoop Nov 13, 2024
cc2bc6a
Revert "Add a deprecation warning that the JSON format of non-detaile…
thecoop Nov 13, 2024
980a449
Propagate changes from deprecation PRs
thecoop Nov 13, 2024
76f9185
Update tests
thecoop Nov 13, 2024
4726363
Update docs/changelog/90529.yaml
thecoop Nov 13, 2024
cb964f0
Revert "Update docs/changelog/90529.yaml"
thecoop Nov 13, 2024
aef3b2d
Use more randomness in detailed parameter
thecoop Nov 13, 2024
cd81c30
splotless
thecoop Nov 13, 2024
108dd01
Update test
thecoop Nov 14, 2024
c4a929c
Merge branch 'main' into exception-json
thecoop Nov 14, 2024
9ccb15c
Don't want stray warnings here
thecoop Nov 14, 2024
66c8b91
Merge branch 'main' into exception-json
elasticmachine Nov 18, 2024
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
Prev Previous commit
Next Next commit
Revert "Add a deprecation warning that the JSON format of non-detaile…
…d errors is changing in v9 (#116330)"

This reverts commit bd091d3.
  • Loading branch information
thecoop committed Nov 13, 2024
commit cc2bc6a9ece4d66cf8bca6fd19fd839d99061856
13 changes: 0 additions & 13 deletions server/src/main/java/org/elasticsearch/rest/RestResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
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 @@ -45,7 +43,6 @@ 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 @@ -145,16 +142,6 @@ 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
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, true, 0);
FakeRestChannel channel = new FakeRestChannel(request, false, 0);
try (var threadPool = createThreadPool()) {
final var nodeClient = new NoOpNodeClient(threadPool);
expectThrows(IllegalArgumentException.class, () -> action.handleRequest(request, channel, nodeClient));
Expand Down
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, true, 0);
FakeRestChannel channel = new FakeRestChannel(request, false, 0);
try (var threadPool = createThreadPool()) {
final var nodeClient = new NoOpNodeClient(threadPool);
expectThrows(IllegalArgumentException.class, () -> action.handleRequest(request, channel, nodeClient));
Expand Down
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,
true,
false,
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, true, RestStatus.BAD_REQUEST);
RestControllerTests.AssertingChannel channel = new RestControllerTests.AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);

try (
AbstractHttpServerTransport transport = new AbstractHttpServerTransport(
Expand Down
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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 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, true, 1);
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> handler.handleRequest(request, channel, mockClient)
Expand Down
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(),
true,
randomBoolean(),
1
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,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, true, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, 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 @@ -180,7 +180,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, true, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRequest, channel, threadContext);
assertTrue(channel.getSendResponseCalled());
}
Expand Down Expand Up @@ -211,7 +211,7 @@ public String getName() {
});
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.OK);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(
eq(1L),
Expand All @@ -235,7 +235,7 @@ public MethodHandlers next() {
return null;
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 400)));
}
Expand All @@ -257,7 +257,7 @@ public MethodHandlers next() {
}
});

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

AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.METHOD_NOT_ALLOWED);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.METHOD_NOT_ALLOWED);
spyRestController.dispatchRequest(fakeRequest, channel, threadContext);
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 405)));
}
Expand All @@ -290,7 +290,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, true, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
restController.dispatchBadRequest(channel, threadContext, new Exception());
verify(requestsCounter).incrementBy(eq(1L), eq(Map.of(STATUS_CODE_KEY, 400)));
}
Expand All @@ -314,7 +314,7 @@ public MethodHandlers next() {
return new MethodHandlers("/").addMethod(GET, RestApiVersion.current(), (request, channel, client) -> {});
}
});
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.BAD_REQUEST);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRequest, channel, threadContext);
verify(tracer).startTrace(
eq(threadContext),
Expand All @@ -340,7 +340,7 @@ public void testRequestWithDisallowedMultiValuedHeaderButSameValues() {
new RestResponse(RestStatus.OK, RestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)
)
);
AssertingChannel channel = new AssertingChannel(fakeRequest, true, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.OK);
restController.dispatchRequest(fakeRequest, channel, threadContext);
assertTrue(channel.getSendResponseCalled());
}
Expand Down Expand Up @@ -831,7 +831,7 @@ public void testFavicon() {
final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withMethod(GET)
.withPath("/favicon.ico")
.build();
final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
final AssertingChannel channel = new AssertingChannel(fakeRestRequest, false, RestStatus.OK);
restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
assertTrue(channel.getSendResponseCalled());
assertThat(channel.getRestResponse().contentType(), containsString("image/x-icon"));
Expand Down Expand Up @@ -1115,7 +1115,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, true, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(request, false, RestStatus.OK);
restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY));
});
}
Expand All @@ -1137,7 +1137,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, true, RestStatus.OK);
AssertingChannel channel = new AssertingChannel(request, false, RestStatus.OK);
restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY));
});
final Consumer<List<String>> checkProtected = paths -> paths.forEach(path -> {
Expand Down
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, true, 1);
FakeRestChannel restChannel = new FakeRestChannel(restRequest, false, 1);
restController.dispatchRequest(restRequest, restChannel, new ThreadContext(Settings.EMPTY));
assertThat(restChannel.capturedResponse().status().getStatus(), is(405));

Expand Down
Loading