From 42a3670812fd20aa61ad352ad2f2a26f6362bb46 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 14 Apr 2026 09:26:27 -0700 Subject: [PATCH] update invalid date error to return 400 Signed-off-by: Ritvi Bhatt --- .../executor/format/ErrorMessageFactory.java | 5 +- .../format/OpenSearchErrorMessage.java | 4 +- .../sql/legacy/plugin/RestSqlAction.java | 15 ++++- .../executor/OpenSearchExecutionEngine.java | 8 ++- .../response/error/ErrorMessageFactory.java | 3 +- .../error/OpenSearchErrorMessage.java | 4 +- .../sql/plugin/rest/RestPPLQueryAction.java | 55 ++++++++++++++----- 7 files changed, 68 insertions(+), 26 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessageFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessageFactory.java index ba28ee83252..b46869cc41c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessageFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessageFactory.java @@ -19,11 +19,10 @@ public class ErrorMessageFactory { */ public static ErrorMessage createErrorMessage(Exception e, int status) { if (e instanceof OpenSearchException) { - return new OpenSearchErrorMessage( - (OpenSearchException) e, ((OpenSearchException) e).status().getStatus()); + return new OpenSearchErrorMessage((OpenSearchException) e, status); } else if (unwrapCause(e) instanceof OpenSearchException) { OpenSearchException exception = (OpenSearchException) unwrapCause(e); - return new OpenSearchErrorMessage(exception, exception.status().getStatus()); + return new OpenSearchErrorMessage(exception, status); } return new ErrorMessage(e, status); } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/OpenSearchErrorMessage.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/OpenSearchErrorMessage.java index 09c09919ec1..8117d241b1a 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/OpenSearchErrorMessage.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/OpenSearchErrorMessage.java @@ -12,8 +12,8 @@ public class OpenSearchErrorMessage extends ErrorMessage { - OpenSearchErrorMessage(OpenSearchException exception, int defaultStatus) { - super(exception, exception.status() != null ? exception.status().getStatus() : defaultStatus); + OpenSearchErrorMessage(OpenSearchException exception, int status) { + super(exception, status); } @Override 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 9be2367dcaa..45c499e097d 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 @@ -23,6 +23,8 @@ import java.util.regex.Pattern; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.opensearch.action.search.SearchPhaseExecutionException; +import org.opensearch.action.search.ShardSearchFailure; import org.opensearch.OpenSearchException; import org.opensearch.common.inject.Injector; import org.opensearch.common.settings.Settings; @@ -171,7 +173,18 @@ private void handleException(RestChannel restChannel, Exception exception) { logAndPublishMetrics(exception); if (exception instanceof OpenSearchException) { OpenSearchException openSearchException = (OpenSearchException) exception; - reportError(restChannel, openSearchException, openSearchException.status()); + RestStatus status = openSearchException.status(); + if (exception instanceof SearchPhaseExecutionException) { + for (ShardSearchFailure failure : + ((SearchPhaseExecutionException) exception).shardFailures()) { + Throwable cause = failure.getCause(); + if (cause instanceof Exception && isClientError((Exception) cause)) { + status = BAD_REQUEST; + break; + } + } + } + reportError(restChannel, openSearchException, status); } else { reportError( restChannel, exception, isClientError(exception) ? BAD_REQUEST : INTERNAL_SERVER_ERROR); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index 32b7891d344..42c48be3220 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -222,8 +222,12 @@ public void execute( metric.add(System.nanoTime() - execTime); listener.onResponse(response); - } catch (SQLException e) { - throw new RuntimeException(e); + } catch (Throwable t) { + if (t instanceof VirtualMachineError) { + throw (VirtualMachineError) t; + } + Exception e = (t instanceof Exception) ? (Exception) t : new RuntimeException(t); + listener.onFailure(e); } }); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java index 8617f264f06..703ada6f905 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java @@ -22,8 +22,7 @@ public class ErrorMessageFactory { public static ErrorMessage createErrorMessage(Throwable e, int status) { Throwable cause = unwrapCause(e); if (cause instanceof OpenSearchException) { - OpenSearchException exception = (OpenSearchException) cause; - return new OpenSearchErrorMessage(exception, exception.status().getStatus()); + return new OpenSearchErrorMessage((OpenSearchException) cause, status); } return new ErrorMessage(cause, status); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java index a712ceaedf2..87a374d3534 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java @@ -13,8 +13,8 @@ /** OpenSearch Error Message. */ public class OpenSearchErrorMessage extends ErrorMessage { - OpenSearchErrorMessage(OpenSearchException exception, int defaultStatus) { - super(exception, exception.status() != null ? exception.status().getStatus() : defaultStatus); + OpenSearchErrorMessage(OpenSearchException exception, int status) { + super(exception, status); } @Override diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java index ffdd90504f7..16402fbf0d7 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java @@ -16,6 +16,8 @@ import java.util.Set; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.opensearch.action.search.SearchPhaseExecutionException; +import org.opensearch.action.search.ShardSearchFailure; import org.opensearch.OpenSearchException; import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; @@ -50,16 +52,23 @@ public RestPPLQueryAction() { } private static boolean isClientError(Exception e) { - return e instanceof NullPointerException - // NPE is hard to differentiate but more likely caused by bad query - || e instanceof IllegalArgumentException - || e instanceof IndexNotFoundException - || e instanceof SemanticCheckException - || e instanceof ExpressionEvaluationException - || e instanceof QueryEngineException - || e instanceof SyntaxCheckException - || e instanceof DataSourceClientException - || e instanceof IllegalAccessException; + // NPE is hard to differentiate but more likely caused by bad query + Throwable current = e; + while (current != null) { + if (current instanceof NullPointerException + || current instanceof IllegalArgumentException + || current instanceof IndexNotFoundException + || current instanceof SemanticCheckException + || current instanceof ExpressionEvaluationException + || current instanceof QueryEngineException + || current instanceof SyntaxCheckException + || current instanceof DataSourceClientException + || current instanceof IllegalAccessException) { + return true; + } + current = current.getCause(); + } + return false; } @Override @@ -106,11 +115,29 @@ public void onFailure(Exception e) { reportError(channel, e, INTERNAL_SERVER_ERROR); } } else if (e instanceof OpenSearchException) { - Metrics.getInstance() - .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS) - .increment(); OpenSearchException exception = (OpenSearchException) e; - reportError(channel, exception, exception.status()); + RestStatus status = exception.status(); + if (e instanceof SearchPhaseExecutionException) { + for (ShardSearchFailure failure : + ((SearchPhaseExecutionException) e).shardFailures()) { + Throwable cause = failure.getCause(); + if (cause instanceof Exception + && isClientError((Exception) cause)) { + status = BAD_REQUEST; + break; + } + } + } + if (status == BAD_REQUEST) { + Metrics.getInstance() + .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS) + .increment(); + } else { + Metrics.getInstance() + .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS) + .increment(); + } + reportError(channel, exception, status); } else { LOG.error("Error happened during query handling", e); if (isClientError(e)) {