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

Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support #24437

Merged
merged 58 commits into from Jul 7, 2017

Conversation

Projects
None yet
6 participants
@dakrone
Copy link
Member

commented May 2, 2017

This is a follow-up of #17916 from @jbertouch's work brought up to date with the latest master, here's a subset of original description:


First pass at closing #15335. The new behavior for the RestController#executeHandler method is as follows:

  • For a request to a valid endpoint with an unsupported HTTP method -> Return a 405 HTTP error including allowed methods for the endpoint in the Allow HTTP header. Refer to HTTP/1.1 - 10.4.6 - 405 Method Not Allowed.
  • For an OPTIONS HTTP method request to a valid endpoint -> Return a 200 HTTP response including allowed methods for the endpoint in the Allow HTTP header. Refer to HTTP/1.1 - 9.2 - Options.

It allows things like the following:

~ λ curl -v -XPOST 'localhost:9200/my_index/_settings'
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9200 (#0)
> POST /my_index/_settings HTTP/1.1
> Host: localhost:9200
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 405 Method Not Allowed
< Allow: PUT,GET
< content-type: application/json; charset=UTF-8
< content-length: 134
< 
{
  "error" : "Incorrect HTTP method for uri [/my_index/_settings] and method [POST], allowed: [PUT, GET]",
  "status" : 405
}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

Where you can see the HTTP/1.1 405 Method Not Allowed response and Allow: GET header.

It also adds support for OPTIONS:

~ λ curl -v -XOPTIONS x:9200/_cluster/settings
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to x (127.0.0.1) port 9200 (#0)
> OPTIONS /_cluster/settings HTTP/1.1
> Host: x:9200
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< Allow: GET,PUT
< content-type: text/plain; charset=UTF-8
< content-length: 0
< 
* Curl_http_done: called premature == 0
* Connection #0 to host x left intact

jbertouch and others added some commits Apr 5, 2016

Improved REST endpoint exception handling, see #15335
Also improved OPTIONS http method handling to better conform with the
http spec.
Capture RestResponse so it can be examined in test cases
Simple addition to surface the RestResponse object so we can run tests
against it (see issue #15335).
Unit test for REST error handling enhancements
Randomizing unit test for enhanced REST response error handling. See
issue #15335 for more details.
New constructor to set HTTP method
Constructor added to support RestController test cases.
Merge remote-tracking branch 'elastic/master' into issue_15335
# Conflicts:
#	core/src/main/java/org/elasticsearch/rest/RestController.java
#
test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest
.java
Further enhancements to request handling
Refactored executeHandler to prioritize explicit path matches. See
#15335 for more information.
Fixed edge case if OPTIONS method randomly selected as invalid method
In this test, an OPTIONS method request is valid, and should not return
a 405 error.
Add PathTrie.retrieveAll where all matching modes can be retrieved
Then TrieMatchingMode can be package private and not leak into RestController
@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2017

@s1monw okay! I've pushed two commits that make some drastic changes to this PR:

The first is a change from PathTrie<RestHandler> to PathTrie<MethodHandlers> so that we can have a single PathTrie for all paths, instead of a separate one for each method. It allows us to retrieve the supported methods for a path in a much easier manner (this was from your suggestion).

The second is a change in PathTrie (and of course RestController) to make it retrieve all of the RestHandlers (now MethodHandlers actually) at once, looping through the candidates and trying each to see if it is valid. This is also from your suggestion. As a byproduct, no outside classes have to know anything about TrieMatchingMode (again, your suggestion) and the class has been made package-private.

@dakrone dakrone requested a review from s1monw Jun 22, 2017

dakrone added some commits Jun 22, 2017

Fix missing usageService handler addition
I accidentally removed this :X
@s1monw
Copy link
Contributor

left a comment

I took another look and I like this. Yet, I think we need to make the change that you did here first in a separate PR and then do the actual change based on that. Is this possible? I'd love to make is smaller so reviews are more effective.

this.path = path;
this.trie = trie;
this.paramSupplier = paramSupplier;
modes.add(TrieMatchingMode.EXPLICIT_NODES_ONLY);

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 28, 2017

Contributor

just use Arrays.asList when this is initializaed?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 28, 2017

Author Member

The modes need to go in a specific order, this ensures that even if someone changes the order in the ENUM definition the matching will still work correctly

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 28, 2017

Author Member

Oh, but I see what you're saying, you're just saying for contructing it, sure I'll change that


@Override
public boolean hasNext() {
if (modes.isEmpty() == false) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 28, 2017

Contributor

return modex.isEmpty() == false?

* @param path Path to handle (e.g., "/{index}/{type}/_bulk")
* @param handler The handler to actually execute
*/
public void registerHandler(RestRequest.Method singleMethod, String path, RestHandler handler) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 28, 2017

Contributor

why did you add this method and why don't you use the registerHandler method above instead?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 28, 2017

Author Member

This is a compatibility layer for all the Rest* actions, if I used only the one above I would have to change every action's registerHandler method call (which I might do, but in a subsequent PR)

}

// Loop through all possible handlers, attempting to dispatch the request
Iterator<MethodHandlers> allHandlers = getAllHandlers(request);

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 28, 2017

Contributor

hmm can't we do for (MethodHandlers handler : ((Iterable<MethodHandlers>) this::getAllHandlers))?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 28, 2017

Author Member

I can't cast the Iterator to Iterable (I tried it and got class cast exceptions), is there a different way to get an Iterable out of an Iterator?

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 28, 2017

Refactor PathTrie and RestController to use a single trie for all met…
…hods

This changes `PathTrie` and `RestController` to use a single `PathTrie` for all
endpoints, it also allows retrieving the endpoints' supported HTTP methods more
easily.

This is a spin-off and prerequisite of elastic#24437
@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2017

Yet, I think we need to make the change that you did here first in a separate PR and then do the actual change based on that.

I opened #25459 as a spin-off encapsulating the MethodHandlers and PathTrie changes

dakrone added a commit that referenced this pull request Jul 5, 2017

Refactor PathTrie and RestController to use a single trie for all met…
…hods (#25459)

* Refactor PathTrie and RestController to use a single trie for all methods

This changes `PathTrie` and `RestController` to use a single `PathTrie` for all
endpoints, it also allows retrieving the endpoints' supported HTTP methods more
easily.

This is a spin-off and prerequisite of #24437

* Use EnumSet instead of multiple if conditions

* Make MethodHandlers package-private and final

* Remove duplicate registerHandler method

* Remove public modifier
@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2017

@s1monw #25459 was merged and I've merged master into this branch, bringing it from +915 lines to +369 (much smaller!), please take another look!

@dakrone dakrone requested a review from s1monw Jul 6, 2017

@s1monw

s1monw approved these changes Jul 7, 2017

Copy link
Contributor

left a comment

this looks good to me. Yet, I think we should have a real REST test for this somehow? WDYT @dakrone we can add a test REST handler in a smoketest qa env where we can add a rest spec that we don't implement, something along those lines? This can be a folllowup IMO

// Get the map of matching handlers for a request, for the full set of HTTP methods.
final Set<RestRequest.Method> validMethodSet = getValidHandlerMethodSet(request);
if (validMethodSet.size() > 0
&& !validMethodSet.contains(request.method())

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 7, 2017

Contributor

I know its controversial but can we use == false ?

// Not Allowed error.
handleUnsupportedHttpMethod(request, channel, validMethodSet);
requestHandled = true;
} else if (!validMethodSet.contains(request.method())

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 7, 2017

Contributor

same here ^^

dakrone added some commits Jul 7, 2017

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

Yet, I think we should have a real REST test for this somehow? WDYT @dakrone we can add a test REST handler in a smoketest qa env where we can add a rest spec that we don't implement, something along those lines?

Sure I will open an issue to add this as a follow-up

Thanks for the reviews @s1monw!

@dakrone dakrone merged commit 8aa0a5c into elastic:master Jul 7, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 7, 2017

Merge branch 'master' into disk-threshold-settings-validation
* master:
  Remove deprecated created and found from index, delete and bulk (elastic#25516)
  fix testEnsureVersionCompatibility for 5.5.0 release
  fix Version.v6_0_0 min compatibility version to 5.5.0
  Add bwc indices for 5.5.0
  Add v5_5_1 constant
  [DOCS] revise high level client Search Scroll API docs (elastic#25599)
  Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437)
  Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254)
  [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 7, 2017

Merge branch 'master' into restore-local-checkpoint-tracker
* master: (42 commits)
  Harden global checkpoint tracker
  Remove deprecated created and found from index, delete and bulk (elastic#25516)
  fix testEnsureVersionCompatibility for 5.5.0 release
  fix Version.v6_0_0 min compatibility version to 5.5.0
  Add bwc indices for 5.5.0
  Add v5_5_1 constant
  [DOCS] revise high level client Search Scroll API docs (elastic#25599)
  Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437)
  Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254)
  [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)
  Enable cross-setting validation
  [Docs] Fix typo in bootstrap-checks.asciidoc (elastic#25597)
  Index ids in binary form. (elastic#25352)
  bwc checkout should fetch from all remotes
  IndexingIT should check for global checkpoints regardless of master version
  [Tests] Add tests for PhraseSuggestionBuilder#build() (elastic#25571)
  Remove unused class MinimalMap (elastic#25590)
  [Docs] Document Scroll API for Java High Level REST Client (elastic#25554)
  Disable date field mapping changing (elastic#25285)
  Allow BWC Testing against a specific branch (elastic#25510)
  ...
@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

Yet, I think we should have a real REST test for this somehow? WDYT @dakrone we can add a test REST handler in a smoketest qa env where we can add a rest spec that we don't implement, something along those lines?

Definitely, I opened #25611 for this.

Thanks for the reviews @s1monw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.