Pass REST params and content to extensions #163

dbwiddis · 2022-09-29T05:58:20Z

Companion PR on OpenSearch: #4633

Gradle checks will fail until companion PR is merged. They pass locally after publishToMavenLocal.
OpenSearch PR will break SDK build due to a class rename, merge these together.

Description

Passes the params() from the original RestRequest object to extensions. Internally tracks the consumed params (similar to the RestRequest class) when they are consumed with the param(key) method (similar to existing plugin usage).

During the process, realized that the RestExecuteOnExtensionRequest class (in OpenSearch) completely duplicated the ExtensionRestHandler class (here), so removed the SDK duplicate and renamed the "Execute" class as ExtensionRestHandler.

Additionally added xContentType and content as 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.

dbwiddis · 2022-09-29T19:13:16Z

Flipping this into draft to handle all the pieces @owaiskazi19 referenced in #58 (comment)

Signed-off-by: Daniel Widdis <[email protected]>

src/main/java/org/opensearch/sdk/ExtensionRestResponse.java

Signed-off-by: Daniel Widdis <[email protected]>

DarshitChanpura

Gradle check won't pass until opensearch-project/OpenSearch#4633 is merged.

codecov-commenter · 2022-09-30T22:07:59Z

Codecov Report

Merging #163 (ca71302) into main (0bdd6b4) will increase coverage by 1.13%.
The diff coverage is 96.00%.

@@             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

Impacted Files	Coverage Δ
...main/java/org/opensearch/sdk/ExtensionsRunner.java	`67.44% <ø> (ø)`
...rch/sdk/handlers/ExtensionsRestRequestHandler.java	`42.85% <66.66%> (+9.52%)`	⬆️
.../org/opensearch/sdk/ExtensionRestPathRegistry.java	`100.00% <100.00%> (ø)`
...java/org/opensearch/sdk/ExtensionRestResponse.java	`100.00% <100.00%> (ø)`
...ch/sdk/sample/helloworld/rest/RestHelloAction.java	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

owaiskazi19 · 2022-09-30T22:26:51Z

src/main/java/org/opensearch/sdk/sample/helloworld/rest/RestHelloAction.java

+        }
+        return new ExtensionRestResponse(request, OK, "Updated the world's name to " + worldName);
+    }
+


Can we have a sample POST request too here?

Can I do that as part of #165?

Sure @dbwiddis! Not blocking this PR :)

owaiskazi19 · 2022-09-30T22:29:25Z

src/main/java/org/opensearch/sdk/ExtensionRestHandler.java

@@ -9,6 +9,7 @@

 import java.util.List;

+import org.opensearch.extensions.rest.ExtensionRestRequest;


Thinking out loud. Why are we keeping ExtensionRestRequest in OpenSearch and not in SDK?

Because the previous RestSendToExtensionRequest entirely duplicated it. Literally all the lines were the same except for a few naming differences.

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.

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

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.

src/main/java/org/opensearch/sdk/ExtensionRestPathRegistry.java

* Rename/merge duplicate ExtensionRestRequest implementations Signed-off-by: Daniel Widdis <[email protected]> * Track consumed params inside request object Signed-off-by: Daniel Widdis <[email protected]> * Put back test for the "this will never happen" case Signed-off-by: Daniel Widdis <[email protected]> * Add content and expand param getters Signed-off-by: Daniel Widdis <[email protected]> * Rebase to main and update after conficts Signed-off-by: Daniel Widdis <[email protected]> * More merge conflict cleanup Signed-off-by: Daniel Widdis <[email protected]> * Whitespace diff reduction Signed-off-by: Daniel Widdis <[email protected]> * Change uri to path in RestRequest handling Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis requested a review from a team September 29, 2022 05:58

dbwiddis mentioned this pull request Sep 29, 2022

[Feature/extensions] Pass REST params and content to extensions opensearch-project/OpenSearch#4633

Merged

6 tasks

dbwiddis changed the title ~~Params~~ Pass REST params to extensions Sep 29, 2022

dbwiddis requested review from owaiskazi19, saratvemulapalli, joshpalis, ryanbogan and peternied September 29, 2022 06:00

