Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class #4792

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Backport Apache Lucene version change for 2.4.0 ([#4677](https://github.com/opensearch-project/OpenSearch/pull/4677))
- Refactor Base Action class javadocs to OpenSearch.API ([#4732](https://github.com/opensearch-project/OpenSearch/pull/4732))
- Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459))
- Mapping IllegalArgumentException to InvalidArgumentException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe what has changed for the caller instead?


### Deprecated

Expand Down Expand Up @@ -169,4 +170,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Security

[Unreleased]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...HEAD
[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x
[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x
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.BAD_REQUEST));
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
assertThat(
exception.getMessage(),
equalTo("OpenSearch exception [type=illegal_argument_exception, reason=transient setting [" + setting + "], not recognized]")
equalTo("OpenSearch exception [type=invalid_argument_exception, reason=transient setting [" + setting + "], not recognized]")
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,7 @@ public void testIndexPutSettings() throws IOException {
assertThat(
exception.getMessage(),
startsWith(
"OpenSearch exception [type=illegal_argument_exception, "
"OpenSearch exception [type=invalid_argument_exception, "
+ "reason=final index setting [index.number_of_shards], not updateable"
)
);
Expand Down Expand Up @@ -1471,11 +1471,11 @@ public void testIndexPutSettingNonExistent() throws IOException {
highLevelClient().indices()::putSettingsAsync
)
);
assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST));
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
assertThat(
exception.getMessage(),
equalTo(
"OpenSearch exception [type=illegal_argument_exception, "
"OpenSearch exception [type=invalid_argument_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: 28 additions & 0 deletions server/src/main/java/org/opensearch/InvalidArgumentException.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch;

import org.opensearch.common.io.stream.StreamInput;

import java.io.IOException;

/**
* Base exception for a missing action on a primary
*
* @opensearch.internal
*/
public class InvalidArgumentException extends OpenSearchException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the existing org.opensearch.common.settings.SettingsException class be used here and avoid creating a new one?

It is important that this case returns a BAD_REQUEST error code, so the following would need to be added to SettingsException to make that happen:

    @Override
    public RestStatus status() {
        return RestStatus.BAD_REQUEST;
    }

That might be okay because it looks like the existing usages of SettingsException would probably be better as a BAD_REQUEST, but I'm not 100% sure about that.

public InvalidArgumentException(String message) {
super(message);
}

public InvalidArgumentException(StreamInput in) throws IOException {
super(in);
}
}
6 changes: 6 additions & 0 deletions server/src/main/java/org/opensearch/OpenSearchException.java
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,12 @@ 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 @@ -38,6 +38,7 @@
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 @@ -99,7 +100,7 @@ protected AbstractScopedSettings(
Map<String, Setting<?>> keySettings = new HashMap<>();
for (Setting<?> setting : settingsSet) {
if (setting.getProperties().contains(scope) == false) {
throw new IllegalArgumentException(
throw new InvalidArgumentException(
"Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties()
);
}
Expand All @@ -108,7 +109,7 @@ protected AbstractScopedSettings(
if (setting.hasComplexMatcher()) {
Setting<?> overlappingSetting = findOverlappingSetting(setting, complexMatchers);
if (overlappingSetting != null) {
throw new IllegalArgumentException(
throw new InvalidArgumentException(
"complex setting key: ["
+ setting.getKey()
+ "] overlaps existing setting key: ["
Expand All @@ -129,7 +130,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 IllegalArgumentException("illegal settings key: [" + setting.getKey() + "]");
throw new InvalidArgumentException("illegal settings key: [" + setting.getKey() + "]");
}
}

Expand Down Expand Up @@ -231,7 +232,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 IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
}
addSettingsUpdater(setting.newUpdater(consumer, logger, validator));
}
Expand Down Expand Up @@ -394,7 +395,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 IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
}
}

Expand All @@ -410,7 +411,7 @@ public synchronized <T> void addAffixMapUpdateConsumer(
) {
final Setting<?> registeredSetting = this.complexMatchers.get(setting.getKey());
if (setting != registeredSetting) {
throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
}
addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator));
}
Expand Down Expand Up @@ -443,10 +444,10 @@ public synchronized <A, B> void addSettingsUpdateConsumer(
BiConsumer<A, B> validator
) {
if (a != get(a.getKey())) {
throw new IllegalArgumentException("Setting is not registered for key [" + a.getKey() + "]");
throw new InvalidArgumentException("Setting is not registered for key [" + a.getKey() + "]");
}
if (b != get(b.getKey())) {
throw new IllegalArgumentException("Setting is not registered for key [" + b.getKey() + "]");
throw new InvalidArgumentException("Setting is not registered for key [" + b.getKey() + "]");
}
addSettingsUpdater(Setting.compoundUpdater(consumer, validator, a, b, logger));
}
Expand Down Expand Up @@ -588,7 +589,7 @@ void validate(
msg += " please check that any required plugins are installed, or check the breaking changes documentation for removed "
+ "settings";
}
throw new IllegalArgumentException(msg);
throw new InvalidArgumentException(msg);
} else {
Set<Setting.SettingDependency> settingsDependencies = setting.getSettingsDependencies(key);
if (setting.hasComplexMatcher()) {
Expand All @@ -605,7 +606,7 @@ void validate(
dependency.getKey(),
setting.getKey()
);
throw new IllegalArgumentException(message);
throw new InvalidArgumentException(message);
}
// validate the dependent setting value
settingDependency.validate(setting.getKey(), setting.get(settings), dependency.get(settings));
Expand All @@ -614,11 +615,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 IllegalArgumentException(
throw new InvalidArgumentException(
"can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API"
);
} else if (setting.isPrivateIndex()) {
throw new IllegalArgumentException(
throw new InvalidArgumentException(
"can not update private setting [" + setting.getKey() + "]; this setting is managed by OpenSearch"
);
}
Expand Down Expand Up @@ -765,12 +766,12 @@ public Settings diff(Settings source, Settings defaultSettings) {
*/
public <T> T get(Setting<T> setting) {
if (setting.getProperties().contains(scope) == false) {
throw new IllegalArgumentException(
throw new InvalidArgumentException(
"settings scope doesn't match the setting scope [" + this.scope + "] not in [" + setting.getProperties() + "]"
);
}
if (get(setting.getKey()) == null) {
throw new IllegalArgumentException("setting " + setting.getKey() + " has not been registered");
throw new InvalidArgumentException("setting " + setting.getKey() + " has not been registered");
}
return setting.get(this.lastSettingsApplied, settings);
}
Expand Down Expand Up @@ -843,17 +844,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 IllegalArgumentException(type + " setting [" + key + "], not recognized");
throw new InvalidArgumentException(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 IllegalArgumentException("final " + type + " setting [" + key + "], not updateable");
throw new InvalidArgumentException("final " + type + " setting [" + key + "], not updateable");
} else {
throw new IllegalArgumentException(type + " setting [" + key + "], not dynamically updateable");
throw new InvalidArgumentException(type + " setting [" + key + "], not dynamically updateable");
}
}
}
Expand Down