Skip to content

Add exception type response header for 5xx errors #130226

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
23 changes: 16 additions & 7 deletions server/src/main/java/org/elasticsearch/ElasticsearchException.java
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
private static final String ROOT_CAUSE = "root_cause";

static final String TIMED_OUT_HEADER = "X-Timed-Out";
static final String EXCEPTION_TYPE_HEADER = "X-Elasticsearch-Exception";

private static final Map<Integer, CheckedFunction<StreamInput, ? extends ElasticsearchException, IOException>> ID_TO_SUPPLIER;
private static final Map<Class<? extends ElasticsearchException>, ElasticsearchExceptionHandle> CLASS_TO_ELASTICSEARCH_EXCEPTION_HANDLE;
Expand All @@ -130,7 +131,6 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
@SuppressWarnings("this-escape")
public ElasticsearchException(Throwable cause) {
super(cause);
maybePutTimeoutHeader();
}

/**
Expand All @@ -145,7 +145,6 @@ public ElasticsearchException(Throwable cause) {
@SuppressWarnings("this-escape")
public ElasticsearchException(String msg, Object... args) {
super(LoggerMessageFormat.format(msg, args));
maybePutTimeoutHeader();
}

/**
Expand All @@ -162,7 +161,6 @@ public ElasticsearchException(String msg, Object... args) {
@SuppressWarnings("this-escape")
public ElasticsearchException(String msg, Throwable cause, Object... args) {
super(LoggerMessageFormat.format(msg, args), cause);
maybePutTimeoutHeader();
}

@SuppressWarnings("this-escape")
Expand All @@ -173,11 +171,19 @@ public ElasticsearchException(StreamInput in) throws IOException {
metadata.putAll(in.readMapOfLists(StreamInput::readString));
}

private void maybePutTimeoutHeader() {
if (isTimeout()) {
private void maybeAddErrorHeaders() {
if (isTimeout() && headers.containsKey(TIMED_OUT_HEADER) == false) {
// see https://www.rfc-editor.org/rfc/rfc8941.html#section-4.1.9 for booleans in structured headers
headers.put(TIMED_OUT_HEADER, List.of("?1"));
}
if (headers.containsKey(EXCEPTION_TYPE_HEADER) == false) {
// TODO: cache unwrapping the cause? we do this in several places...
Throwable cause = unwrapCause();
RestStatus status = ExceptionsHelper.status(cause);
if (status.getStatus() >= 500) {
headers.put(EXCEPTION_TYPE_HEADER, List.of(cause.getClass().getSimpleName()));
}
}
}

/**
Expand Down Expand Up @@ -243,6 +249,7 @@ public void addHeader(String key, String... value) {
* Returns a set of all header keys on this exception
*/
public Set<String> getHeaderKeys() {
maybeAddErrorHeaders();
return headers.keySet();
}

Expand All @@ -251,10 +258,12 @@ public Set<String> getHeaderKeys() {
* given key exists.
*/
public List<String> getHeader(String key) {
maybeAddErrorHeaders();
return headers.get(key);
}

protected Map<String, List<String>> getHeaders() {
maybeAddErrorHeaders();
return headers;
}

Expand Down Expand Up @@ -335,7 +344,7 @@ private static Writer<Throwable> createNestingFunction(int thisLevel, Runnable n
protected void writeTo(StreamOutput out, Writer<Throwable> nestedExceptionsWriter) throws IOException {
out.writeOptionalString(this.getMessage());
nestedExceptionsWriter.write(out, this);
out.writeMap(headers, StreamOutput::writeStringCollection);
out.writeMap(getHeaders(), StreamOutput::writeStringCollection);
out.writeMap(metadata, StreamOutput::writeStringCollection);
}

Expand Down Expand Up @@ -384,7 +393,7 @@ protected XContentBuilder toXContent(XContentBuilder builder, Params params, int
if (ex != this) {
generateThrowableXContent(builder, params, this, nestedLevel);
} else {
innerToXContent(builder, params, this, headers, metadata, getCause(), nestedLevel);
innerToXContent(builder, params, this, getHeaders(), metadata, getCause(), nestedLevel);
}
return builder;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1553,19 +1553,24 @@ private void testExceptionLoop(Exception rootException) throws IOException {
assertArrayEquals(ser.getStackTrace(), rootException.getStackTrace());
}

static class ExceptionSubclass extends ElasticsearchException {
static class TimeoutSubclass extends ElasticsearchException {
TimeoutSubclass(String message) {
super(message);
}

@Override
public boolean isTimeout() {
return true;
}

ExceptionSubclass(String message) {
super(message);
@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}

public void testTimeout() throws IOException {
var e = new ExceptionSubclass("some timeout");
public void testTimeoutHeader() throws IOException {
var e = new TimeoutSubclass("some timeout");
assertThat(e.getHeaderKeys(), hasItem(ElasticsearchException.TIMED_OUT_HEADER));

XContentBuilder builder = XContentFactory.jsonBuilder();
Expand All @@ -1574,7 +1579,7 @@ public void testTimeout() throws IOException {
builder.endObject();
String expected = """
{
"type": "exception_subclass",
"type": "timeout_subclass",
"reason": "some timeout",
"timed_out": true,
"header": {
Expand All @@ -1583,4 +1588,61 @@ public void testTimeout() throws IOException {
}""";
assertEquals(XContentHelper.stripWhitespace(expected), Strings.toString(builder));
}

static class Exception5xx extends ElasticsearchException {
Exception5xx(String message) {
super(message);
}

@Override
public RestStatus status() {
return RestStatus.INTERNAL_SERVER_ERROR;
}
}

static class Exception4xx extends ElasticsearchException {
Exception4xx(String message) {
super(message);
}

@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}

public void testExceptionTypeHeader() throws IOException {
var e = new Exception5xx("some exception");
assertThat(e.getHeaderKeys(), hasItem(ElasticsearchException.EXCEPTION_TYPE_HEADER));

XContentBuilder builder = XContentFactory.jsonBuilder();
builder.startObject();
e.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
String expected = """
{
"type": "exception5xx",
"reason": "some exception",
"header": {
"X-Elasticsearch-Exception": "Exception5xx"
}
}""";
assertEquals(XContentHelper.stripWhitespace(expected), Strings.toString(builder));
}

public void testNoExceptionTypeHeaderOn4xx() throws IOException {
var e = new Exception4xx("some exception");
assertThat(e.getHeaderKeys(), not(hasItem(ElasticsearchException.EXCEPTION_TYPE_HEADER)));

XContentBuilder builder = XContentFactory.jsonBuilder();
builder.startObject();
e.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
String expected = """
{
"type": "exception4xx",
"reason": "some exception"
}""";
assertEquals(XContentHelper.stripWhitespace(expected), Strings.toString(builder));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public void testExceptionRegistration() throws IOException, URISyntaxException {
CancellableThreadsTests.CustomException.class,
RestResponseTests.WithHeadersException.class,
AbstractClientHeadersTestCase.InternalException.class,
ElasticsearchExceptionTests.ExceptionSubclass.class
ElasticsearchExceptionTests.TimeoutSubclass.class
);
FileVisitor<Path> visitor = new FileVisitor<Path>() {
private Path pkgPrefix = PathUtils.get(path).getParent();
Expand Down