Skip to content

Commit

Permalink
delete InvalidArgumentException and change illegalArgumentException t…
Browse files Browse the repository at this point in the history
…o SettingsException

Signed-off-by: Xue Zhou <xuezhou@amazon.com>
  • Loading branch information
xuezhou25 committed Oct 19, 2022
1 parent d0d7465 commit 6ec459b
Show file tree
Hide file tree
Showing 15 changed files with 95 additions and 127 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,10 @@ public void testClusterUpdateSettingNonExistent() {
highLevelClient().cluster()::putSettingsAsync
)
);
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST));
assertThat(
exception.getMessage(),
equalTo("OpenSearch exception [type=invalid_argument_exception, reason=transient setting [" + setting + "], not recognized]")
equalTo("OpenSearch exception [type=settings_exception, reason=transient setting [" + setting + "], not recognized]")
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1437,8 +1437,7 @@ public void testIndexPutSettings() throws IOException {
assertThat(
exception.getMessage(),
startsWith(
"OpenSearch exception [type=invalid_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"
)
);
}
Expand Down Expand Up @@ -1471,11 +1470,11 @@ public void testIndexPutSettingNonExistent() throws IOException {
highLevelClient().indices()::putSettingsAsync
)
);
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST));
assertThat(
exception.getMessage(),
equalTo(
"OpenSearch exception [type=invalid_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]"
)
Expand Down
28 changes: 0 additions & 28 deletions server/src/main/java/org/opensearch/InvalidArgumentException.java

This file was deleted.

6 changes: 0 additions & 6 deletions server/src/main/java/org/opensearch/OpenSearchException.java
Original file line number Diff line number Diff line change
Expand Up @@ -1613,12 +1613,6 @@ private enum OpenSearchExceptionHandle {
org.opensearch.cluster.decommission.NodeDecommissionedException::new,
164,
V_3_0_0
),
INVALID_ARGUMENT_EXCEPTION(
org.opensearch.InvalidArgumentException.class,
org.opensearch.InvalidArgumentException::new,
165,
V_3_0_0
);

final Class<? extends OpenSearchException> exceptionClass;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import org.opensearch.common.regex.Regex;
import org.opensearch.common.settings.IndexScopedSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.settings.SettingsException;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.common.util.set.Sets;
import org.opensearch.common.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -220,7 +221,7 @@ ClusterState addComponentTemplate(
) throws Exception {
final ComponentTemplate existing = currentState.metadata().componentTemplates().get(name);
if (create && existing != null) {
throw new IllegalArgumentException("component template [" + name + "] already exists");
throw new SettingsException("component template [" + name + "] already exists");
}

CompressedXContent mappings = template.template().mappings();
Expand Down Expand Up @@ -256,7 +257,7 @@ ClusterState addComponentTemplate(
}
}
if (globalTemplatesThatUseThisComponent.isEmpty() == false) {
throw new IllegalArgumentException(
throw new SettingsException(
"cannot update component template ["
+ name
+ "] because the following global templates would resolve to specifying the ["
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.apache.lucene.search.spell.LevenshteinDistance;
import org.apache.lucene.util.CollectionUtil;
import org.opensearch.ExceptionsHelper;
import org.opensearch.InvalidArgumentException;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.regex.Regex;

Expand Down Expand Up @@ -100,16 +99,14 @@ protected AbstractScopedSettings(
Map<String, Setting<?>> keySettings = new HashMap<>();
for (Setting<?> setting : settingsSet) {
if (setting.getProperties().contains(scope) == false) {
throw new InvalidArgumentException(
"Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties()
);
throw new SettingsException("Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties());
}
validateSettingKey(setting);

if (setting.hasComplexMatcher()) {
Setting<?> overlappingSetting = findOverlappingSetting(setting, complexMatchers);
if (overlappingSetting != null) {
throw new InvalidArgumentException(
throw new SettingsException(
"complex setting key: ["
+ setting.getKey()
+ "] overlaps existing setting key: ["
Expand All @@ -130,7 +127,7 @@ protected void validateSettingKey(Setting<?> setting) {
if (isValidKey(setting.getKey()) == false
&& (setting.isGroupSetting() && isValidGroupKey(setting.getKey()) || isValidAffixKey(setting.getKey())) == false
|| setting.getKey().endsWith(".0")) {
throw new InvalidArgumentException("illegal settings key: [" + setting.getKey() + "]");
throw new SettingsException("illegal settings key: [" + setting.getKey() + "]");
}
}

Expand Down Expand Up @@ -232,7 +229,7 @@ public synchronized Settings applySettings(Settings newSettings) {
*/
public synchronized <T> void addSettingsUpdateConsumer(Setting<T> setting, Consumer<T> consumer, Consumer<T> validator) {
if (setting != get(setting.getKey())) {
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]");
}
addSettingsUpdater(setting.newUpdater(consumer, logger, validator));
}
Expand Down Expand Up @@ -395,7 +392,7 @@ public void apply(Map<String, Settings> values, Settings current, Settings previ
private void ensureSettingIsRegistered(Setting.AffixSetting<?> setting) {
final Setting<?> registeredSetting = this.complexMatchers.get(setting.getKey());
if (setting != registeredSetting) {
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]");
}
}

Expand All @@ -411,7 +408,7 @@ public synchronized <T> void addAffixMapUpdateConsumer(
) {
final Setting<?> registeredSetting = this.complexMatchers.get(setting.getKey());
if (setting != registeredSetting) {
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]");
}
addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator));
}
Expand Down Expand Up @@ -444,10 +441,10 @@ public synchronized <A, B> void addSettingsUpdateConsumer(
BiConsumer<A, B> validator
) {
if (a != get(a.getKey())) {
throw new InvalidArgumentException("Setting is not registered for key [" + a.getKey() + "]");
throw new SettingsException("Setting is not registered for key [" + a.getKey() + "]");
}
if (b != get(b.getKey())) {
throw new InvalidArgumentException("Setting is not registered for key [" + b.getKey() + "]");
throw new SettingsException("Setting is not registered for key [" + b.getKey() + "]");
}
addSettingsUpdater(Setting.compoundUpdater(consumer, validator, a, b, logger));
}
Expand Down Expand Up @@ -544,7 +541,7 @@ public final void validate(
* @param key the key of the setting to validate
* @param settings the settings
* @param validateDependencies true if dependent settings should be validated
* @throws IllegalArgumentException if the setting is invalid
* @throws SettingsException if the setting is invalid
*/
void validate(final String key, final Settings settings, final boolean validateDependencies) {
validate(key, settings, validateDependencies, false);
Expand All @@ -557,7 +554,7 @@ void validate(final String key, final Settings settings, final boolean validateD
* @param settings the settings
* @param validateDependencies true if dependent settings should be validated
* @param validateInternalOrPrivateIndex true if internal index settings should be validated
* @throws IllegalArgumentException if the setting is invalid
* @throws SettingsException if the setting is invalid
*/
void validate(
final String key,
Expand Down Expand Up @@ -589,7 +586,7 @@ void validate(
msg += " please check that any required plugins are installed, or check the breaking changes documentation for removed "
+ "settings";
}
throw new InvalidArgumentException(msg);
throw new SettingsException(msg);
} else {
Set<Setting.SettingDependency> settingsDependencies = setting.getSettingsDependencies(key);
if (setting.hasComplexMatcher()) {
Expand All @@ -606,7 +603,7 @@ void validate(
dependency.getKey(),
setting.getKey()
);
throw new InvalidArgumentException(message);
throw new SettingsException(message);
}
// validate the dependent setting value
settingDependency.validate(setting.getKey(), setting.get(settings), dependency.get(settings));
Expand All @@ -615,11 +612,11 @@ void validate(
// the only time that validateInternalOrPrivateIndex should be true is if this call is coming via the update settings API
if (validateInternalOrPrivateIndex) {
if (setting.isInternalIndex()) {
throw new InvalidArgumentException(
throw new SettingsException(
"can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API"
);
} else if (setting.isPrivateIndex()) {
throw new InvalidArgumentException(
throw new SettingsException(
"can not update private setting [" + setting.getKey() + "]; this setting is managed by OpenSearch"
);
}
Expand Down Expand Up @@ -766,12 +763,12 @@ public Settings diff(Settings source, Settings defaultSettings) {
*/
public <T> T get(Setting<T> setting) {
if (setting.getProperties().contains(scope) == false) {
throw new InvalidArgumentException(
throw new SettingsException(
"settings scope doesn't match the setting scope [" + this.scope + "] not in [" + setting.getProperties() + "]"
);
}
if (get(setting.getKey()) == null) {
throw new InvalidArgumentException("setting " + setting.getKey() + " has not been registered");
throw new SettingsException("setting " + setting.getKey() + " has not been registered");
}
return setting.get(this.lastSettingsApplied, settings);
}
Expand All @@ -780,7 +777,7 @@ public <T> T get(Setting<T> setting) {
* Updates a target settings builder with new, updated or deleted settings from a given settings builder.
* <p>
* Note: This method will only allow updates to dynamic settings. if a non-dynamic setting is updated an
* {@link IllegalArgumentException} is thrown instead.
* {@link SettingsException} is thrown instead.
* </p>
*
* @param toApply the new settings to apply
Expand Down Expand Up @@ -844,17 +841,17 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin
toRemove.add(key);
// we don't set changed here it's set after we apply deletes below if something actually changed
} else if (get(key) == null) {
throw new InvalidArgumentException(type + " setting [" + key + "], not recognized");
throw new SettingsException(type + " setting [" + key + "], not recognized");
} else if (isDelete == false && canUpdate.test(key)) {
get(key).validateWithoutDependencies(toApply); // we might not have a full picture here do to a dependency validation
settingsBuilder.copy(key, toApply);
updates.copy(key, toApply);
changed |= toApply.get(key).equals(target.get(key)) == false;
} else {
if (isFinalSetting(key)) {
throw new InvalidArgumentException("final " + type + " setting [" + key + "], not updateable");
throw new SettingsException("final " + type + " setting [" + key + "], not updateable");
} else {
throw new InvalidArgumentException(type + " setting [" + key + "], not dynamically updateable");
throw new SettingsException(type + " setting [" + key + "], not dynamically updateable");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
Map<String, Settings> groups = s.getAsGroups();
for (String key : SimilarityService.BUILT_IN.keySet()) {
if (groups.containsKey(key)) {
throw new IllegalArgumentException(
throw new SettingsException(
"illegal value for [index.similarity." + key + "] cannot redefine built-in similarity"
);
}
Expand Down Expand Up @@ -247,7 +247,7 @@ public IndexScopedSettings copy(Settings settings, IndexMetadata metadata) {
@Override
protected void validateSettingKey(Setting setting) {
if (setting.getKey().startsWith("index.") == false) {
throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "] must start with [index.]");
throw new SettingsException("illegal settings key: [" + setting.getKey() + "] must start with [index.]");
}
super.validateSettingKey(setting);
}
Expand Down
11 changes: 3 additions & 8 deletions server/src/main/java/org/opensearch/common/settings/Setting.java
Original file line number Diff line number Diff line change
Expand Up @@ -496,15 +496,10 @@ private T get(Settings settings, boolean validate) {
}
return parsed;
} catch (OpenSearchParseException ex) {
throw new IllegalArgumentException(ex.getMessage(), ex);
} catch (NumberFormatException ex) {
throw new SettingsException(ex.getMessage(), ex);
} catch (Exception ex) {
String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]";
throw new IllegalArgumentException(err, ex);
} catch (IllegalArgumentException ex) {
throw ex;
} catch (Exception t) {
String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]";
throw new IllegalArgumentException(err, t);
throw new SettingsException(err, ex);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import org.opensearch.OpenSearchException;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.rest.RestStatus;

import java.io.IOException;

Expand All @@ -59,4 +60,9 @@ public SettingsException(StreamInput in) throws IOException {
public SettingsException(String msg, Object... args) {
super(msg, args);
}

@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.opensearch.common.settings.ClusterSettings;
import org.opensearch.common.settings.IndexScopedSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.settings.SettingsException;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
import org.opensearch.common.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -378,8 +379,8 @@ public void testAddComponentTemplate() throws Exception {
assertThat(state.metadata().componentTemplates().get("foo"), equalTo(componentTemplate));

final ClusterState throwState = ClusterState.builder(state).build();
IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
SettingsException e = expectThrows(
SettingsException.class,
() -> metadataIndexTemplateService.addComponentTemplate(throwState, true, "foo", componentTemplate)
);
assertThat(e.getMessage(), containsString("component template [foo] already exists"));
Expand Down Expand Up @@ -416,7 +417,7 @@ public void testAddComponentTemplate() throws Exception {
);
ComponentTemplate componentTemplate4 = new ComponentTemplate(template, 1L, new HashMap<>());
expectThrows(
IllegalArgumentException.class,
SettingsException.class,
() -> metadataIndexTemplateService.addComponentTemplate(throwState, true, "foo2", componentTemplate4)
);
}
Expand Down
Loading

0 comments on commit 6ec459b

Please sign in to comment.