Added support for headers for ExtensionRestRequest

[nassipkali]

Description

Adds SDKRestRequest, SDKHttpRequest

Companion PR: opensearch-project/OpenSearch#6826

Issues Resolved

#431

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.

[nassipkali]

@dbwiddis first option mentioned in previous pr is more complicated because protected constructor of RestRequest requires also to fill HttpRequest. We can create sdkHttpRequest instead sdkRestRequest. sdkHttpRequest can replace anonymous HttpRequest class.

[dbwiddis]

@dbwiddis first option mentioned in previous pr is more complicated because protected constructor of RestRequest requires also to fill HttpRequest. We can create sdkHttpRequest instead sdkRestRequest. sdkHttpRequest can replace anonymous HttpRequest class.

We still need both. In particular we really need the params() in the RestRequest() and the only way to get them in there from the static HttpRequest is by hacking at the uri (which I did in the test class). Take a look at TestBaseExtensionRestHandler.createTestRestRequest() in particular the uri() method. That works for combining path + params into a uri, but that's not the original uri.

But we should be able to use an anonymous RestRequest subclass instead of the static method to create the RestRequest. Change RestRequest.request(xContentRegistry, new HttpRequest() { ... }, null); to new RestRequest(xContentRegistry, params, path, headers, new HttpRequest() { ... existing overrides ... } , null) {};.

[nassipkali]

@dbwiddis first option mentioned in previous pr is more complicated because protected constructor of RestRequest requires also to fill HttpRequest. We can create sdkHttpRequest instead sdkRestRequest. sdkHttpRequest can replace anonymous HttpRequest class.

We still need both. In particular we really need the params() in the RestRequest() and the only way to get them in there from the static HttpRequest is by hacking at the uri (which I did in the test class). Take a look at TestBaseExtensionRestHandler.createTestRestRequest() in particular the uri() method. That works for combining path + params into a uri, but that's not the original uri.

But we should be able to use an anonymous RestRequest subclass instead of the static method to create the RestRequest. Change RestRequest.request(xContentRegistry, new HttpRequest() { ... }, null); to new RestRequest(xContentRegistry, params, path, headers, new HttpRequest() { ... existing overrides ... } , null) {};.

https://github.com/opensearch-project/OpenSearch/blob/bcbb561b51c60776257e68786fcc411eebf4fab4/server/src/main/java/org/opensearch/rest/RestRequest.java#LL170C42-L170C42
Constuctor gets params from uri. Is RestRequest subclass needed in future? If yes then I try implement that class

[nassipkali]

@dbwiddis I can add path to ExtensionRestRequest. If I keep path, uri, params in ExtensionRestRequest then I can create instance of RestRequest subclass without overhead(converting uri to path, getting params from uri)

[dbwiddis]

@dbwiddis I can add path to ExtensionRestRequest. If I keep path, uri, params in ExtensionRestRequest then I can create instance of RestRequest subclass without overhead(converting uri to path, getting params from uri)

The "params from uri" happens in OpenSearch inside the RestController. It has already:

• Decoded params from the URI ( such as the name/blah pair in /restcommand/foo?name=blah)
• Decoded named params from registered REST actions (such as the id/123 pair in a registered rest path /detector/{id} when a user types /detector/123.

It's that second part that we do not have the ability to decode in the Extension, which is why we must pass the params() map in the ExtensionRestRequest and use it to recreate the RestRequest with those same params. What I did in the test class was a hack around the way we should do it.

Since we have the params, we don't really need the uri, since the path is the interesting part. But since we are recreating the whole HttpRequest it doesn't hurt to include both (by putting the uri where it belongs, the user can get either the path or the uri). It looks like you've handled these in your PR, so I'll go review it now!

[dbwiddis]

This looks great!

I've made a few suggestions for improvements, mostly in the documentation.

One more request: our org.opensearch.sdk package is getting cluttered and we should move stuff out of it when we can. Please create a new org.opensearch.sdk.rest package and put your two new SDK classes into it.

Might also be a good home for these classes (we can address in a later PR):

• ExtensionRestHandler
• BaseExtensionRestHandler (will break dependent extensions like AD but we can synchronize fix)
• RouteHandler (will break dependent extensions like AD but we can synchronize fix)
• ExtensionRestPathRegistry

[dbwiddis]

In addition to the comment changes and package change requested above, also please update the tests:

• the TestBaseExtensionRestHandler class contains a method createTestRestRequest(). Replace the code there to use your new SDK classes, so that test code relying on them will use your new classes.

[dbwiddis]

LGTM!

Tests should pass following merge of opensearch-project/OpenSearch#6826

[dbwiddis]

Looks good, some suggestions to improve it!

[nassipkali]

@dbwiddis Map<String, Object> source = request.contentParser().map();. Can I use there mapStrings()? mapStrings returns Map<String, String> and doesnt need cast/convert Object to String

[dbwiddis]

@dbwiddis Map<String, Object> source = request.contentParser().map();. Can I use there mapStrings()? mapStrings returns Map<String, String> and doesnt need cast/convert Object to String

Yep!

[nassipkali]

@dbwiddis When snapshot that contains latest commits of OpenSearch is will be used? My PR from OpenSearch side is merged but this PR checks didn't see changes from OpenSearch side.

[dbwiddis]

OK, new snapshot is being read but there's a missing method signature we're trying to read:

/home/runner/work/opensearch-sdk-java/opensearch-sdk-java/src/main/java/org/opensearch/sdk/rest/SDKHttpRequest.java:42: error: cannot find symbol

    this.httpVersion = request.httpVersion();

[nassipkali]

OK, new snapshot is being read but there's a missing method signature we're trying to read:

/home/runner/work/opensearch-sdk-java/opensearch-sdk-java/src/main/java/org/opensearch/sdk/rest/SDKHttpRequest.java:42: error: cannot find symbol

    this.httpVersion = request.httpVersion();

Yes, there should be protocolVersion()

[dbwiddis]

Yes, there should be protocolVersion()

Just found that myself :)

Should be a quick fix and we can merge this!

[dbwiddis]

@nassipkali are you going to be fixing that method?

[nassipkali]

@dbwiddis I have a problem with git push. I made change from github web editor

[dbwiddis]

@dbwiddis I have a problem with git push. I made change from github web editor

Lots of different errors in the build now.

Locally you should be able to pull the changes down from your branch.

[dbwiddis]

@nassipkali I'm going to go ahead and fix up the issues here, since the SDK build is now failing other PRs.

One challenge with these companion PRs is finding out what's broken before we merge the companion. You should be using ./gradlew publishToMavenLocal on the OpenSearch PR branch and then running ./gradlew check on this PR branch to verify that things work.