Skip to content
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

Provide a higher level implementation for REST handlers #246

Merged
merged 6 commits into from
Nov 18, 2022

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Nov 12, 2022

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

Description

Creates a RouteHandler class extending Route which also includes a handler method

Creates a BaseExtensionRestHandler abstract class:

  • Matches the request's route to its handler
  • Moves the unhandled request boilerplate into the superclass

Note to reviewers: consider this comment for two possible implementations. Happy to switch back to the other one if you don't like the one I chose. The actual implementation permits either choice; the question is more about what we want the HelloWorld sample to show.

Issues Resolved

Fixes #128
Fixes #245

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 dbwiddis requested a review from a team November 12, 2022 20:21
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2022

Codecov Report

Merging #246 (ee5a298) into main (6df1fa4) will increase coverage by 1.60%.
The diff coverage is 88.37%.

@@             Coverage Diff              @@
##               main     #246      +/-   ##
============================================
+ Coverage     67.19%   68.79%   +1.60%     
- Complexity      102      114      +12     
============================================
  Files            24       27       +3     
  Lines           506      532      +26     
  Branches         17       17              
============================================
+ Hits            340      366      +26     
- Misses          154      155       +1     
+ Partials         12       11       -1     
Impacted Files Coverage Δ
.../java/org/opensearch/sdk/ExtensionRestHandler.java 0.00% <0.00%> (ø)
...a/org/opensearch/sdk/BaseExtensionRestHandler.java 89.47% <89.47%> (ø)
...ch/sdk/sample/helloworld/rest/RestHelloAction.java 85.10% <89.47%> (+7.32%) ⬆️
src/main/java/org/opensearch/sdk/RouteHandler.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.

@dbwiddis
Copy link
Member Author

dbwiddis commented Nov 12, 2022

@mloufra FYI this can simplify your handlers in #213 and #221 if merged before they are.

A better example of the changes you might need is in https://github.com/opensearch-project/anomaly-detection/pull/726/files

However, it's a bit out of scope of what you're doing, so it's totally optional if you think it's too confusing.

@dbwiddis dbwiddis marked this pull request as draft November 12, 2022 22:01
@dbwiddis dbwiddis changed the title Refactor unhandled REST request into handler interface Provide a higher level implementation for REST handlers Nov 13, 2022
@dbwiddis dbwiddis marked this pull request as ready for review November 13, 2022 01:05
@mloufra
Copy link
Contributor

mloufra commented Nov 14, 2022

@mloufra FYI this can simplify your handlers in #213 and #221 if merged before they are.

A better example of the changes you might need is in https://github.com/opensearch-project/anomaly-detection/pull/726/files

However, it's a bit out of scope of what you're doing, so it's totally optional if you think it's too confusing.

Thanks @dbwiddis for these information, I will take a look on that.

@mloufra
Copy link
Contributor

mloufra commented Nov 15, 2022

@mloufra FYI this can simplify your handlers in #213 and #221 if merged before they are.
A better example of the changes you might need is in https://github.com/opensearch-project/anomaly-detection/pull/726/files
However, it's a bit out of scope of what you're doing, so it's totally optional if you think it's too confusing.

I will push the code change after this PR merged

@dbwiddis
Copy link
Member Author

I will push the code change after this PR merged

@mloufra this one may take a while, don't hold up your PR

@mloufra
Copy link
Contributor

mloufra commented Nov 15, 2022

I will push the code change after this PR merged

@mloufra this one may take a while, don't hold up your PR

got it

owaiskazi19
owaiskazi19 previously approved these changes Nov 15, 2022
@dbwiddis dbwiddis requested review from ryanbogan and removed request for dblock November 17, 2022 01:16
@dbwiddis dbwiddis requested a review from joshpalis November 18, 2022 04:59
@ryanbogan ryanbogan merged commit 456c53c into opensearch-project:main Nov 18, 2022
@dbwiddis dbwiddis deleted the unhandledResponse branch February 19, 2023 22:42
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
…project#246)

* Provide a higher level implementation for REST handlers

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

* Use Function rather than Supplier for better thread safety

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

* Even shorter syntax

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

* Add tests for more BaseExtensionRestHandler coverage

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

* Make ExtensionRestHandler a Functional Interface

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

* Don't require subclasses to define route handlers.

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

Signed-off-by: Daniel Widdis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants