Handle named wildcards (REST path parameters) #123

dbwiddis · 2022-09-06T04:09:05Z

Companion PR: opensearch-project/OpenSearch#4415

Description

Adds the ability to handle REST path parameters (named wildcards). It is necessary to indicate which parameters were consumed. If the user provides too many parameters, they receive an error.

In local/plugin operation, simply fetching the parameter via param(key) consumes it. Since the parameters are being consumed remotely, this requires communicating the list of consumed parameters, which is done in the response header (normally unused and provided for purposes such as this!)

Primary changes:

(First commit) Replaces the pathMap in ExtensionsRunner with an ExtensionRestPathRegistry.
- The internal storage of the REST paths uses a PathTrie so that the matching behavior matches that of the OpenSearch RestController
  - Exact match conflicts don't work
  - Conflicting wildcards don't work
  - An exact match takes precedence over a wildcard match when they overlap
(Fourth commit) Changes the return type from extension rest handlers to an ExtensionRestResponse. This is a simple wrapper for a BytesRestResponse with added handling for a list of consumed parameters
- The RestResponse class already provides a facility for custom headers, presently only used with the OpenSearchException class. Adding a new key for consumed parameters doesn't conflict with this usage.
- See the companion PR for how this is parsed out on the other side
- There will be a need to more elegantly handle all the types of parameters or better document usage for developers; this detail is deferred until [FEATURE] Pass REST params to extensions #111 is complete.
(Third Commit) Updated the HelloWorldExtension to add another Route:
- Using PUT and a {name} parameter allows changing the name of the world for future greetings.
(Throughout) Added tests for the ExtensionRestPathRegistry, ExtensionRestResponse, and sample extension behavior, as well as updating other tests as required.

Other changes (mostly in second commit):

Changed the default ./gradlew run to the test settings on ExtensionsRunner and added a new task for the Hello World extension: it can be executed with ./gradlew helloWorld.
Moved the HelloWorld extension into a subpackage of the sample package in anticipation of more samples (such as the CRUD sample the security team is working on).
Added OpenAPI-compatible spec files (both YAML and JSON). We probably don't need both, but adding for demo/discussion until we settle on something. View by importing to https://editor.swagger.io/
Updated DESIGN and DEVELOPER_GUIDE with the latest changes

Issues Resolved

Fixes #122

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 <[email protected]>

codecov-commenter · 2022-09-06T04:22:13Z

Codecov Report

Merging #123 (67e5b3a) into main (4dc1895) will increase coverage by 3.81%.
The diff coverage is 97.95%.

@@             Coverage Diff              @@
##               main     #123      +/-   ##
============================================
+ Coverage     65.81%   69.62%   +3.81%     
- Complexity       60       73      +13     
============================================
  Files            14       16       +2     
  Lines           313      349      +36     
  Branches          9       11       +2     
============================================
+ Hits            206      243      +37     
+ Misses          100       99       -1     
  Partials          7        7

Impacted Files	Coverage Δ
...rch/sdk/sample/helloworld/HelloWorldExtension.java	`57.14% <ø> (ø)`
...main/java/org/opensearch/sdk/ExtensionsRunner.java	`64.90% <83.33%> (+0.42%)`	⬆️
.../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.

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

dbwiddis · 2022-09-06T04:29:58Z

Rocking the code coverage 💪

Also poor-man's integration test:

[~/git/OpenSearch] % curl -X GET localhost:9200/_extensions/_opensearch-sdk-1/hello
Hello, World!%

[~/git/OpenSearch] % curl -X PUT localhost:9200/_extensions/_opensearch-sdk-1/hello/OpenSearch+Build+Hamster
Updated the world's name to OpenSearch Build Hamster%

[~/git/OpenSearch] % curl -X GET localhost:9200/_extensions/_opensearch-sdk-1/hello
Hello, OpenSearch Build Hamster!%

owaiskazi19 · 2022-09-06T22:22:04Z

@dbwiddis do you think we should have break this PR into smaller pieces?

dbwiddis · 2022-09-06T23:17:37Z

@dbwiddis do you think we should have break this PR into smaller pieces?

Possibly, but some of the pieces might break as they depend on each other. The first commit is the main part of the implementation and is only 4 files (2 changes + 2 tests) and is most of what needs to be reviewed. The rest is implementing/demoing/documenting/bugfixing that first commit.

For ease of reviewing, you can click on each commit separately:

ab2e0dc is the registry code using the PathTrie rather than a hashmap and is the main topic of the PR, with only 4 files to review
fea6ebc is mostly just moving files into a subpackage, with the addition of some documentation updates and enabling gradle to run the extension as other than the default "run". It shouldnt be too hard to review all of these changes quickly.
a2c127f updates the sample extension (in its new location so the diff is easier to see) to take advantage of the new capability, and helps demo what the first commit enables.
0b6a19b and the companion PR add the list of consumed parameters to the response, a bug I didn't find until I thought I was done and tried manual integration testing.

dbwiddis · 2022-09-06T23:29:02Z

@owaiskazi19 I added some notes to the outline in the first comment noting which commit they correspond to. I think that should help reviewers logically piece together all the changes.)

owaiskazi19 · 2022-09-07T00:03:51Z

Probably comments on specific commits are not visible here. Something new I learned :)
Okay. So that's because those commits redirected to the fork repo. Got it!

dbwiddis · 2022-09-07T00:10:12Z

Probably comments on specific commits are not visible here.

Yeah, bummer. Click on the "Commits" tab up top :)

I did see and reply to your comments, though...

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

saratvemulapalli

Thanks @dbwiddis for the changes.

DESIGN.md

saratvemulapalli · 2022-09-07T16:08:17Z

src/main/java/org/opensearch/sdk/sample/helloworld/spec/openapi.json

@@ -0,0 +1,89 @@
+{


For my learning: Why do we need specs with .json and .yml?
Is it mostly of different clients to generate APIs?

We don't need both. It's probably an either-or. I find the YAML one easier to read/edit, but the JSON one seems common for the OpenSearch spec. Also there is discussion about using other tools to generate either of these from a simpler format. This is mostly just a prompt/reminder for the future. See #127

(Another reason I prefer YAML: comments are supported.)

(But OpenSearch uses JSON.)

saratvemulapalli · 2022-09-07T16:16:39Z

src/test/java/org/opensearch/sdk/TestExtensionRestPathRegistry.java

+ assertNull(extensionRestPathRegistry.getHandler(Method.PUT, "/bar/mars/bar"));
+
+ assertEquals(bazHandler, extensionRestPathRegistry.getHandler(Method.POST, "/baz/europa/qux"));
+ assertNull(extensionRestPathRegistry.getHandler(Method.POST, "/bar/europa"));


I was like why doesn't this work and I realized the Method is POST.
For few paths, folks usually want to support multiple Methods like GET, DELETE etc.
Is this something you plan on to add ?

The capability is there, you just add it in routes() and handle it appropriately in handleRequest(). I'm trying not to overcomplicate hello world, though! :)

Co-authored-by: Sarat Vemulapalli <[email protected]> Signed-off-by: Daniel Widdis <[email protected]>

peternied

One nit, great forward progress.

peternied · 2022-09-08T16:20:54Z

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

+ // PathTrie to match paths to handlers
+ private PathTrie<ExtensionRestHandler> pathTrie = new PathTrie<>(RestUtils.REST_DECODER);
+ // List to return registered handlers
+ private List<String> registeredPaths = new ArrayList<>();


This gives off a code smell where this class is additional functionality to the data structure, can we add getKeys() to the PathTrie implementation?

@peternied I tried that route. Unfortunately the PathTrie does not have functionality to list keys. You can insert, but the retrieve functionality requires you to give it a path (and possibly parameters to match). There is no way to generate a list of paths (short of a brute force dictionary attack of all possible strings to see which ones work).

This lack of functionality on the PathTrie is also a challenge in testing (no way to validate/confirm that a path has been inserted, other than an IT) and might be a reasonable feature request to OpenSearch. But for now, for the purposes of deserializing the extension REST API to pass it over, this slightly smelly code is really the only reasonable option.

* Register REST paths including named wildcards Signed-off-by: Daniel Widdis <[email protected]> * Move HelloWorld extension to subpackage Signed-off-by: Daniel Widdis <[email protected]> * Update Hello World example with named wildcard example Signed-off-by: Daniel Widdis <[email protected]> * Pass consumed params in RestResponse header Signed-off-by: Daniel Widdis <[email protected]> * Linelint hates me and web tool exports Signed-off-by: Daniel Widdis <[email protected]> * Code Review tweaks Signed-off-by: Daniel Widdis <[email protected]> * Update DESIGN.md Co-authored-by: Sarat Vemulapalli <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Co-authored-by: Sarat Vemulapalli <[email protected]>

dbwiddis added 4 commits September 5, 2022 15:55

Register REST paths including named wildcards

ab2e0dc

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

Move HelloWorld extension to subpackage

fea6ebc

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

Update Hello World example with named wildcard example

a2c127f

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

Pass consumed params in RestResponse header

0b6a19b

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

dbwiddis requested review from peternied, owaiskazi19, saratvemulapalli and a team September 6, 2022 04:09

Linelint hates me and web tool exports

67e5b3a

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

dbwiddis force-pushed the namedWildcard branch from ac0a4c2 to 67e5b3a Compare September 6, 2022 04:27

dbwiddis mentioned this pull request Sep 6, 2022

[Feature/extensions] Handle named wildcards (REST path parameters) opensearch-project/OpenSearch#4415

Merged

5 tasks

dbwiddis mentioned this pull request Sep 6, 2022

[FEATURE] Support Routes with named wildcards #122

Closed

This was referenced Sep 6, 2022

[FEATURE] Code coverage reports for SDK #48

Closed

Code coverage #104

Merged

Code Review tweaks

73de199

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

saratvemulapalli previously approved these changes Sep 7, 2022

View reviewed changes

Update DESIGN.md

0f0485e

Co-authored-by: Sarat Vemulapalli <[email protected]> Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis dismissed saratvemulapalli’s stale review via 0f0485e September 7, 2022 16:40

dbwiddis requested a review from saratvemulapalli September 7, 2022 16:47

saratvemulapalli approved these changes Sep 7, 2022

View reviewed changes

peternied approved these changes Sep 8, 2022

View reviewed changes

dbwiddis merged commit 27612c8 into opensearch-project:main Sep 8, 2022

dbwiddis deleted the namedWildcard branch September 8, 2022 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle named wildcards (REST path parameters) #123

Handle named wildcards (REST path parameters) #123

dbwiddis commented Sep 6, 2022 •

edited

Loading

codecov-commenter commented Sep 6, 2022 •

edited

Loading

dbwiddis commented Sep 6, 2022

owaiskazi19 commented Sep 6, 2022

dbwiddis commented Sep 6, 2022 •

edited

Loading

dbwiddis commented Sep 6, 2022

owaiskazi19 commented Sep 7, 2022 •

edited

Loading

dbwiddis commented Sep 7, 2022

saratvemulapalli left a comment

saratvemulapalli Sep 7, 2022

dbwiddis Sep 7, 2022

dbwiddis Sep 7, 2022

saratvemulapalli Sep 7, 2022

dbwiddis Sep 7, 2022

peternied left a comment

peternied Sep 8, 2022

dbwiddis Sep 8, 2022

Handle named wildcards (REST path parameters) #123

Handle named wildcards (REST path parameters) #123

Conversation

dbwiddis commented Sep 6, 2022 • edited Loading

Description

Issues Resolved

codecov-commenter commented Sep 6, 2022 • edited Loading

Codecov Report

dbwiddis commented Sep 6, 2022

owaiskazi19 commented Sep 6, 2022

dbwiddis commented Sep 6, 2022 • edited Loading

dbwiddis commented Sep 6, 2022

owaiskazi19 commented Sep 7, 2022 • edited Loading

dbwiddis commented Sep 7, 2022

saratvemulapalli left a comment

Choose a reason for hiding this comment

saratvemulapalli Sep 7, 2022

Choose a reason for hiding this comment

dbwiddis Sep 7, 2022

Choose a reason for hiding this comment

dbwiddis Sep 7, 2022

Choose a reason for hiding this comment

saratvemulapalli Sep 7, 2022

Choose a reason for hiding this comment

dbwiddis Sep 7, 2022

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

peternied Sep 8, 2022

Choose a reason for hiding this comment

dbwiddis Sep 8, 2022

Choose a reason for hiding this comment

dbwiddis commented Sep 6, 2022 •

edited

Loading

codecov-commenter commented Sep 6, 2022 •

edited

Loading

dbwiddis commented Sep 6, 2022 •

edited

Loading

owaiskazi19 commented Sep 7, 2022 •

edited

Loading