RestUpgradeAction : remove explicit http method check #28992
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
@lastticmachine test this please |
|
Pinging @elastic/es-core-infra |
jasontedor
left a comment
jasontedor
left a comment
There was a problem hiding this comment.
I left a suggestion; I’m good either way.
| throw new IllegalArgumentException("illegal method [" + request.method() + "] for request [" + request.path() + "]"); | ||
| } | ||
| } | ||
| return handlePost(request, client); |
There was a problem hiding this comment.
Maybe only replace the else statement with assert false?
| throw new IllegalArgumentException("illegal method [" + request.method() + "] for request [" + request.path() + "]"); | ||
| } | ||
| assert false; | ||
| return null; |
There was a problem hiding this comment.
I would prefer not to use assert false because of the dummy return..
There was a problem hiding this comment.
That's fine; throw an AssertionError then. Also, I think that you can keep the existing structure: if / else if / else.
There was a problem hiding this comment.
Then the only change is actually the type of the exception which does not make it really simpler ...
|
Throwing the assertion communicates very strongly "I don't expect another type of method" so I think it is a positive change. Not a huge change, but a positive one. |
|
Agreed. But at the same time, the error is thrown only when a handler has been registered to the controller and no handling of the new http method is added to the I think that it would be cleaner if the handling of |
I think this sounds better to me, I don't like having this class do double-duty |
|
+1 |
|
++ thanks @olcbean |
7 participants
The check if an HTTP method is supported is performed in the
RestController( if a method different thatGET/POSTis used for_upgrade, theRestControllerthrows"Incorrect HTTP method for uri [/_upgrade] and method [PUT], allowed: [GET, POST]")This PR simply removed the unnecessary / unreachable explicit check for the
_upgradeendpointRelates to #24437
CC @dakrone