Improve summary error message for invalid setting updates

Signed-off-by: Xue Zhou <xuezhou@amazon.com>
opensearch-project · Feb 6, 2023 · ee8105b · ee8105b
1 parent 74912d2
commit ee8105b
Show file tree

Hide file tree

Showing 20 changed files with 131 additions and 115 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Changed http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
 - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))
 - Require MediaType in Strings.toString API ([#6009](https://github.com/opensearch-project/OpenSearch/pull/6009))
+- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
 
 ### Deprecated
 

diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java
@@ -162,7 +162,7 @@ public void testClusterUpdateSettingNonExistent() {
         assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST));
         assertThat(
             exception.getMessage(),
-            equalTo("OpenSearch exception [type=illegal_argument_exception, reason=transient setting [" + setting + "], not recognized]")
+            equalTo("OpenSearch exception [type=settings_exception, reason=transient setting [" + setting + "], not recognized]")
         );
     }
 

diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java
@@ -1437,8 +1437,7 @@ public void testIndexPutSettings() throws IOException {
         assertThat(
             exception.getMessage(),
             startsWith(
-                "OpenSearch exception [type=illegal_argument_exception, "
-                    + "reason=final index setting [index.number_of_shards], not updateable"
+                "OpenSearch exception [type=settings_exception, " + "reason=final index setting [index.number_of_shards], not updateable"
             )
         );
     }
