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

Reject DELETE requests with a body #21453

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
@brunocvcunha
Copy link

commented Nov 10, 2016

According to issues #8217 and #5960, DELETE requests that contains a body should be rejected.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

Hi @brunocvcunha

Thanks for the PR. This is slightly more complicated than just outright rejecting DELETE requests with body because the clear scroll API takes a body. I think we need to deprecate that form of the API and support eg POST _search/clear_scroll instead.

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

This change per se would be ok as it only rejects delete api calls with a body. Maybe we should define a better error message, but most important is that other apis should get the same change, pretty much every api that registers a DELETE endpoint:

  • RestDeleteIndexAction
  • RestIndexDeleteAliasesAction
  • RestDeleteIndexTemplateAction
  • RestDeleteStoredScriptAction
  • RestDeleteSnapshotAction
  • RestDeleteRepositoryAction
  • RestDeletePipelineAction
  • RestClearScrollAction
  • RestDeleteSearchTemplateAction

If all of these API need to change, then we may want to make the change once in RestController rather than in each and every delete action. But before we make the generic change we have to address the clear scroll api as that one does something with the body when provided.

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

@clintongormley we also support deleting a scroll by providing the id as a query_string parameter. Isn't that enough? Do we have to add POST with a body?

@dadoonet

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

@javanna I did not look at the code but delete by query needs a body, right?

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

@dadoonet

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

oh? Thanks @javanna. I was not aware of that change.
Then I agree with your comment:

we may want to make the change once in RestController

@clintongormley

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

we also support deleting a scroll by providing the id as a query_string parameter. Isn't that enough? Do we have to add POST with a body?

No it is not enough, unfortunately. It is quite easy to exceed the HTTP header length with clear-scroll requests against many shards

@brunocvcunha

This comment has been minimized.

Copy link
Author

commented Nov 10, 2016

Thanks for the suggestions guys.

This handler is only registered for the /{index}/{type}/{id} endpoint. The _scroll requests get processed by RestClearScrollAction, which isn't included in this PR.

As of the message, I took some others as example, and it follows the same format/level of detail.
core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java: throw new IllegalArgumentException("Failed to parse request body", e);. Any suggestions? I think anything more than just "can't specify a request body" will become redundant.

I could extend the PR to other endpoints, and take a look on how this could be handled by the RestController itself, but there are some DELETE endpoints that require body as mentioned above, so I'm not sure if the controller should have this knowledge.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

We prefer pull requests against our master branch.

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

hi @brunocvcunha thanks for your contribution!

The only delete endpoint left that requires a body is clear scroll, which we are going to deprecate in 5.x. We can then make the generic change to RestController safely in master (future 6.x) where no DELETE endpoint will require a body. I can work on the clear scroll changes needed in the coming days and you can take a spin at updating this PR if you are up for it.

I defer to @clintongormley for the error message that we want to return.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

Re the exception message, how about: DELETE requests may not contain a request body?

javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 23, 2016

Add POST /_search/clear_scroll endpoint and deprecate delete scroll e…
…ndpoint

The clear scroll api currently allows to provide a scroll by specifying it either as part of the url (it is effectively the resource that gets deleted) or within the request body. The current api uses the DELETE method though, and we have decided to remove support for providing the request body with any DELETE endpoint in the future. In order to get to this for the next major version, we introduce the  new endpoint `POST /_search/clear_scroll` which replaces the current clear_scroll api and uses POST instead of DELETE. It allows to provide the `scroll_id` as a url parameter, which is though deprecated (will output a deprecation warning when used) in favour of providing it as part of the request body.

 The `DELETE /_search/scroll/` is deprecated, hence it will output a deprecation warning whenever used. The DELETE endpoints will be removed in 6.0, as well as the support for providing the scroll_id as a url parameter against the POST endpoint.

Relates to elastic#8217
Relates to elastic#21453
@rjernst

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

@brunocvcunha Are you interested in updating this PR given the suggestions made?

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2017

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?

@rjernst

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

Apologies, I see now you already made the suggested wording change. In the future, you should ping reviewers after updating a PR.

@elasticmachine test this please

@rjernst

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

Ah, @brunocvcunha if you are still interested in this change, can you cherry pick to master and force push to this branch?

@javanna

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

@rjernst this PR requires #21510 which is stalled as it requires #21888. I will label this PR as stalled for now. I need to find time to implement #21888 and then get #22510 in with proper tests. After that I will get back to this change.

@javanna javanna self-assigned this Jun 9, 2017

@javanna javanna added the stalled label Jun 9, 2017

@javanna javanna self-requested a review Jun 9, 2017

@martijnvg martijnvg closed this Aug 2, 2017

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

why did you close this @martijnvg ?

@javanna javanna changed the base branch from 5.x to 5.6 Aug 3, 2017

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2017

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?

@javanna javanna reopened this Aug 3, 2017

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

I get that this has been stalled for a long time, but do we have any chance of un-stalling it? Should we start fresh on the issue once we've unblocked the prerequisites? @brunocvcunha, are you still interested in this after all this time?

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2018

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?

@javanna

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@nik9000 yea we have a chance to un-stall it if somebody gets to work on #21888 so that we can finally properly test bw comp against changes like this and #21510.

@tomcallahan tomcallahan removed the stalled label Dec 6, 2018

@jasontedor

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

No additional feedback, closing.

@jasontedor jasontedor closed this Dec 6, 2018

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.