Pass REST params and content to extensions#163
Pass REST params and content to extensions#163owaiskazi19 merged 8 commits intoopensearch-project:mainfrom
Conversation
|
Flipping this into draft to handle all the pieces @owaiskazi19 referenced in #58 (comment) |
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
There was a problem hiding this comment.
Gradle check won't pass until opensearch-project/OpenSearch#4633 is merged.
Codecov Report
@@ Coverage Diff @@
## main #163 +/- ##
============================================
+ Coverage 64.60% 65.73% +1.13%
+ Complexity 99 94 -5
============================================
Files 25 24 -1
Lines 500 464 -36
Branches 18 13 -5
============================================
- Hits 323 305 -18
+ Misses 169 151 -18
Partials 8 8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| } | ||
| return new ExtensionRestResponse(request, OK, "Updated the world's name to " + worldName); | ||
| } | ||
|
|
|
|
||
| import java.util.List; | ||
|
|
||
| import org.opensearch.extensions.rest.ExtensionRestRequest; |
There was a problem hiding this comment.
Thinking out loud. Why are we keeping ExtensionRestRequest in OpenSearch and not in SDK?
There was a problem hiding this comment.
Because the previous RestSendToExtensionRequest entirely duplicated it. Literally all the lines were the same except for a few naming differences.
There was a problem hiding this comment.
Also it makes sense (see #165) to move the ExtensionRestResponse there as well since it's really the same thing as the BytesRestResponse with some tweaks.
There was a problem hiding this comment.
This answers my questions. Just one thing to add can we keep the name as RestSendToExtensionRequest instead of ExtensionRestRequest for better understanding. Can be address in the next PR
There was a problem hiding this comment.
Let’s discuss on #165, because both classes are part of the API we expect extension authors to use. The ExtensionRestRequest is the argument type for the handleRequest method and it closely parallels the same method on plugins and I’d really like similar names.
* Rename/merge duplicate ExtensionRestRequest implementations Signed-off-by: Daniel Widdis <widdis@gmail.com> * Track consumed params inside request object Signed-off-by: Daniel Widdis <widdis@gmail.com> * Put back test for the "this will never happen" case Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add content and expand param getters Signed-off-by: Daniel Widdis <widdis@gmail.com> * Rebase to main and update after conficts Signed-off-by: Daniel Widdis <widdis@gmail.com> * More merge conflict cleanup Signed-off-by: Daniel Widdis <widdis@gmail.com> * Whitespace diff reduction Signed-off-by: Daniel Widdis <widdis@gmail.com> * Change uri to path in RestRequest handling Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Daniel Widdis <widdis@gmail.com>
* Rename/merge duplicate ExtensionRestRequest implementations Signed-off-by: Daniel Widdis <widdis@gmail.com> * Track consumed params inside request object Signed-off-by: Daniel Widdis <widdis@gmail.com> * Put back test for the "this will never happen" case Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add content and expand param getters Signed-off-by: Daniel Widdis <widdis@gmail.com> * Rebase to main and update after conficts Signed-off-by: Daniel Widdis <widdis@gmail.com> * More merge conflict cleanup Signed-off-by: Daniel Widdis <widdis@gmail.com> * Whitespace diff reduction Signed-off-by: Daniel Widdis <widdis@gmail.com> * Change uri to path in RestRequest handling Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Daniel Widdis <widdis@gmail.com>
Companion PR on OpenSearch: #4633
publishToMavenLocal.Description
Passes the
params()from the originalRestRequestobject to extensions. Internally tracks the consumed params (similar to theRestRequestclass) when they are consumed with theparam(key)method (similar to existing plugin usage).During the process, realized that the
RestExecuteOnExtensionRequestclass (in OpenSearch) completely duplicated theExtensionRestHandlerclass (here), so removed the SDK duplicate and renamed the "Execute" class asExtensionRestHandler.Additionally added
xContentTypeandcontentas well as getters for the params that will enable AD extension functionality.Remaining TODO to be handled as part of #132 is to instantiate the parser on the SDK side. All that's needed from the REST request is the xContentType.
Issues Resolved
Fixes #111
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.