[Extensions] Adds a parameter to Setting Validator to toggle regex matching #6823

Kuanysh-kst · 2023-03-24T04:42:01Z

Description

Update the validator to take a boolean parameter switching between "fail if it matches" or "fail if it doesn't match". This provides the most flexibility.

Issues Resolved

opensearch-project/opensearch-sdk-java#591

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

github-actions · 2023-03-24T05:13:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12868/
CommitID: 9a58652
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis

Thanks for contributing, this will be a good functionality!

A few changes needed as noted below.

Also:

You have named the PR matching the issue (bug) you're working on, but since that's in a different repo, the maintainers here may not understand the subject. And this isn't actually fixing the bug it's adding a feature to make the bug fix easier on the other Repo! So I'd suggest changing your title to describe what this change actually does, something like "Adds a parameter to Setting Validator to toggle regex matching".
There are test cases for this function in a test class somewhere (probably SettingTests). Find them and update the tests to work with the new parameters.
Make sure you check all the boxes in the first comment (one of which is adding tests and that they pass!)

dbwiddis · 2023-03-24T06:18:00Z

server/src/main/java/org/opensearch/common/settings/Setting.java

-                throw new IllegalArgumentException("Setting [" + value + "] does not match regex [" + pattern.pattern() + "]");
+            if (isMatching){
+                if (!pattern.matcher(value).matches()) {
+                    throw new IllegalArgumentException("Setting must not contain [" + pattern.pattern() + "]");


You have the same error for both cases here. This one should keep the old "does not match" pattern.

dbwiddis · 2023-03-24T06:19:03Z

server/src/main/java/org/opensearch/common/settings/Setting.java

+                }
+            }else {
+                if (pattern.matcher(value).matches()) {
+                    throw new IllegalArgumentException("Setting must not contain [" + pattern.pattern() + "]");


Don't be specific about "contain". Use a variant of the previous message, probably "Setting [value] must match regex [pattern]"

dbwiddis · 2023-03-24T06:21:24Z

server/src/main/java/org/opensearch/common/settings/Setting.java

        /**
         * @param regex A regular expression containing the only valid input for this setting.
         */
        public RegexValidator(String regex) {
            this.pattern = Pattern.compile(regex);
        }

+        /**
+         * @param regex A regular expression containing the only valid input for this setting.
+         * @param isMatching this is a boolean switching between "fail, if it matches" or "fail, if it doesn't match".


This javadoc doesn't tell me which is which. It should be clear what true means and what false means.

Something like "If true, the setting must match the given regex. If false, the setting must not match the given regex."

dbwiddis · 2023-03-24T06:24:31Z

server/src/main/java/org/opensearch/common/settings/Setting.java

        /**
         * @param regex A regular expression containing the only valid input for this setting.
         */
        public RegexValidator(String regex) {
            this.pattern = Pattern.compile(regex);
        }

+        /**
+         * @param regex A regular expression containing the only valid input for this setting.


This sentence is no longer true all the time with the boolean. Consider adding a summary statement like "Constructs a validator based on a regular expression." and then this param can just be "the regular expression".

dbwiddis · 2023-03-24T06:25:31Z

server/src/main/java/org/opensearch/common/settings/Setting.java

@@ -1256,13 +1256,24 @@ private static boolean isFiltered(Property[] properties) {
    public static class RegexValidator implements Writeable, Validator<String> {
        private Pattern pattern;

+        private boolean isMatching = true;
+
        /**
         * @param regex A regular expression containing the only valid input for this setting.
         */
        public RegexValidator(String regex) {
            this.pattern = Pattern.compile(regex);


(Style) This (still) works, but the pattern in most of this class is to overload constructors to call other constructors. So you could use this(regex, true) here, and then not pre-initialize the boolean.

Also probably want to make a similar summary statement as the one you are adding in the constructor below.

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

github-actions · 2023-03-24T15:40:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12882/
CommitID: dd646a6
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis · 2023-03-25T16:54:22Z

server/src/main/java/org/opensearch/common/settings/Setting.java

+
+        /**
+         * @param regex constructs a validator based on a regular expression.
+         * @param isMatching If true, the setting must match the given regex. If false, the setting must not match the given regex..


Nit: the .. at the end.

dbwiddis · 2023-03-25T16:55:33Z

server/src/main/java/org/opensearch/common/settings/Setting.java

@@ -1273,14 +1285,21 @@ Pattern getPattern() {

        @Override
        public void validate(String value) {
-            if (!pattern.matcher(value).matches()) {
-                throw new IllegalArgumentException("Setting [" + value + "] does not match regex [" + pattern.pattern() + "]");
+            if (isMatching){


The logic here is good, but all the nesting can be simplified.

if (isMatching && !pattern.matcher(value).matches()) { first throw } else if (pattern.matcher(value).matches()) { second throw }

dbwiddis · 2023-03-25T16:57:29Z

server/src/test/java/org/opensearch/common/settings/SettingTests.java

@@ -329,12 +329,18 @@ public void testRegexValidator() throws Exception {
        String expectedRegex = "\\d+";
        Pattern expectedPattern = Pattern.compile(expectedRegex);
        RegexValidator regexValidator = new RegexValidator(expectedRegex);
+        RegexValidator regexValidatorMatcher = new RegexValidator(expectedRegex,false);


This looks great for testing the false case, but can we add a test for the true case?

Also, I suspect spotless is going to complain when CI is run here. Make sure you always run ./gradlew spotlessApply before pushing your commits here.

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

github-actions · 2023-03-26T16:37:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12932/
CommitID: 711a2d8
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis

Just a few more tweaks!

dbwiddis · 2023-03-29T16:12:23Z

server/src/test/java/org/opensearch/common/settings/SettingTests.java

@@ -329,12 +329,24 @@ public void testRegexValidator() throws Exception {
        String expectedRegex = "\\d+";
        Pattern expectedPattern = Pattern.compile(expectedRegex);
        RegexValidator regexValidator = new RegexValidator(expectedRegex);
+        RegexValidator regexValidatorMatcherFalse = new RegexValidator(expectedRegex, false);
+        RegexValidator regexValidatorMatcherTrue = new RegexValidator(expectedRegex, true);


This duplicates the regexValidator on line 331, so you don't need it.

dbwiddis · 2023-03-29T16:12:59Z

server/src/test/java/org/opensearch/common/settings/SettingTests.java

+        assertNotNull(regexValidatorMatcherFalse.getPattern());
+        assertEquals(expectedPattern.pattern(), regexValidatorMatcherFalse.getPattern().pattern());
+
+        // Test that checks the pattern and isMatching with the set value true parameters are working correctly during initialization


IF you delete line 333 you don't need these tests.

dbwiddis · 2023-03-29T16:14:05Z

server/src/test/java/org/opensearch/common/settings/SettingTests.java

+        assertNotNull(regexValidatorMatcherTrue);
+        assertNotNull(regexValidatorMatcherTrue);
+        assertEquals(expectedPattern.pattern(), regexValidatorMatcherTrue.getPattern().pattern());
+
        // Test that validate() throws an exception for invalid input
        final RegexValidator finalValidator = new RegexValidator(expectedRegex);


You need to make a copy of these tests (line 351-358) for regexValidatorMatcherFalse. You should get the opposite behavior (exception when it matches and no exception when it doesn't match).

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

github-actions · 2023-03-30T13:37:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.shrink/30_copy_settings/Copy settings during shrink index}

URL: https://build.ci.opensearch.org/job/gradle-check/13101/
CommitID: a83da78
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2023-03-30T13:41:33Z

Codecov Report

Merging #6823 (15d5ba8) into main (4d78bbf) will increase coverage by 0.13%.
The diff coverage is 77.77%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6823      +/-   ##
============================================
+ Coverage     70.65%   70.78%   +0.13%     
- Complexity    59184    59221      +37     
============================================
  Files          4810     4810              
  Lines        283487   283494       +7     
  Branches      40877    40878       +1     
============================================
+ Hits         200299   200674     +375     
+ Misses        66761    66358     -403     
- Partials      16427    16462      +35

Impacted Files	Coverage Δ
...n/java/org/opensearch/common/settings/Setting.java	`86.20% <77.77%> (-0.12%)`	⬇️

... and 500 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

github-actions · 2023-03-30T17:33:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      2 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.split/30_copy_settings/Copy settings during split index}

URL: https://build.ci.opensearch.org/job/gradle-check/13112/
CommitID: 15d5ba8
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

dbwiddis

LGTM!

…tching (#6823) (cherry picked from commit c765e3a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…tching (#6823) (#7054) (cherry picked from commit c765e3a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…tching (opensearch-project#6823)

add boolean parametr isMatching

9a58652

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

Kuanysh-kst requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners March 24, 2023 04:42

github-actions bot added the first-time-contributor label Mar 24, 2023

Kuanysh-kst mentioned this pull request Mar 24, 2023

[BUG] Hello World sample validator reverses logic opensearch-project/opensearch-sdk-java#591

Closed

dbwiddis requested changes Mar 24, 2023

View reviewed changes

Kuanysh-kst changed the title ~~[BUG] Hello World sample validator reverses logic~~ Adds a parameter to Setting Validator to toggle regex matching Mar 24, 2023

code refactoring and adding tests

dd646a6

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

dbwiddis added the CCI College Contributor Initiative related issues and PRs label Mar 24, 2023

Kuanysh-kst mentioned this pull request Mar 25, 2023

Changed the test for RegexValidator class opensearch-project/opensearch-sdk-java#608

Merged

dbwiddis reviewed Mar 25, 2023

View reviewed changes

add test if isMatching is false

711a2d8

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

dbwiddis requested changes Mar 29, 2023

View reviewed changes

add test for false parametr

a83da78

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

changed matcher() to find()

15d5ba8

Signed-off-by: Kuanysh <kuanysh4646@gmail.com>

Kuanysh-kst requested a review from dbwiddis April 3, 2023 09:52

dbwiddis approved these changes Apr 5, 2023

View reviewed changes

dbwiddis added the skip-changelog label Apr 5, 2023

Kuanysh-kst changed the title ~~Adds a parameter to Setting Validator to toggle regex matching~~ [Extensions] Adds a parameter to Setting Validator to toggle regex matching Apr 6, 2023

ryanbogan approved these changes Apr 6, 2023

View reviewed changes

owaiskazi19 approved these changes Apr 6, 2023

View reviewed changes

owaiskazi19 merged commit c765e3a into opensearch-project:main Apr 7, 2023

ryanbogan added the backport 2.x Backport to 2.x branch label Apr 7, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 7, 2023

[Backport 2.x] [Extensions] Adds a parameter to Setting Validator to toggle regex matching #7054

Merged

austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Apr 28, 2023

[Extensions] Adds a parameter to Setting Validator to toggle regex ma…

9bc0e96

…tching (opensearch-project#6823)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extensions] Adds a parameter to Setting Validator to toggle regex matching #6823

[Extensions] Adds a parameter to Setting Validator to toggle regex matching #6823

Kuanysh-kst commented Mar 24, 2023 •

edited

Loading

github-actions bot commented Mar 24, 2023

dbwiddis left a comment

dbwiddis Mar 24, 2023

dbwiddis Mar 24, 2023

dbwiddis Mar 24, 2023

dbwiddis Mar 24, 2023

dbwiddis Mar 24, 2023

github-actions bot commented Mar 24, 2023

dbwiddis Mar 25, 2023

dbwiddis Mar 25, 2023

dbwiddis Mar 25, 2023

github-actions bot commented Mar 26, 2023

dbwiddis left a comment

dbwiddis Mar 29, 2023

dbwiddis Mar 29, 2023

dbwiddis Mar 29, 2023

github-actions bot commented Mar 30, 2023

codecov-commenter commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023

dbwiddis left a comment

[Extensions] Adds a parameter to Setting Validator to toggle regex matching #6823

[Extensions] Adds a parameter to Setting Validator to toggle regex matching #6823

Conversation

Kuanysh-kst commented Mar 24, 2023 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Mar 24, 2023

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 24, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2023

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 30, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Mar 30, 2023 • edited Loading

Codecov Report

github-actions bot commented Mar 30, 2023

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

Kuanysh-kst commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 30, 2023 •

edited

Loading