diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java index 1b740924c44..96b2c7b4940 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java @@ -13,11 +13,13 @@ import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_TYPE; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PHRASE; +import static org.opensearch.sql.legacy.plugin.RestSqlAction.EXPLAIN_API_ENDPOINT; import com.google.common.io.Files; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import org.json.JSONObject; import org.junit.Assert; import org.junit.Ignore; import org.junit.Test; @@ -253,4 +255,25 @@ public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { assertEquals("application/json; charset=UTF-8", response.getHeader("content-type")); } + + /** + * Prior to OpenSearch 3.0, {@code ?format=json} was a valid way to request the explain plan. + * "json" was never a real {@code Format} value the query endpoint accepts (only + * jdbc/csv/raw/table are), so this depends on the explain endpoint specifically tolerating it. + * Regression test for https://github.com/opensearch-project/sql/issues/4373: this used to fail + * with "Failed to create executor due to unknown response format: json" before the explain + * endpoint reached any query-specific logic at all. + */ + @Test + public void testExplainAcceptsJsonFormatForBackwardCompatibility() throws IOException { + String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); + Request request = new Request("POST", EXPLAIN_API_ENDPOINT + "?format=json"); + request.setJsonEntity(query); + + Response response = client().performRequest(request); + + assertEquals(200, response.getStatusLine().getStatusCode()); + JSONObject explanation = new JSONObject(TestUtils.getResponseBody(response)); + Assert.assertFalse("explain response should not contain an error", explanation.has("error")); + } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 4064b73d4a4..cc772ef83ea 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -143,7 +143,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli LOG.info("[{}] Incoming request {}", QueryContext.getRequestId(), request.uri()); - Format format = SqlRequestParam.getFormat(request.params()); + Format format = resolveFormat(request); SQLQueryRequest newSqlRequest = new SQLQueryRequest( @@ -320,6 +320,29 @@ private static boolean isExplainRequest(final RestRequest request) { return request.path().endsWith("/_explain"); } + /** + * Resolve the response {@link Format} for this request. + * + *

{@code format=json} is accepted for explain requests for backward compatibility: prior to + * OpenSearch 3.0, {@code ?format=json} was a valid way to request the explain plan (see #4373). + * It was never a real member of {@link Format} - {@code SqlRequestParam#getFormat} always + * rejected it - but the explain response body is JSON regardless of this parameter (see {@link + * #executeSqlRequest}, which calls {@code queryAction.explain().explain()} directly instead of + * going through a {@link Format}-specific executor), so the parameter can simply be ignored here + * for explain requests without changing any actual behavior. + */ + private static Format resolveFormat(final RestRequest request) { + try { + return SqlRequestParam.getFormat(request.params()); + } catch (IllegalArgumentException e) { + if (isExplainRequest(request) + && "json".equalsIgnoreCase(request.param(SqlRequestParam.QUERY_PARAMS_FORMAT))) { + return Format.JDBC; + } + throw e; + } + } + private static boolean isClientError(Exception e) { return e instanceof diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index 456ea212717..a9d70126873 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -159,7 +159,11 @@ private boolean isSupportedFormat() { } private boolean isSupportedExplainFormat() { - return Stream.of("simple", "standard", "extended", "cost").anyMatch(format::equalsIgnoreCase); + // "json" is accepted for backward compatibility: the explain endpoint always returns JSON + // regardless of this parameter, so treating it as valid avoids the 400 regression + // introduced in OpenSearch 3.0. See https://github.com/opensearch-project/sql/issues/4373 + return Stream.of("simple", "standard", "extended", "cost", "json") + .anyMatch(format::equalsIgnoreCase); } private String getFormat(Map params) { diff --git a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java index e5f2400e6cb..5c8b12256dd 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java @@ -104,6 +104,22 @@ public void should_support_explain_format() { () -> assertTrue(explainRequest.isSupported())); } + @Test + public void should_support_explain_with_json_format() { + // Regression test for https://github.com/opensearch-project/sql/issues/4373. + // ?format=json was accepted before OpenSearch 3.0 and should continue to be valid. + // The explain endpoint always returns JSON regardless of this parameter. + SQLQueryRequest explainRequest = + SQLQueryRequestBuilder.request("SELECT 1") + .path("_plugins/_sql/_explain") + .params(Map.of("format", "json")) + .build(); + + assertAll( + () -> assertTrue(explainRequest.isExplainRequest()), + () -> assertTrue(explainRequest.isSupported())); + } + @Test public void should_not_support_explain_with_unsupported_explain_format() { SQLQueryRequest explainRequest =