Skip to content

Provide a higher level implementation for REST handlers #246

Merged
ryanbogan merged 6 commits intoopensearch-project:mainfrom
dbwiddis:unhandledResponse
Nov 18, 2022
Merged

Provide a higher level implementation for REST handlers #246
ryanbogan merged 6 commits intoopensearch-project:mainfrom
dbwiddis:unhandledResponse

Conversation

@dbwiddis
Copy link
Copy Markdown
Member

@dbwiddis dbwiddis commented Nov 12, 2022 •
edited
Loading

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

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
Copy Markdown

codecov-commenter commented Nov 12, 2022 •
edited by codecov bot
Loading

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.79%. Comparing base (6df1fa4) to head (ee5a298).
⚠️ Report is 281 commits behind head on main.

Files with missing lines Patch % Lines
...a/org/opensearch/sdk/BaseExtensionRestHandler.java 89.47% 1 Missing and 1 partial ⚠️
...ch/sdk/sample/helloworld/rest/RestHelloAction.java 89.47% 2 Missing ⚠️
.../java/org/opensearch/sdk/ExtensionRestHandler.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbwiddis
Copy link
Copy Markdown
Member Author

dbwiddis commented Nov 12, 2022 •
edited
Loading

@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
Provide a higher level implementation for REST handlers
833e52a 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@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
@dbwiddis
Use Function rather than Supplier for better thread safety
56d6347 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Even shorter syntax
b7ee19d 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Add tests for more BaseExtensionRestHandler coverage
fca592c 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@mloufra
Copy link
Copy Markdown
Contributor

mloufra commented Nov 14, 2022 •
edited
Loading

@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
Copy Markdown
Contributor

mloufra commented Nov 15, 2022 •
edited
Loading

@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
Copy Markdown
Member Author

dbwiddis 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

@mloufra
Copy link
Copy Markdown
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
View reviewed changes
@dbwiddis
Make ExtensionRestHandler a Functional Interface
da7cbd6 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis requested a review from owaiskazi19 November 16, 2022 04:55
@dbwiddis
Don't require subclasses to define route handlers.
ee5a298 
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@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
@dbwiddis @kokibas
Provide a higher level implementation for REST handlers (opensearch-…
4685ada 
…project#246)

* Provide a higher level implementation for REST handlers

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

* Use Function rather than Supplier for better thread safety

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

* Even shorter syntax

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

* Add tests for more BaseExtensionRestHandler coverage

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

* Make ExtensionRestHandler a Functional Interface

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

* Don't require subclasses to define route handlers.

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

Signed-off-by: Daniel Widdis <widdis@gmail.com>
caokyhieu pushed a commit to caokyhieu/opensearch-sdk-java that referenced this pull request Aug 15, 2025
@dbwiddis
Provide a higher level implementation for REST handlers (opensearch-…
8678ed8 
…project#246)

* Provide a higher level implementation for REST handlers

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

* Use Function rather than Supplier for better thread safety

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

* Even shorter syntax

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

* Add tests for more BaseExtensionRestHandler coverage

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

* Make ExtensionRestHandler a Functional Interface

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

* Don't require subclasses to define route handlers.

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

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanbogan ryanbogan ryanbogan approved these changes

@owaiskazi19 owaiskazi19 owaiskazi19 approved these changes

@joshpalis joshpalis Awaiting requested review from joshpalis joshpalis is a code owner

+1 more reviewer

@dblock dblock dblock left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Create boilerplate "not configured to handle" ExtensionRestResponse [FEATURE] Provide a higher level implementation for REST handlers

6 participants

@dbwiddis @codecov-commenter @mloufra @dblock @ryanbogan @owaiskazi19