[Feature/extensions] Pass REST params and content to extensions by dbwiddis · Pull Request #4633 · opensearch-project/OpenSearch

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

Companion PR on SDK: #163

This PR will break SDK build due to a class rename, merge these together.
This PR will also break Extension API due to change in package location for ExtensionRestRequest. AD Extension will need to be updated

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 (here) completely duplicated the ExtensionRestHandler class (in the SDK), 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, and handling the consumed content on the response.

Issues Resolved

Fixes #111

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: Daniel Widdis <widdis@gmail.com>

github-actions · 2022-09-29T06:28:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3558/
CommitID: 2073a7b

github-actions · 2022-09-29T06:30:14Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3559/
CommitID: f853429

codecov-commenter · 2022-09-29T06:30:47Z

Codecov Report

Merging #4633 (f853429) into feature/extensions (eae2c0b) will increase coverage by 0.00%.
The diff coverage is 52.11%.

❗ Current head f853429 differs from pull request most recent head 63f611b. Consider uploading reports for the commit 63f611b to get more accurate results

@@                  Coverage Diff                  @@
##             feature/extensions    #4633   +/-   ##
=====================================================
  Coverage                 70.62%   70.63%           
- Complexity                57557    57559    +2     
=====================================================
  Files                      4654     4656    +2     
  Lines                    276657   276679   +22     
  Branches                  40427    40429    +2     
=====================================================
+ Hits                     195399   195434   +35     
+ Misses                    64967    64918   -49     
- Partials                  16291    16327   +36

Impacted Files	Coverage Δ
...opensearch/extensions/ExtensionActionListener.java	`72.72% <ø> (ø)`
...va/org/opensearch/extensions/ExtensionRequest.java	`29.41% <ø> (-14.59%)`	⬇️
...rch/extensions/rest/RestSendToExtensionAction.java	`27.27% <0.00%> (-0.36%)`	⬇️
...sions/ExtensionActionListenerOnFailureRequest.java	`29.41% <29.41%> (ø)`
...ensearch/extensions/rest/ExtensionRestRequest.java	`60.52% <60.52%> (ø)`
...rch/extensions/ExtensionActionListenerHandler.java	`66.66% <66.66%> (ø)`
.../opensearch/extensions/ExtensionsOrchestrator.java	`66.66% <75.00%> (-4.22%)`	⬇️
.../java/org/opensearch/node/NodeClosedException.java	`50.00% <0.00%> (-50.00%)`	⬇️
...adcast/BroadcastShardOperationFailedException.java	`55.55% <0.00%> (-44.45%)`	⬇️
.../java/org/opensearch/search/dfs/AggregatedDfs.java	`54.83% <0.00%> (-41.94%)`	⬇️
... and 479 more

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

github-actions · 2022-09-29T06:48:17Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3561/
CommitID: 63f611b

owaiskazi19 · 2022-09-29T18:57:23Z

@dbwiddis we will be needing these params mentioned here too.

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

@dbwiddis we will be needing these params mentioned here too.

Got it... the PR includes param and enables hasParam/paramAsLong (though I can add them specifically). Flipping this to draft to look into also passing content and necessary contentParser pieces. I had already considered including that anyway.

peternied

Looks great!

peternied · 2022-09-29T21:37:19Z

server/src/main/java/org/opensearch/extensions/rest/ExtensionRestRequest.java

+ *
+ * @opensearch.api
+ */
+public class ExtensionRestRequest extends TransportRequest {


New shorter name, huzzah!

Signed-off-by: Daniel Widdis <widdis@gmail.com>

github-actions · 2022-09-30T03:16:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3608/
CommitID: fe3d26c

dbwiddis · 2022-09-30T04:03:47Z

Ready for review. Gradle check failure is unrelated to this PR.

Signed-off-by: Daniel Widdis <widdis@gmail.com>

github-actions · 2022-09-30T17:03:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3637/
CommitID: 035c44b

DarshitChanpura · 2022-09-30T19:34:05Z

server/src/main/java/org/opensearch/extensions/rest/ExtensionRestRequest.java

+    public int hashCode() {
+        return Objects.hash(method, path, params, xContentType, content, principalIdentifierToken);
+    }
+}


nicely done!

DarshitChanpura · 2022-09-30T19:37:53Z

server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java

@@ -105,12 +108,16 @@ public List<Route> routes() {

    @Override
    public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {


qq, out of scope of this PR, should we change the method name to handleRequest in TransportRequest.java at some point in future?

dbwiddis added 3 commits September 28, 2022 19:34

Send REST params to extension

2ccfe00

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Rename/merge duplicate ExtensionRestRequest implementations

25b6dea

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Track consumed params inside request object

2073a7b

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis requested review from a team and reta as code owners September 29, 2022 05:58

dbwiddis requested review from owaiskazi19, peternied and saratvemulapalli September 29, 2022 06:01

dbwiddis mentioned this pull request Sep 29, 2022

Pass REST params and content to extensions opensearch-project/opensearch-sdk-java#163

Merged

Change log

63f611b

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis force-pushed the params branch from f853429 to 63f611b Compare September 29, 2022 06:20

dbwiddis requested review from joshpalis and ryanbogan September 29, 2022 06:20

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

peternied approved these changes Sep 29, 2022

View reviewed changes

Add content and expand param getters

fe3d26c

Signed-off-by: Daniel Widdis <widdis@gmail.com>

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

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

Change uri to path in RestRequest handling

035c44b

Signed-off-by: Daniel Widdis <widdis@gmail.com>

DarshitChanpura approved these changes Sep 30, 2022

View reviewed changes

peternied merged commit 54b4ad0 into opensearch-project:feature/extensions Sep 30, 2022

dbwiddis deleted the params branch October 6, 2022 17:11

ryanbogan mentioned this pull request Nov 17, 2022

[Feature/extensions] Merge to main tracking - REST, TranportActions, and EnvironmentSettings updates (up to present) #5273

Closed

		@@ -105,12 +108,16 @@ public List<Route> routes() {

		@Override
		public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {

Conversation

dbwiddis commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Sep 29, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

owaiskazi19 commented Sep 29, 2022

Uh oh!

dbwiddis commented Sep 29, 2022

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

peternied Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 30, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dbwiddis commented Sep 30, 2022

Uh oh!

github-actions bot commented Sep 30, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

DarshitChanpura Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbwiddis commented Sep 29, 2022 •

edited

Loading

codecov-commenter commented Sep 29, 2022 •

edited

Loading