Makes route prefix setting routeNamePrefix optional

[DarshitChanpura]

Description

Describe what this change achieves.

Issues Resolved

#827 (comment)

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.

[dbwiddis]

While renaming will "fix" the sample, it doesn't solve the underlying problem:

1. There is no documentation to users on what is a valid name (and the OpenSearch error message isn't helpful enough to debug the problem without referencing the source code)
2. It disallows the existing names for every plugin, many of which we would migrate, e.g., opensearch-alerting, opensearch-anomaly-detection, opensearch-knn, opensearch-ml, etc.

The name here isn't actually used for anything programmatic. It's purely decorative and is only used as the response to the initialization command where it is displayed in a log message, and as the "node name". Node names in OpenSearch use hyphens.

Our test cases still use sample-extension as well, and don't fail on an illegal name there. If we're really set on disallowing a hyphen those need to validate the names.

I strongly prefer one of the following two options:

1. Simply remove the routePrefix() method and replace it with a constant in each REST handler, or a local method in a REST handler that uses an allowed prefix
2. If you keep the route prefix method, add a setRoutePrefix() method that validates it's a legal prefix.

[DarshitChanpura]

While renaming will "fix" the sample, it doesn't solve the underlying problem:

1. There is no documentation to users on what is a valid name (and the OpenSearch error message isn't helpful enough to debug the problem without referencing the source code)
2. It disallows the existing names for every plugin, many of which we would migrate, e.g., opensearch-alerting, opensearch-anomaly-detection, opensearch-knn, opensearch-ml, etc.

The name here isn't actually used for anything programmatic. It's purely decorative and is only used as the response to the initialization command where it is displayed in a log message, and as the "node name". Node names in OpenSearch use hyphens.

Our test cases still use sample-extension as well, and don't fail on an illegal name there. If we're really set on disallowing a hyphen those need to validate the names.

I strongly prefer one of the following two options:

1. Simply remove the routePrefix() method and replace it with a constant in each REST handler, or a local method in a REST handler that uses an allowed prefix
2. If you keep the route prefix method, add a setRoutePrefix() method that validates it's a legal prefix.

I agree. However routePrefix only comes into play if an extension developer decides to use it while registering a route.

i.e. HelloWorld extension can also be registered without calling routePrefix method.

new NamedRoute.Builder().method(GET)
                .path("/hello")
                .handler(handleGetRequest)
                .uniqueName(routePrefix("greet"))   -----> can be written as `.uniqueName("greet")` OR `.uniqueName("helloWorld:greet")`
                .legacyActionNames(Set.of("cluster:admin/opensearch/hw/greet"))
                .build()

This would still work. routePrefix is completely optional. I would update the javaDoc for routePrefix to state that valid extension name patterns are "this" and then add a check to validate this. Would that work? If not I can remove all references to routePrefix.

[DarshitChanpura]

Added a validation check when extensionName is set to be later used by routePrefix

[DarshitChanpura]

@dbwiddis I've update the routePrefix setting to be completely optional in extension-settings.yml

cc: @joshpalis @owaiskazi19

[owaiskazi19]

@DarshitChanpura I'm still confused, isn't this PR purpose is to add hyphen to the extension name?
Since we want to support other plugins like: opensearch-alerting, opensearch-anomaly-detection, opensearch-knn, opensearch-ml.

[cwperks]

If this is about adding support for hyphen then why not update the pattern here?

[DarshitChanpura]

@DarshitChanpura I'm still confused, isn't this PR purpose is to add hyphen to the extension name? Since we want to support other plugins like: opensearch-alerting, opensearch-anomaly-detection, opensearch-knn, opensearch-ml.

We are not meddling with extension name any more. This PR talks about a new optional setting routeNamePrefix which has to be alphanumeric and can contain _ only. This lifts the restriction from the extension name setting.

[DarshitChanpura]

If this is about adding support for hyphen then why not update the pattern here?

This can be addressed in future PRs. As of now there are no restrictions on the way extension is named. The restrictions only apply to routeNamePrefix setting here and when the route name is set while creating NamedRoute object

[opensearch-trigger-bot]

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/opensearch-sdk-java/backport-1.x 1.x
# Navigate to the new working tree
pushd ../.worktrees/opensearch-sdk-java/backport-1.x
# Create a new branch
git switch --create backport/backport-868-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9e31418e5642cd4772db8641d8baed2392480a16
# Push it to GitHub
git push --set-upstream origin backport/backport-868-to-1.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/opensearch-sdk-java/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-868-to-1.x.

[DarshitChanpura]

Cherry-picking this manually into #866