Disable DELETE clause by defaut and add opensearch.sql.delete.enabled setting

docs/category.json

@@ -3,7 +3,8 @@
     "experiment/ppl/interfaces/endpoint.rst",
     "experiment/ppl/interfaces/protocol.rst",
     "experiment/ppl/admin/settings.rst",
-    "user/optimization/optimization.rst"
+    "user/optimization/optimization.rst",
+    "user/admin/settings.rst"
   ],
   "ppl_cli": [
     "experiment/ppl/cmd/dedup.rst",

docs/user/admin/settings.rst

@@ -275,3 +275,56 @@ Result set::
       }
     }
 
+opensearch.sql.delete.enabled
+======================
+
+Description
+-----------
+
+By default, DELETE clause disabled. You can enable DELETE clause by this setting.
+
+1. The default value is false.
+2. This setting is node scope.
+3. This setting can be updated dynamically.
+
+
+Example 1
+---------
+
+You can update the setting with a new value like this.
+
+SQL query::
+
+    sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_plugins/_sql/settings \
+    ... -d '{"transient":{"opensearch.sql.delete.enabled":"false"}}'
+    {
+      "acknowledged": true,
+      "persistent": {},
+      "transient": {
+        "opensearch": {
+          "sql": {
+            "delete": {
+              "enabled": "false"
+            }
+          }
+        }
+      }
+    }
+
+Example 2
+---------
+
+Query result after the setting updated is like:
+
+SQL query::
+
+    sh$ curl -sS -H 'Content-Type: application/json' -X POST localhost:9200/_plugins/_sql \
+    ... -d '{"query" : "DELETE * FROM accounts"}'
+    {
+      "error": {
+        "reason": "Invalid SQL query",
+        "details": "DELETE clause is disabled by default and will be deprecated. Using the opensearch.sql.delete.enabled setting to enable it",
+        "type": "SQLFeatureDisabledException"
+      },
+      "status": 400
+    }

integ-test/src/test/java/org/opensearch/sql/legacy/DeleteIT.java

@@ -28,6 +28,7 @@
 package org.opensearch.sql.legacy;
 
 import static org.hamcrest.core.IsEqual.equalTo;
+import static org.opensearch.sql.legacy.plugin.SqlSettings.SQL_DELETE_ENABLED;
 
 import java.io.IOException;
 import org.json.JSONObject;
@@ -40,6 +41,7 @@ public class DeleteIT extends SQLIntegTestCase {
   protected void init() throws Exception {
     loadIndex(Index.ACCOUNT);
     loadIndex(Index.PHRASE);
+    updateClusterSettings(new ClusterSetting(PERSISTENT, SQL_DELETE_ENABLED, "true"));
   }
 
   @Test

integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java

@@ -31,6 +31,7 @@
 import static org.opensearch.sql.legacy.TestUtils.getResponseBody;
 import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
 import static org.opensearch.sql.legacy.plugin.RestSqlSettingsAction.SETTINGS_API_ENDPOINT;
+import static org.opensearch.sql.legacy.plugin.SqlSettings.SQL_DELETE_ENABLED;
 
 import java.io.IOException;
 import java.util.Locale;
@@ -40,6 +41,7 @@
 import org.opensearch.client.RequestOptions;
 import org.opensearch.client.Response;
 import org.opensearch.client.ResponseException;
+import org.opensearch.sql.legacy.utils.StringUtils;
 
 public class PluginIT extends SQLIntegTestCase {
 
@@ -75,6 +77,30 @@ public void sqlEnableSettingsTest() throws IOException {
     wipeAllClusterSettings();
   }
 
+  @Test
+  public void sqlDeleteSettingsTest() throws IOException {
+    updateClusterSettings(new ClusterSetting(PERSISTENT, SQL_DELETE_ENABLED, "false"));
+
+    String deleteQuery = StringUtils.format("DELETE FROM %s", TestsConstants.TEST_INDEX_ACCOUNT);
+    final ResponseException exception =
+        expectThrows(ResponseException.class, () -> executeQuery(deleteQuery));
+    JSONObject actual = new JSONObject(TestUtils.getResponseBody(exception.getResponse()));
+    JSONObject expected =
+        new JSONObject(
+            "{\n"
+                + "  \"error\": {\n"
+                + "    \"reason\": \"Invalid SQL query\",\n"
+                + "    \"details\": \"DELETE clause is disabled by default and will be deprecated. Using "
+                + "the opensearch.sql.delete.enabled setting to enable it\",\n"
+                + "    \"type\": \"SQLFeatureDisabledException\"\n"
+                + "  },\n"
+                + "  \"status\": 400\n"
+                + "}");
+    assertTrue(actual.toString(), actual.similar(expected));
+
+    wipeAllClusterSettings();
+  }
+
   @Test
   public void sqlTransientOnlySettingTest() throws IOException {
     // (1) compact form

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

@@ -214,7 +214,7 @@ private static void logAndPublishMetrics(final Exception e) {
     }
 
     private static QueryAction explainRequest(final NodeClient client, final SqlRequest sqlRequest, Format format)
-            throws SQLFeatureNotSupportedException, SqlParseException {
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
 
         ColumnTypeProvider typeProvider = performAnalysis(sqlRequest.getSql());
 

legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java

@@ -31,6 +31,7 @@
 import java.util.Set;
 import org.opensearch.client.Client;
 import org.opensearch.sql.legacy.domain.QueryActionRequest;
+import org.opensearch.sql.legacy.exception.SQLFeatureDisabledException;
 import org.opensearch.sql.legacy.exception.SqlParseException;
 import org.opensearch.sql.legacy.query.OpenSearchActionFactory;
 import org.opensearch.sql.legacy.query.QueryAction;
@@ -67,7 +68,7 @@ public Client getClient() {
      * @throws SqlParseException
      */
     public QueryAction explain(QueryActionRequest queryActionRequest)
-            throws SqlParseException, SQLFeatureNotSupportedException {
+        throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException {
         return OpenSearchActionFactory.create(client, queryActionRequest);
     }
 }

legacy/src/main/java/org/opensearch/sql/legacy/plugin/SqlSettings.java

@@ -52,6 +52,7 @@ public class SqlSettings {
     public static final String QUERY_SLOWLOG = "opensearch.sql.query.slowlog";
     public static final String METRICS_ROLLING_WINDOW = "opensearch.sql.metrics.rollingwindow";
     public static final String METRICS_ROLLING_INTERVAL = "opensearch.sql.metrics.rollinginterval";
+    public static final String SQL_DELETE_ENABLED = "opensearch.sql.delete.enabled";
 
     public static final String CURSOR_ENABLED= "opensearch.sql.cursor.enabled";
     public static final String CURSOR_FETCH_SIZE = "opensearch.sql.cursor.fetch_size";
@@ -65,6 +66,8 @@ public SqlSettings() {
         Map<String, Setting<?>> settings = new HashMap<>();
         settings.put(SQL_ENABLED, Setting.boolSetting(SQL_ENABLED, true, NodeScope, Dynamic));
         settings.put(QUERY_SLOWLOG, Setting.intSetting(QUERY_SLOWLOG, 2, NodeScope, Dynamic));
+        settings.put(SQL_DELETE_ENABLED, Setting.boolSetting(SQL_DELETE_ENABLED, false, NodeScope,
+            Dynamic));
 
         settings.put(METRICS_ROLLING_WINDOW, Setting.longSetting(METRICS_ROLLING_WINDOW, 3600L, 2L,
                 NodeScope, Dynamic));

legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java

@@ -27,6 +27,7 @@
 package org.opensearch.sql.legacy.query;
 
 import static org.opensearch.sql.legacy.domain.IndexStatement.StatementType;
+import static org.opensearch.sql.legacy.plugin.SqlSettings.SQL_DELETE_ENABLED;
 import static org.opensearch.sql.legacy.utils.Util.toSqlExpr;
 
 import com.alibaba.druid.sql.ast.expr.SQLAggregateExpr;
@@ -56,6 +57,7 @@
 import org.opensearch.sql.legacy.domain.QueryActionRequest;
 import org.opensearch.sql.legacy.domain.Select;
 import org.opensearch.sql.legacy.esdomain.LocalClusterState;
+import org.opensearch.sql.legacy.exception.SQLFeatureDisabledException;
 import org.opensearch.sql.legacy.exception.SqlParseException;
 import org.opensearch.sql.legacy.executor.ElasticResultHandler;
 import org.opensearch.sql.legacy.executor.Format;
@@ -79,11 +81,12 @@
 import org.opensearch.sql.legacy.rewriter.ordinal.OrdinalRewriterRule;
 import org.opensearch.sql.legacy.rewriter.parent.SQLExprParentSetterRule;
 import org.opensearch.sql.legacy.rewriter.subquery.SubQueryRewriteRule;
+import org.opensearch.sql.legacy.utils.StringUtils;
 
 public class OpenSearchActionFactory {
 
     public static QueryAction create(Client client, String sql)
-            throws SqlParseException, SQLFeatureNotSupportedException {
+        throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException {
         return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON));
     }
 
@@ -95,7 +98,7 @@ public static QueryAction create(Client client, String sql)
      * @return Query object.
      */
     public static QueryAction create(Client client, QueryActionRequest request)
-            throws SqlParseException, SQLFeatureNotSupportedException {
+        throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException {
         String sql = request.getSql();
         // Remove line breaker anywhere and semicolon at the end
         sql = sql.replaceAll("\\R", " ").trim();
@@ -138,10 +141,17 @@ public static QueryAction create(Client client, QueryActionRequest request)
                     return handleSelect(client, select);
                 }
             case "DELETE":
-                SQLStatementParser parser = createSqlStatementParser(sql);
-                SQLDeleteStatement deleteStatement = parser.parseDeleteStatement();
-                Delete delete = new SqlParser().parseDelete(deleteStatement);
-                return new DeleteQueryAction(client, delete);
+                if (isSQLDeleteEnabled()) {
+                    SQLStatementParser parser = createSqlStatementParser(sql);
+                    SQLDeleteStatement deleteStatement = parser.parseDeleteStatement();
+                    Delete delete = new SqlParser().parseDelete(deleteStatement);
+                    return new DeleteQueryAction(client, delete);
+                } else {
+                    throw new SQLFeatureDisabledException(
+                        StringUtils.format("DELETE clause is disabled by default and will be "
+                                + "deprecated. Using the %s setting to enable it",
+                            SQL_DELETE_ENABLED));
+                }
             case "SHOW":
                 IndexStatement showStatement = new IndexStatement(StatementType.SHOW, sql);
                 return new ShowQueryAction(client, showStatement);
@@ -154,6 +164,10 @@ public static QueryAction create(Client client, QueryActionRequest request)
         }
     }
 
+    private static boolean isSQLDeleteEnabled() {
+        return LocalClusterState.state().getSettingValue(SQL_DELETE_ENABLED);
+    }
+
     private static String getFirstWord(String sql) {
         int endOfFirstWord = sql.indexOf(' ');
         return sql.substring(0, endOfFirstWord > 0 ? endOfFirstWord : sql.length()).toUpperCase();

legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java

@@ -29,7 +29,9 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Matchers.anyInt;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import com.alibaba.druid.sql.parser.ParserException;
@@ -41,14 +43,20 @@
 import org.json.JSONObject;
 import org.junit.Before;
 import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
+import org.mockito.MockedStatic;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 import org.opensearch.client.Client;
+import org.opensearch.sql.common.utils.StringUtils;
 import org.opensearch.sql.legacy.domain.ColumnTypeProvider;
 import org.opensearch.sql.legacy.domain.QueryActionRequest;
+import org.opensearch.sql.legacy.esdomain.LocalClusterState;
+import org.opensearch.sql.legacy.exception.SQLFeatureDisabledException;
 import org.opensearch.sql.legacy.exception.SqlParseException;
 import org.opensearch.sql.legacy.executor.Format;
 import org.opensearch.sql.legacy.executor.format.Schema;
@@ -306,15 +314,39 @@ public void aggregationQuery() throws IOException {
 
     @Test
     public void deleteSanity() throws IOException {
-        String result = explain(String.format("{\"query\":\"" +
+        try (MockedStatic<LocalClusterState> localClusterStateMockedStatic =
+            Mockito.mockStatic(LocalClusterState.class)) {
+            LocalClusterState state = mock(LocalClusterState.class);
+            localClusterStateMockedStatic.when(LocalClusterState::state).thenReturn(state);
+            when(state.getSettingValue(anyString())).thenReturn(true);
+
+            String result = explain(String.format("{\"query\":\"" +
                 "DELETE " +
                 "FROM %s " +
                 "WHERE firstname LIKE 'A%%' AND age > 20\"}", TestsConstants.TEST_INDEX_ACCOUNT));
-        String expectedOutput = Files.toString(
+            String expectedOutput = Files.toString(
                 new File(getResourcePath() + "src/test/resources/expectedOutput/delete_explain.json"), StandardCharsets.UTF_8)
                 .replaceAll("\r", "");
+            assertThat(removeSpaces(result), equalTo(removeSpaces(expectedOutput)));
+        }
+    }
 
-        assertThat(removeSpaces(result), equalTo(removeSpaces(expectedOutput)));
+    @Test(expected = SQLFeatureDisabledException.class)
+    public void deleteShouldThrowExceptionWhenDisabled()
+        throws SQLFeatureDisabledException, SQLFeatureNotSupportedException,
+        SqlParseException {
+        try (MockedStatic<LocalClusterState> localClusterStateMockedStatic =
+                 Mockito.mockStatic(LocalClusterState.class)) {
+            LocalClusterState state = mock(LocalClusterState.class);
+            localClusterStateMockedStatic.when(LocalClusterState::state).thenReturn(state);
+            when(state.getSettingValue(anyString())).thenReturn(false);
+
+            JSONObject jsonRequest = new JSONObject(StringUtils.format("{\"query\":\"" +
+                "DELETE " +
+                "FROM %s " +
+                "WHERE firstname LIKE 'A%%' AND age > 20\"}", TestsConstants.TEST_INDEX_ACCOUNT));
+            translate(jsonRequest.getString("query"), jsonRequest);
+        }
     }
 
     @Test
@@ -366,13 +398,14 @@ private String explain(String request) {
             String sql = jsonRequest.getString("query");
 
             return translate(sql, jsonRequest);
-        } catch (SqlParseException | SQLFeatureNotSupportedException e) {
+        } catch (SqlParseException | SQLFeatureNotSupportedException | SQLFeatureDisabledException e) {
             throw new ParserException("Illegal sql expr in request: " + request);
         }
     }
 
-    private String translate(String sql, JSONObject jsonRequest) throws SQLFeatureNotSupportedException, SqlParseException {
-        Client mockClient = Mockito.mock(Client.class);
+    private String translate(String sql, JSONObject jsonRequest)
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
+        Client mockClient = mock(Client.class);
         CheckScriptContents.stubMockClient(mockClient);
         QueryAction queryAction =
                 OpenSearchActionFactory

legacy/src/test/java/org/opensearch/sql/legacy/unittest/QueryFunctionsTest.java

@@ -48,6 +48,7 @@
 import org.opensearch.index.query.AbstractQueryBuilder;
 import org.opensearch.index.query.MultiMatchQueryBuilder;
 import org.opensearch.search.builder.SearchSourceBuilder.ScriptField;
+import org.opensearch.sql.legacy.exception.SQLFeatureDisabledException;
 import org.opensearch.sql.legacy.exception.SqlParseException;
 import org.opensearch.sql.legacy.query.OpenSearchActionFactory;
 import org.opensearch.sql.legacy.util.CheckScriptContents;
@@ -283,27 +284,32 @@ public void isNullWithMathExpr() {
     }
 
     @Test(expected = SQLFeatureNotSupportedException.class)
-    public void emptyQueryShouldThrowSQLFeatureNotSupportedException() throws SQLFeatureNotSupportedException, SqlParseException {
+    public void emptyQueryShouldThrowSQLFeatureNotSupportedException()
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
         OpenSearchActionFactory.create(Mockito.mock(Client.class), "");
     }
 
     @Test(expected = SQLFeatureNotSupportedException.class)
-    public void emptyNewLineQueryShouldThrowSQLFeatureNotSupportedException() throws SQLFeatureNotSupportedException, SqlParseException {
+    public void emptyNewLineQueryShouldThrowSQLFeatureNotSupportedException()
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
         OpenSearchActionFactory.create(Mockito.mock(Client.class), "\n");
     }
 
     @Test(expected = SQLFeatureNotSupportedException.class)
-    public void emptyNewLineQueryShouldThrowSQLFeatureNotSupportedException2() throws SQLFeatureNotSupportedException, SqlParseException {
+    public void emptyNewLineQueryShouldThrowSQLFeatureNotSupportedException2()
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
         OpenSearchActionFactory.create(Mockito.mock(Client.class), "\r\n");
     }
 
     @Test(expected = SQLFeatureNotSupportedException.class)
-    public void queryWithoutSpaceShouldSQLFeatureNotSupportedException() throws SQLFeatureNotSupportedException, SqlParseException {
+    public void queryWithoutSpaceShouldSQLFeatureNotSupportedException()
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
         OpenSearchActionFactory.create(Mockito.mock(Client.class), "SELE");
     }
 
     @Test(expected = SQLFeatureNotSupportedException.class)
-    public void spacesOnlyQueryShouldThrowSQLFeatureNotSupportedException() throws SQLFeatureNotSupportedException, SqlParseException {
+    public void spacesOnlyQueryShouldThrowSQLFeatureNotSupportedException()
+        throws SQLFeatureNotSupportedException, SqlParseException, SQLFeatureDisabledException {
         OpenSearchActionFactory.create(Mockito.mock(Client.class), "      ");
     }