dbwiddis marked this pull request as draft September 29, 2022 19:13

This was referenced Sep 29, 2022

Refactor handler method #158

Merged

[Feature/extensions] Implement createDetector request in AD Extension #58

Closed

dbwiddis changed the title ~~Pass REST params to extensions~~ Pass REST params and content to extensions Sep 30, 2022

dbwiddis added 5 commits September 29, 2022 20:06

Rename/merge duplicate ExtensionRestRequest implementations

70a8c32

Signed-off-by: Daniel Widdis <[email protected]>

Track consumed params inside request object

16854bd

Signed-off-by: Daniel Widdis <[email protected]>

Put back test for the "this will never happen" case

61efa08

Signed-off-by: Daniel Widdis <[email protected]>

Add content and expand param getters

f29c893

Signed-off-by: Daniel Widdis <[email protected]>

Rebase to main and update after conficts

89eee75

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the params branch from 159e6f2 to 89eee75 Compare September 30, 2022 03:58

dbwiddis marked this pull request as ready for review September 30, 2022 04:00

dbwiddis added 2 commits September 29, 2022 21:15

More merge conflict cleanup

ffa6b38

Signed-off-by: Daniel Widdis <[email protected]>

Whitespace diff reduction

a9033cb

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the params branch from e57a481 to a9033cb Compare September 30, 2022 04:27

peternied reviewed Sep 30, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionRestResponse.java Show resolved Hide resolved

Change uri to path in RestRequest handling

ca71302

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis mentioned this pull request Sep 30, 2022

[FEATURE] Move ExtensionRestResponse class to same package as ExtensionRestRequest #165

Closed

DarshitChanpura approved these changes Sep 30, 2022

View reviewed changes

peternied approved these changes Sep 30, 2022

View reviewed changes

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionRestPathRegistry.java Show resolved Hide resolved

dbwiddis requested a review from owaiskazi19 September 30, 2022 22:55

owaiskazi19 approved these changes Sep 30, 2022

View reviewed changes

owaiskazi19 merged commit 7218f6e into opensearch-project:main Sep 30, 2022

dbwiddis deleted the params branch September 30, 2022 23:00

dbwiddis mentioned this pull request Oct 10, 2022

[Feature/extensions] Add getSettings support for AD opensearch-project/anomaly-detection#686

Merged

dbwiddis mentioned this pull request Oct 31, 2022

[FEATURE] Give Extensions access to core NamedXContent #208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass REST params and content to extensions #163

Pass REST params and content to extensions #163

dbwiddis commented Sep 29, 2022 •

edited

Loading

dbwiddis commented Sep 29, 2022

DarshitChanpura left a comment •

edited

Loading

codecov-commenter commented Sep 30, 2022

owaiskazi19 Sep 30, 2022

dbwiddis Sep 30, 2022

owaiskazi19 Sep 30, 2022

owaiskazi19 Sep 30, 2022

dbwiddis Sep 30, 2022

dbwiddis Sep 30, 2022

owaiskazi19 Sep 30, 2022 •

edited

Loading

dbwiddis Sep 30, 2022

		@@ -9,6 +9,7 @@

		import java.util.List;

		import org.opensearch.extensions.rest.ExtensionRestRequest;

Pass REST params and content to extensions #163

Pass REST params and content to extensions #163

Conversation

dbwiddis commented Sep 29, 2022 • edited Loading

Description

Issues Resolved

dbwiddis commented Sep 29, 2022

DarshitChanpura left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 30, 2022

Codecov Report

owaiskazi19 Sep 30, 2022

Choose a reason for hiding this comment

dbwiddis Sep 30, 2022

Choose a reason for hiding this comment

owaiskazi19 Sep 30, 2022

Choose a reason for hiding this comment

owaiskazi19 Sep 30, 2022

Choose a reason for hiding this comment

dbwiddis Sep 30, 2022

Choose a reason for hiding this comment

dbwiddis Sep 30, 2022

Choose a reason for hiding this comment

owaiskazi19 Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

dbwiddis Sep 30, 2022

Choose a reason for hiding this comment

dbwiddis commented Sep 29, 2022 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading

owaiskazi19 Sep 30, 2022 •

edited

Loading