Normalize URI paths in RestClient

[Xtansia]

Description

There are a sprinkling of locations such as this line that construct a RestClient request with a non-absolute path (ie. no leading /). This is being passed verbatim to the HTTP request which results in a technically invalid request start-line of GET _render/template HTTP/1.1 which should be GET /_render/template HTTP/1.1. OpenSearch itself is lenient and happily serves this request, however in cases of stricter intermediaries such as proxies or load-balances as in Amazon OpenSearch Service, this request ends up denied with a 400 Bad Request.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

❌ Gradle check result for 0d562b7: FAILURE

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?

[github-actions]

❌ Gradle check result for fa00762: FAILURE

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?

[reta]

It seems like we just need to return new URI("/").resolve(uriBuilder.build().toASCIIString()); to make sure URI is always resolved properly:

 new URI("/").resolve("/_render/template") ==> /_render/template
 new URI("/").resolve("_render/template") ==> /_render/template

[Xtansia]

I ended up tweaking the URIBuilder construction to use its appending rather than building up the full path manually which then takes care of prefixing the /.

[github-actions]

❌ Gradle check result for c2349ff: FAILURE

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?

[reta]

I am not sure the equality checks are suitable here, fe passing // as path would be a miss, the previous code definitely was more robust towards such cases.

[Xtansia]

In this case I was skipping a single / due to the logic of URIBuilder.appendPath:

new URIBuilder().appendPath("/").appendPath("/") -> //
new URIBuilder().appendPath("/foo").appendPath("/bar") -> /foo/bar

I've combined this URIBuilder.appendPath approach with the previous trimSlashes implementation and some extra test cases to see how we feel about this level of normalization. Though this will strip duplicate / in leading or trailing position, it won't touch foo////bar. If we would want to normalize that, then this could be changed to instead split on / and skip empty segments.

[github-actions]

❌ Gradle check result for 0d33093: FAILURE

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?

[github-actions]

❌ Gradle check result for 2b8571b: FAILURE

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?

[github-actions]

❌ Gradle check result for a59b21e: FAILURE

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?

[Xtansia]

There's two tests failing with these changes:

• https://github.com/opensearch-project/OpenSearch/blob/main/client/rest/src/test/java/org/opensearch/client/RestClientSingleHostIntegTests.java#L535 - which expected request to fail when not adding leading slash, now request succeeds due to automatic adding of slash
• https://github.com/opensearch-project/OpenSearch/blob/main/client/rest/src/test/java/org/opensearch/client/RestClientTests.java#L112 - which expects a failure due to malformed URL where as now has slashes trimmed and gets URI encoded as /%3A%3Ahttp%3A and fails elsewhere due to no nodes being configured

Would be great to hear your thoughts on these two cases @reta?

[reta]

There's two tests failing with these changes:

Thanks @Xtansia I think for main (3.0.0) we could change the tests to reflect the new normalization logic, but I would not recommend to backport this change to 2.x branch - it leads to the change in behavior (it is breaking).

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[dblock]

@Xtansia Want to finish this?

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[github-actions]

❌ Gradle check result for 8a6f900: FAILURE

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?