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

Parse empty first line in msearch request body as action metadata #41011

Merged
merged 1 commit into from
May 1, 2019

Conversation

javanna
Copy link
Member

@javanna javanna commented Apr 9, 2019

It looks like when parsing the msearch request body, we allow for the first line to be empty, yet that causes some different treatment for the first line that ends up requiring the action metadata line, while all other lines don't as they can be just empty.

With this change we properly accept the following which we would otherwise reject due to the first line being empty:


{ "query": {"match_all": {}}}

{ "query": {"match_all": {}}}

but we stop accepting the following (note the empty line before the first action metadata line:


{}
{ "query": {"match_all": {}}}

{ "query": {"match_all": {}}}

Relates to #39841

@javanna javanna added the :Search/Search Search-related issues that do not fall into other categories label Apr 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@javanna javanna added the >bug label Apr 9, 2019
@javanna javanna marked this pull request as ready for review April 9, 2019 18:09
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

LGTM, I think failing on the second example (empty line and then an empty header line) is something to be expected

@javanna
Copy link
Member Author

javanna commented Apr 23, 2019

@jimczi do you have thoughts on whether this should be considered a breaking change, hence go to master only, or not?

@jimczi
Copy link
Contributor

jimczi commented Apr 23, 2019

If it's a breaking change we also need to deprecate this format in 7x so would it be possible to log a deprecation warning if the first line is empty in 7x ? It's not a major bug so I guess it's ok to fix only in 8 and warn users about the deprecated format in 7.1 ?

javanna added a commit to javanna/elasticsearch that referenced this pull request Apr 23, 2019
In order to support for empty action metadata in the first msearch item,
we need to remove support for prepending msearch request body with an
empty line, which prevents us from parsing the empty line as action
metadata for the first search item.

Relates to elastic#41011
@javanna
Copy link
Member Author

javanna commented Apr 23, 2019

ok @jimczi I did not manage to have both behaviours work at the same time, but I opened #41442 for the deprecation, which should be enough to push the "breaking" change to master.

javanna added a commit that referenced this pull request Apr 25, 2019
In order to support empty action metadata in the first msearch item,
we need to remove support for prepending msearch request body with an
empty line, which prevents us from parsing the empty line as action
metadata for the first search item.

Relates to #41011
@javanna javanna changed the title Support empty first line in msearch request body Parse empty first line in msearch request body as action metadata Apr 25, 2019
@javanna javanna merged commit 8508795 into elastic:master May 1, 2019
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
With elastic#41442 we have deprecated support for empty line before any action metadata in msearch API. With this commit we remove support for such empty line, in place of parsing it as empty action metadata, which was previously not supported although the following items could have an empty line representing their corresponding action metadata. This way we make all times equal.

Relates to elastic#39841
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
With elastic#41442 we have deprecated support for empty line before any action metadata in msearch API. With this commit we remove support for such empty line, in place of parsing it as empty action metadata, which was previously not supported although the following items could have an empty line representing their corresponding action metadata. This way we make all times equal.

Relates to elastic#39841
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 30, 2021
The support for this buggy behaviour was removed in elastic#41011
however since this is a change that affects a request shape it should
still be available to use it in rest api compatibility

relates elastic#51816
pgomulka added a commit that referenced this pull request Aug 5, 2021
The support for this lenient behaviour was removed in #41011
however since this is a change that affects a request shape it should
still be available to use it in rest api compatibility

relates #51816
jrodewig added a commit that referenced this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Search/Search Search-related issues that do not fall into other categories v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants