Skip to content

Do not allow version in Rest Update API #43516

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

Merged
merged 7 commits into from
Jul 16, 2019
Merged

Conversation

PnPie
Copy link
Contributor

@PnPie PnPie commented Jun 23, 2019

The versioning of Update API doesn't rely on version number anymore (and rather on sequence number). But in rest api level we ignored the "version" and "version_type" parameter, so that the server cannot raise the exception when whey were set.

This PR restores "version" and "version_type" parsing in Update Rest API so that we can get the appropriate errors.

Relates to #42497

The versioning of Update API doesn't rely on version number anymore (and rather on sequence number now). But in rest api level we ignored and didn't parse the "version" and "version_type" parameter, so that the server doesn't raise the exception even if we set them.

This PR restores "version" and "version_type" parsing in Update Rest API so that we can get the appropriate errors.
@ywelsch ywelsch added :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue labels Jun 24, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch ywelsch requested a review from dnhatn June 24, 2019 07:58
@dnhatn
Copy link
Member

dnhatn commented Jul 1, 2019

Thank you for working on this PR. I think we should not call UpdateRequest#version() and UpdateRequest#versionType() when parsing an update request because these methods always throw an exception. This would make every update request fail (you can run ./gradlew :distribution:archives:integ-test-zip:check). We can instead throw an exception only if either version or version_type parameter is provided. We might also need to make the behavior of update and delete consistent in a follow-up.

@PnPie
Copy link
Contributor Author

PnPie commented Jul 1, 2019

Hi @dnhatn I see the problem and I updated it to parse version/version_type if provided.

@dnhatn dnhatn self-assigned this Jul 1, 2019
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PnPie I am sorry for being slow on this. I left a comment on the validation. Can you also add a YAML test ensuring that we return bad_request for an update request if version or version_type is provided? Thank you!

PnPie added 2 commits July 14, 2019 17:32
Change exception thrown from UnsupportedOperationException to ActionRequestValidationException
@PnPie
Copy link
Contributor Author

PnPie commented Jul 14, 2019

Hi @dnhatn sorry for being slow on this too. I added a YAML test for this but it throws an exception directly at the YAML test level, before going into rest request parsing part. I see it's because in YAML test update api spec we have removed version/version_type from supported Params. So that means we cannot test this Exception at YAML test layer ?

@dnhatn
Copy link
Member

dnhatn commented Jul 15, 2019

@elasticmachine test this please

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left an optional comment but LGTM. Thanks @PnPie for working on this :).

@dnhatn
Copy link
Member

dnhatn commented Jul 16, 2019

@elasticmachine update branch

@dnhatn
Copy link
Member

dnhatn commented Jul 16, 2019

@elasticmachine test this please

@dnhatn dnhatn merged commit afa9b35 into elastic:master Jul 16, 2019
@dnhatn dnhatn changed the title Restore version parsing for Rest Update API Do not allow version in Rest Update API Jul 16, 2019
dnhatn pushed a commit that referenced this pull request Jul 16, 2019
The versioning of Update API doesn't rely on version number anymore (and
rather on sequence number). But in rest api level we ignored the
"version" and "version_type" parameter, so that the server cannot raise
the exception when whey were set.

This PR restores "version" and "version_type" parsing in Update Rest API
so that we can get the appropriate errors.

Relates to #42497
@dnhatn
Copy link
Member

dnhatn commented Jul 16, 2019

@PnPie Thanks again for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants