Implement error_trace parameter for HTTP Bulk requests

CHANGELOG.md

@@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add async periodic flush task support for pull-based ingestion ([#19878](https://github.com/opensearch-project/OpenSearch/pull/19878))
 - Add support for context aware segments ([#19098](https://github.com/opensearch-project/OpenSearch/pull/19098))
 - Implement GRPC FunctionScoreQuery ([#19888](https://github.com/opensearch-project/OpenSearch/pull/19888))
+- Implement error_trace parameter for bulk requests ([#19985](https://github.com/opensearch-project/OpenSearch/pull/19985))
 
 ### Changed
 - Faster `terms` query creation for `keyword` field with index and docValues enabled ([#19350](https://github.com/opensearch-project/OpenSearch/pull/19350))

qa/smoke-test-http/src/test/java/org/opensearch/http/DetailedErrorsDisabledIT.java

@@ -34,17 +34,26 @@
 
 import java.io.IOException;
 
+import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.ByteArrayEntity;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.opensearch.action.search.MultiSearchRequest;
+import org.opensearch.action.search.SearchRequest;
 import org.opensearch.client.Request;
 import org.opensearch.client.Response;
 import org.opensearch.client.ResponseException;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.core.xcontent.MediaType;
+import org.opensearch.core.xcontent.MediaTypeRegistry;
+import org.opensearch.index.query.QueryStringQueryBuilder;
+import org.opensearch.search.builder.SearchSourceBuilder;
 import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope;
 import org.opensearch.test.OpenSearchIntegTestCase.Scope;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 
 /**
  * Tests that when disabling detailed errors, a request with the error_trace parameter returns an HTTP 400 response.
@@ -73,4 +82,19 @@ public void testThatErrorTraceParamReturns400() throws IOException, ParseExcepti
                    containsString("\"error\":\"error traces in responses are disabled.\""));
         assertThat(response.getStatusLine().getStatusCode(), is(400));
     }
+
+    public void testDetailedStackTracesAreNotIncludedWhenErrorTraceIsDisabledForBulkApis() throws IOException, ParseException {
+        MediaType contentType = MediaTypeRegistry.JSON;
+        MultiSearchRequest multiSearchRequest = new MultiSearchRequest().add(
+            new SearchRequest("missing_index")
+                .source(new SearchSourceBuilder().query(new QueryStringQueryBuilder("foo").field("*"))));
+        Request request = new Request("POST", "/_msearch");
+        byte[] requestBody = MultiSearchRequest.writeMultiLineFormat(multiSearchRequest, contentType.xContent());
+        request.setEntity(new ByteArrayEntity(requestBody, ContentType.APPLICATION_JSON));
+
+        Response response = getRestClient().performRequest(request);
+
+        assertThat(EntityUtils.toString(response.getEntity()), not(containsString("stack_trace")));
+    }
+
 }

qa/smoke-test-http/src/test/java/org/opensearch/http/DetailedErrorsEnabledIT.java

@@ -32,16 +32,24 @@
 
 package org.opensearch.http;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+
+import java.io.IOException;
+
+import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.ByteArrayEntity;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.opensearch.action.search.MultiSearchRequest;
+import org.opensearch.action.search.SearchRequest;
 import org.opensearch.client.Request;
 import org.opensearch.client.Response;
 import org.opensearch.client.ResponseException;
-
-import java.io.IOException;
-
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.not;
+import org.opensearch.core.xcontent.MediaType;
+import org.opensearch.core.xcontent.MediaTypeRegistry;
+import org.opensearch.index.query.QueryStringQueryBuilder;
+import org.opensearch.search.builder.SearchSourceBuilder;
 
 /**
  * Tests that by default the error_trace parameter can be used to show stacktraces
@@ -54,23 +62,40 @@ public void testThatErrorTraceWorksByDefault() throws IOException, ParseExceptio
             request.addParameter("error_trace", "true");
             getRestClient().performRequest(request);
             fail("request should have failed");
-        } catch(ResponseException e) {
+        } catch (ResponseException e) {
             Response response = e.getResponse();
             assertThat(response.getHeader("Content-Type"), containsString("application/json"));
             assertThat(EntityUtils.toString(response.getEntity()),
-                    containsString("\"stack_trace\":\"OpenSearchException[Validation Failed: 1: index / indices is missing;]; " +
+                containsString("\"stack_trace\":\"OpenSearchException[Validation Failed: 1: index / indices is missing;]; " +
                     "nested: ActionRequestValidationException[Validation Failed: 1:"));
         }
 
         try {
             getRestClient().performRequest(new Request("DELETE", "/"));
             fail("request should have failed");
-        } catch(ResponseException e) {
+        } catch (ResponseException e) {
             Response response = e.getResponse();
             assertThat(response.getHeader("Content-Type"), containsString("application/json; charset=UTF-8"));
             assertThat(EntityUtils.toString(response.getEntity()),
-                    not(containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; "
+                not(containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; "
                     + "nested: ActionRequestValidationException[Validation Failed: 1:")));
         }
     }
+
+    public void testDetailedStackTracesAreIncludedWhenErrorTraceIsExplicitlyEnabledForBulkApis() throws IOException, ParseException {
+        MediaType contentType = MediaTypeRegistry.JSON;
+        MultiSearchRequest multiSearchRequest = new MultiSearchRequest().add(
+            new SearchRequest("missing_index")
+                .source(new SearchSourceBuilder().query(new QueryStringQueryBuilder("foo").field("*"))));
+        Request request = new Request("POST", "/_msearch");
+        request.addParameter("error_trace", "true");
+        byte[] requestBody = MultiSearchRequest.writeMultiLineFormat(multiSearchRequest, contentType.xContent());
+        request.setEntity(new ByteArrayEntity(requestBody, ContentType.APPLICATION_JSON));
+
+        Response response = getRestClient().performRequest(request);
+
+        assertThat(EntityUtils.toString(response.getEntity()),
+            containsString("\"stack_trace\":\"[missing_index] IndexNotFoundException[no such index [missing_index]]"));
+    }
+
 }

server/src/main/java/org/opensearch/rest/AbstractRestChannel.java

@@ -64,6 +64,7 @@ public abstract class AbstractRestChannel implements RestChannel {
     private final boolean pretty;
     private final boolean human;
     private final String acceptHeader;
+    private final boolean detailedErrorStackTraceRequested;
 
     private BytesStreamOutput bytesOut;
 
@@ -82,6 +83,7 @@ protected AbstractRestChannel(RestRequest request, boolean detailedErrorsEnabled
         this.filterPath = request.param("filter_path", null);
         this.pretty = request.paramAsBoolean("pretty", false);
         this.human = request.paramAsBoolean("human", false);
+        this.detailedErrorStackTraceRequested = request.paramAsBoolean("error_trace", false);
     }
 
     @Override
@@ -189,4 +191,8 @@ public RestRequest request() {
     public boolean detailedErrorsEnabled() {
         return detailedErrorsEnabled;
     }
+
+    public boolean detailedErrorStackTraceRequested() {
+        return detailedErrorStackTraceRequested;
+    }
 }

server/src/main/java/org/opensearch/rest/RestChannel.java

@@ -66,5 +66,10 @@ XContentBuilder newBuilder(@Nullable MediaType mediaType, @Nullable MediaType re
      */
     boolean detailedErrorsEnabled();
 
+    /**
+     * @return true iff detailed stack traces should be included in the response.
+     */
+    boolean detailedErrorStackTraceRequested();
+
     void sendResponse(RestResponse response);
 }

server/src/main/java/org/opensearch/rest/RestController.java

@@ -436,7 +436,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
         }
         // error_trace cannot be used when we disable detailed errors
         // we consume the error_trace parameter first to ensure that it is always consumed
-        if (request.paramAsBoolean("error_trace", false) && channel.detailedErrorsEnabled() == false) {
+        if (channel.detailedErrorStackTraceRequested() && channel.detailedErrorsEnabled() == false) {
             channel.sendResponse(
                 BytesRestResponse.createSimpleErrorResponse(channel, BAD_REQUEST, "error traces in responses are disabled.")
             );
@@ -636,6 +636,11 @@ public boolean detailedErrorsEnabled() {
             return delegate.detailedErrorsEnabled();
         }
 
+        @Override
+        public boolean detailedErrorStackTraceRequested() {
+            return delegate.detailedErrorStackTraceRequested();
+        }
+
         @Override
         public void sendResponse(RestResponse response) {
             close();
@@ -699,6 +704,11 @@ public boolean detailedErrorsEnabled() {
             return delegate.detailedErrorsEnabled();
         }
 
+        @Override
+        public boolean detailedErrorStackTraceRequested() {
+            return delegate.detailedErrorStackTraceRequested();
+        }
+
         @Override
         public void sendResponse(RestResponse response) {
             close();

server/src/main/java/org/opensearch/rest/action/RestStatusToXContentListener.java

@@ -69,7 +69,7 @@ public RestStatusToXContentListener(RestChannel channel, Function<Response, Stri
     @Override
     public RestResponse buildResponse(Response response, XContentBuilder builder) throws Exception {
         assert response.isFragment() == false; // would be nice if we could make default methods final
-        response.toXContent(builder, channel.request());
+        response.toXContent(builder, getRequestParams());
         RestResponse restResponse = new BytesRestResponse(response.status(), builder);
         if (RestStatus.CREATED == restResponse.status()) {
             final String location = extractLocation.apply(response);

server/src/main/java/org/opensearch/rest/action/RestToXContentListener.java

@@ -32,6 +32,7 @@
 
 package org.opensearch.rest.action;
 
+import org.opensearch.OpenSearchException;
 import org.opensearch.core.rest.RestStatus;
 import org.opensearch.core.xcontent.ToXContent;
 import org.opensearch.core.xcontent.ToXContentObject;
@@ -40,6 +41,8 @@
 import org.opensearch.rest.RestChannel;
 import org.opensearch.rest.RestResponse;
 
+import java.util.Map;
+
 /**
  * A REST based action listener that assumes the response is of type {@link ToXContent} and automatically
  * builds an XContent based response (wrapping the toXContent in startObject/endObject).
@@ -59,11 +62,20 @@ public final RestResponse buildResponse(Response response) throws Exception {
 
     public RestResponse buildResponse(Response response, XContentBuilder builder) throws Exception {
         assert response.isFragment() == false; // would be nice if we could make default methods final
-        response.toXContent(builder, channel.request());
+        response.toXContent(builder, getRequestParams());
         return new BytesRestResponse(getStatus(response), builder);
     }
 
     protected RestStatus getStatus(Response response) {
         return RestStatus.OK;
     }
+
+    protected ToXContent.Params getRequestParams() {
+        ToXContent.Params params = channel.request();
+        // We add detailed stack traces if the request indicates so
+        if (channel.detailedErrorStackTraceRequested()) {
+            return new ToXContent.DelegatingMapParams(Map.of(OpenSearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
+        }
+        return params;
+    }
 }

server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableRestChannel.java

@@ -94,6 +94,11 @@ public boolean detailedErrorsEnabled() {
         return delegate.detailedErrorsEnabled();
     }
 
+    @Override
+    public boolean detailedErrorStackTraceRequested() {
+        return delegate.detailedErrorStackTraceRequested();
+    }
+
     @Override
     public void sendResponse(RestResponse response) {
         try (SpanScope scope = tracer.withSpanInScope(span)) {

server/src/test/java/org/opensearch/rest/RestControllerTests.java

@@ -139,8 +139,8 @@ public void testDefaultRestControllerGetAllHandlersContainsFavicon() {
         Iterator<MethodHandlers> handlers = restController.getAllHandlers();
         assertTrue(handlers.hasNext());
         MethodHandlers faviconHandler = handlers.next();
-        assertEquals(faviconHandler.getPath(), "/favicon.ico");
-        assertEquals(faviconHandler.getValidMethods(), Set.of(RestRequest.Method.GET));
+        assertEquals("/favicon.ico", faviconHandler.getPath());
+        assertEquals(Set.of(RestRequest.Method.GET), faviconHandler.getValidMethods());
         assertFalse(handlers.hasNext());
     }
 
@@ -154,13 +154,13 @@ public void testRestControllerGetAllHandlers() {
 
         assertTrue(handlers.hasNext());
         MethodHandlers rootHandler = handlers.next();
-        assertEquals(rootHandler.getPath(), "/foo");
-        assertEquals(rootHandler.getValidMethods(), Set.of(RestRequest.Method.GET, RestRequest.Method.PATCH));
+        assertEquals("/foo", rootHandler.getPath());
+        assertEquals(Set.of(RestRequest.Method.GET, RestRequest.Method.PATCH), rootHandler.getValidMethods());
 
         assertTrue(handlers.hasNext());
         MethodHandlers faviconHandler = handlers.next();
-        assertEquals(faviconHandler.getPath(), "/favicon.ico");
-        assertEquals(faviconHandler.getValidMethods(), Set.of(RestRequest.Method.GET));
+        assertEquals("/favicon.ico", faviconHandler.getPath());
+        assertEquals(Set.of(RestRequest.Method.GET), faviconHandler.getValidMethods());
 
         assertFalse(handlers.hasNext());
     }
@@ -595,6 +595,18 @@ public void testHandleBadRequestWithHtmlSpecialCharsInUri() {
         assertThat(channel.getRestResponse().content().utf8ToString(), containsString("invalid uri has been requested"));
     }
 
+    public void testHandleBadRequestWithInvalidCombinationForDetailedErrors() {
+        final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath("/foo")
+            .withMethod(RestRequest.Method.GET)
+            .withParams(Map.of("error_trace", "true"))
+            .build();
+        final AssertingChannel channel = new AssertingChannel(fakeRestRequest, false, RestStatus.BAD_REQUEST);
+        restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
+        assertThat(channel.detailedErrorsEnabled(), equalTo(false));
+        assertThat(channel.detailedErrorStackTraceRequested(), equalTo(true));
+        assertThat(channel.getRestResponse().content().utf8ToString(), containsString("error traces in responses are disabled."));
+    }
+
     public void testHandleBadInputWithCreateIndex() {
         final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath("/foo")
             .withMethod(RestRequest.Method.PUT)
@@ -604,9 +616,34 @@ public void testHandleBadInputWithCreateIndex() {
         restController.registerHandler(RestRequest.Method.PUT, "/foo", new RestCreateIndexAction());
         restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
         assertEquals(
-            channel.getRestResponse().content().utf8ToString(),
-            "{\"error\":{\"root_cause\":[{\"type\":\"not_x_content_exception\",\"reason\":\"Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes\"}],\"type\":\"not_x_content_exception\",\"reason\":\"Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes\"},\"status\":400}"
+            "{\"error\":{\"root_cause\":[{\"type\":\"not_x_content_exception\",\"reason\":\"Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes\"}],\"type\":\"not_x_content_exception\",\"reason\":\"Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes\"},\"status\":400}",
+            channel.getRestResponse().content().utf8ToString()
+        );
+    }
+
+    public void testHandleBadInputWithCreateIndexReturnsDetailedErrorStackTraceWhenRequested() {
+        final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath("/foo")
+            .withMethod(RestRequest.Method.PUT)
+            .withParams(new HashMap<>(Map.of("error_trace", "true")))
+            .withContent(new BytesArray("ddd"), MediaTypeRegistry.JSON)
+            .build();
+        final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST);
+        restController.registerHandler(RestRequest.Method.PUT, "/foo", new RestCreateIndexAction());
+        restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
+        String responseBodyString = channel.getRestResponse().content().utf8ToString();
+        assertThat(
+            responseBodyString,
+            containsString(
+                "{\"error\":{\"root_cause\":[{\"type\":\"not_x_content_exception\",\"reason\":\"Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes\""
+            )
+        );
+        assertThat(
+            responseBodyString,
+            containsString(
+                "\"stack_trace\":\"OpenSearchException[Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes];"
+            )
         );
+        assertThat(responseBodyString, containsString("\"status\":400"));
     }
 
     public void testDispatchUnsupportedHttpMethod() {