@@ -1475,7 +1474,7 @@ public void testIndexPutSettingNonExistent() throws IOException {
         assertThat(
             exception.getMessage(),
             equalTo(
-                "OpenSearch exception [type=illegal_argument_exception, "
+                "OpenSearch exception [type=settings_exception, "
                     + "reason=unknown setting [index.no_idea_what_you_are_talking_about] please check that any required plugins are installed, "
                     + "or check the breaking changes documentation for removed settings]"
             )

diff --git a/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java b/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java
@@ -31,8 +31,10 @@
 
 package org.opensearch.cluster.metadata;
 
+import org.hamcrest.Matchers;
 import org.opensearch.common.SuppressForbidden;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.test.OpenSearchTestCase;
 
 import static org.opensearch.cluster.metadata.IndexMetadata.DEFAULT_NUMBER_OF_SHARDS;

diff --git a/...rc/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java b/...rc/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java
@@ -47,6 +47,7 @@
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.common.collect.ImmutableOpenMap;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.index.IndexNotFoundException;
@@ -82,7 +83,7 @@ public void testCreationDateGivenFails() {
         try {
             prepareCreate("test").setSettings(Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 4L)).get();
             fail();
-        } catch (IllegalArgumentException ex) {
+        } catch (SettingsException ex) {
             assertEquals(
                 "unknown setting [index.creation_date] please check that any required plugins are installed, or check the "
                     + "breaking changes documentation for removed settings",
@@ -203,7 +204,7 @@ public void testUnknownSettingFails() {
         try {
             prepareCreate("test").setSettings(Settings.builder().put("index.unknown.value", "this must fail").build()).get();
             fail("should have thrown an exception about the shard count");
-        } catch (IllegalArgumentException e) {
+        } catch (SettingsException e) {
             assertEquals(
                 "unknown setting [index.unknown.value] please check that any required plugins are installed, or check the"
                     + " breaking changes documentation for removed settings",

diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java
@@ -41,6 +41,7 @@
 import org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.common.unit.ByteSizeUnit;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.indices.recovery.RecoverySettings;
@@ -78,7 +79,7 @@ public void testClusterNonExistingSettingsUpdate() {
         try {
             client().admin().cluster().prepareUpdateSettings().setTransientSettings(Settings.builder().put(key1, value1).build()).get();
             fail("bogus value");
-        } catch (IllegalArgumentException ex) {
+        } catch (SettingsException ex) {
             assertEquals("transient setting [no_idea_what_you_are_talking_about], not recognized", ex.getMessage());
         }
     }

diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java
@@ -34,6 +34,7 @@
 
 import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.plugins.Plugin;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
@@ -64,8 +65,8 @@ public void testUpdateInternalIndexSettingViaSettingsAPI() {
         final GetSettingsResponse response = client().admin().indices().prepareGetSettings("test").get();
         assertThat(response.getSetting("test", "index.internal"), equalTo("internal"));
         // we can not update the setting via the update settings API
-        final IllegalArgumentException e = expectThrows(
-            IllegalArgumentException.class,
+        final SettingsException e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .indices()
                 .prepareUpdateSettings("test")

diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java
@@ -35,6 +35,7 @@
 import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
 import org.opensearch.common.ValidationException;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.plugins.Plugin;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
@@ -64,8 +65,8 @@ public void testSetPrivateIndexSettingOnCreate() {
     public void testUpdatePrivateIndexSettingViaSettingsAPI() {
         createIndex("test");
         // we can not update the setting via the update settings API
-        final IllegalArgumentException e = expectThrows(
-            IllegalArgumentException.class,
+        final SettingsException e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .indices()
                 .prepareUpdateSettings("test")

diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java
@@ -40,6 +40,7 @@
 import org.opensearch.common.Priority;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.index.IndexModule;
 import org.opensearch.index.IndexService;
@@ -174,52 +175,52 @@ protected Settings nodeSettings(int nodeOrdinal) {
     }
 
     public void testUpdateDependentClusterSettings() {
-        IllegalArgumentException iae = expectThrows(
-            IllegalArgumentException.class,
+        SettingsException e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .cluster()
                 .prepareUpdateSettings()
                 .setPersistentSettings(Settings.builder().put("cluster.acc.test.pw", "asdf"))
                 .get()
         );
-        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
+        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage());
 
-        iae = expectThrows(
-            IllegalArgumentException.class,
+        e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .cluster()
                 .prepareUpdateSettings()
                 .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf"))
                 .get()
         );
-        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
+        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage());
 
-        iae = expectThrows(
-            IllegalArgumentException.class,
+        e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .cluster()
                 .prepareUpdateSettings()
                 .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf"))
                 .setPersistentSettings(Settings.builder().put("cluster.acc.test.user", "asdf"))
                 .get()
         );
-        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
+        assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage());
 
         if (randomBoolean()) {
             client().admin()
                 .cluster()
                 .prepareUpdateSettings()
                 .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf").put("cluster.acc.test.user", "asdf"))
                 .get();
-            iae = expectThrows(
-                IllegalArgumentException.class,
+            e = expectThrows(
+                SettingsException.class,
                 () -> client().admin()
                     .cluster()
                     .prepareUpdateSettings()
                     .setTransientSettings(Settings.builder().putNull("cluster.acc.test.user"))
                     .get()
             );
-            assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
+            assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage());
             client().admin()
                 .cluster()
                 .prepareUpdateSettings()
@@ -232,15 +233,15 @@ public void testUpdateDependentClusterSettings() {
                 .setPersistentSettings(Settings.builder().put("cluster.acc.test.pw", "asdf").put("cluster.acc.test.user", "asdf"))
                 .get();
 
-            iae = expectThrows(
-                IllegalArgumentException.class,
+            e = expectThrows(
+                SettingsException.class,
                 () -> client().admin()
                     .cluster()
                     .prepareUpdateSettings()
                     .setPersistentSettings(Settings.builder().putNull("cluster.acc.test.user"))
                     .get()
             );
-            assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
+            assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage());
 
             client().admin()
                 .cluster()
@@ -252,11 +253,11 @@ public void testUpdateDependentClusterSettings() {
     }
 
     public void testUpdateDependentIndexSettings() {
-        IllegalArgumentException iae = expectThrows(
-            IllegalArgumentException.class,
+        SettingsException e = expectThrows(
+            SettingsException.class,
             () -> prepareCreate("test", Settings.builder().put("index.acc.test.pw", "asdf")).get()
         );
-        assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage());
+        assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage());
 
         createIndex("test");
         for (int i = 0; i < 2; i++) {
@@ -265,16 +266,16 @@ public void testUpdateDependentIndexSettings() {
                 client().admin().indices().prepareClose("test").get();
             }
 
-            iae = expectThrows(
-                IllegalArgumentException.class,
+            e = expectThrows(
+                SettingsException.class,
                 () -> client().admin()
                     .indices()
                     .prepareUpdateSettings("test")
                     .setSettings(Settings.builder().put("index.acc.test.pw", "asdf"))
                     .execute()
                     .actionGet()
             );
-            assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage());
+            assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage());
 
             // user has no dependency
             client().admin()
@@ -293,16 +294,16 @@ public void testUpdateDependentIndexSettings() {
                 .actionGet();
 
             // now try to remove it and make sure it fails
-            iae = expectThrows(
-                IllegalArgumentException.class,
+            e = expectThrows(
+                SettingsException.class,
                 () -> client().admin()
                     .indices()
                     .prepareUpdateSettings("test")
                     .setSettings(Settings.builder().putNull("index.acc.test.user"))
                     .execute()
                     .actionGet()
             );
-            assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage());
+            assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage());
 
             // now we are consistent
             client().admin()
@@ -480,8 +481,8 @@ public void testOpenCloseUpdateSettings() throws Exception {
         assertThat(indexMetadata.getSettings().get("index.refresh_interval"), equalTo("1s"));
         assertThat(indexMetadata.getSettings().get("index.fielddata.cache"), equalTo("none"));
 
-        IllegalArgumentException ex = expectThrows(
-            IllegalArgumentException.class,
+        SettingsException ex = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .indices()
                 .prepareUpdateSettings("test")

diff --git a/...r/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java b/...r/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java
@@ -48,6 +48,7 @@
 import org.opensearch.common.ParsingException;
 import org.opensearch.common.bytes.BytesArray;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.settings.SettingsException;
 import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.common.xcontent.XContentType;
 import org.opensearch.index.mapper.MapperParsingException;
@@ -494,8 +495,8 @@ public void testInvalidSettings() throws Exception {
         GetIndexTemplatesResponse response = client().admin().indices().prepareGetTemplates().get();
         assertThat(response.getIndexTemplates(), empty());
 
-        IllegalArgumentException e = expectThrows(
-            IllegalArgumentException.class,
+        SettingsException e = expectThrows(
+            SettingsException.class,
             () -> client().admin()
                 .indices()
                 .preparePutTemplate("template_1")