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

Fail _search request with trailing tokens #29428

Merged
merged 5 commits into from Apr 11, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Apr 9, 2018

This change validates that the _search request does not have trailing
tokens after the main object and fails the request with a parsing exception otherwise.

Closes #28995

This change validates that the `_search` request does not have trailing
tokens after the main object and fails the request with a parsing exception otherwise.

Closes elastic#28995
@jimczi jimczi added >bug >breaking :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Apr 9, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Looks good. Let's turn the error into a deprecation log on 6.x?

@jimczi jimczi added the v6.3.0 label Apr 9, 2018
@@ -951,12 +951,18 @@ private SearchSourceBuilder shallowCopy(QueryBuilder queryBuilder, QueryBuilder
return rewrittenBuilder;
}

public void parseXContent(XContentParser parser) throws IOException {
parseXContent(parser, false);
Copy link
Contributor

@colings86 colings86 Apr 10, 2018

Choose a reason for hiding this comment

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

Should this default not be the other way around (pass in true by default)? The only time I think we should allow trailing tokens is in the _msearch API which is a special case. It seems safer to me to check and error on trailing tokens in the default case and then have the _msearch API use the more specialised method below? or is there something I am missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I pushed c6a113b to change the default

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

@jimczi jimczi merged commit 1b6d5e5 into elastic:master Apr 11, 2018
@jimczi jimczi deleted the invalid_search_source_parsing branch April 11, 2018 11:10
@jimczi
Copy link
Contributor Author

jimczi commented Apr 11, 2018

Thanks @jpountz @colings86
I'll turn the error into a deprecation log for the backport in 6x.

jimczi added a commit that referenced this pull request Apr 11, 2018
This change removes the check for extra tokens when parsing a source generated by a templated
_msearch request. This was added unintentionally in #29428 but the intent of this modification was to validate
simple _search request only.
jimczi added a commit that referenced this pull request Apr 12, 2018
This change validates that the _search request does not have trailing
tokens after the main object and prints a deprecation message otherwise.

Relates #29428
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >bug :Search/Search Search-related issues that do not fall into other categories v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants