-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature/extensions] Pass REST params and content to extensions #4633
[Feature/extensions] Pass REST params and content to extensions #4633
Conversation
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
Got it... the PR includes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
* | ||
* @opensearch.api | ||
*/ | ||
public class ExtensionRestRequest extends TransportRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New shorter name, huzzah!
Signed-off-by: Daniel Widdis <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Ready for review. Gradle check failure is unrelated to this PR. |
Signed-off-by: Daniel Widdis <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
public int hashCode() { | ||
return Objects.hash(method, path, params, xContentType, content, principalIdentifierToken); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done!
@@ -105,12 +108,16 @@ public List<Route> routes() { | |||
|
|||
@Override | |||
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq, out of scope of this PR, should we change the method name to handleRequest
in TransportRequest.java at some point in future?
Companion PR on SDK: #163
Description
Passes the
params()
from the originalRestRequest
object to extensions. Internally tracks the consumed params (similar to theRestRequest
class) when they are consumed with theparam(key)
method (similar to existing plugin usage).During the process, realized that the
RestExecuteOnExtensionRequest
class (here) completely duplicated theExtensionRestHandler
class (in the SDK), so removed the SDK duplicate and renamed the "Execute" class asExtensionRestHandler
.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
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.