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

[BUG] Hello World sample validator reverses logic #591

Closed
dbwiddis opened this issue Mar 20, 2023 · 6 comments
Closed

[BUG] Hello World sample validator reverses logic #591

dbwiddis opened this issue Mar 20, 2023 · 6 comments
Assignees
Labels
bug Something isn't working CCI Part of the College Contributor Initiative good first issue Good for newcomers help wanted Extra attention is needed

Comments

@dbwiddis
Copy link
Member

What is the bug?

A sample validated setting was added to the HelloWorld extension in #421. It is based on original assumptions that a matching validator would fail. During review of the companion PR, the implementation reversed the logic so it now does the opposite of what's intended.

The unit test was missing the @Test annotation so it wasn't being adequately tested, as I noticed when reviewing code coverage.

How can one reproduce the bug?

  1. Add the @Test annotation to TestHelloWorldExtension method testValidatedSettings().
  2. Run gradle check

What is the expected behavior?

Tests pass.

Do you have any additional context?

Possible resolutions:

  1. (Easy) Change the regular expression from "forbidden" to one that verifies a word doesn't exist. This is a confusing regex: "^((?!forbidden).)*$"
  2. (Preferred) 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.

Bonus points:

  • Add more validated settings like IP address matching
@dbwiddis dbwiddis added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed backport 1.x labels Mar 20, 2023
@Kuanysh-kst
Copy link
Contributor

Hello @dbwiddis, I would like to take this issue

@dbwiddis
Copy link
Member Author

Great! It's yours! This is one where you will need to create two PRs, one here and one on OpenSearch. On your local checkout you will make the change on OpenSearch branch and use ./gradlew publishToMavenLocal to have a copy that will let your SDK branch compile/test.

@Kuanysh-kst
Copy link
Contributor

Kuanysh-kst commented Mar 23, 2023

In the test testValidatedSettings() method, I get an error of 246 lines when I run Junit Test, I think I can temporarily remove the first 3 lines of this method to make this issue(I get an error because of this line: final String expected = randomAlphaOfLengthBetween(1, 5);)

@dbwiddis
Copy link
Member Author

I think I can temporarily remove the first 3 lines of this method to make this issue(I get an error because of this line: final String expected = randomAlphaOfLengthBetween(1, 5);)

Yes, just remove that. It works on OpenSearch but we don't have the right setup for it to work for us. Just pick a test string of your own.

@Kuanysh-kst
Copy link
Contributor

@dbwiddis dbwiddis added the CCI Part of the College Contributor Initiative label Mar 24, 2023
@dbwiddis
Copy link
Member Author

Done in #608 and opensearch-project/OpenSearch#6823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CCI Part of the College Contributor Initiative good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants