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

Allow overriding all-field leniency when lenient option is specified #21504

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Nov 11, 2016

As part of #20925 and #21341 we added an "all-fields" mode to the
query_string and simple_query_string. This would expand the query to
all fields and automatically set lenient to true.

However, we should still allow a user to override the lenient flag to
whichever value they desire, should they add it in the request. This
commit does that.

@jpountz
Copy link
Contributor

jpountz commented Nov 14, 2016

I'm not happy that the the lenient property can now be null and that the default needs to be handled in multiple places. Maybe instead the builders could keep track of whether the lenient property was set and only automatically enable leniency if it was not set?

@dakrone
Copy link
Member Author

dakrone commented Nov 14, 2016

I'm not happy that the the lenient property can now be null and that the default needs to be handled in multiple places. Maybe instead the builders could keep track of whether the lenient property was set and only automatically enable leniency if it was not set?

I agree that I don't like having it set to null indicating "unset" either. I'll try it with your suggestion!

@dakrone
Copy link
Member Author

dakrone commented Nov 14, 2016

@jpountz I pushed another commit that changes the @Nullable in favor of a flag

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.

I left a comment about the bw logic. Otherwise LGTM.

@@ -162,6 +164,9 @@ public SimpleQueryStringBuilder(StreamInput in) throws IOException {
in.readBoolean(); // lowercase_expanded_terms
}
settings.lenient(in.readBoolean());
if (in.getVersion().after(V_5_1_0_UNRELEASED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/after/onOrAfter/ ?

@@ -188,6 +193,9 @@ protected void doWriteTo(StreamOutput out) throws IOException {
out.writeBoolean(true); // lowercase_expanded_terms
}
out.writeBoolean(settings.lenient());
if (out.getVersion().after(V_5_1_0_UNRELEASED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here?

@dakrone dakrone force-pushed the fix-lenient-overriding branch 2 times, most recently from 67407e1 to 864f6d3 Compare November 22, 2016 03:55
As part of elastic#20925 and elastic#21341 we added an "all-fields" mode to the
`query_string` and `simple_query_string`. This would expand the query to
all fields and automatically set `lenient` to true.

However, we should still allow a user to override the `lenient` flag to
whichever value they desire, should they add it in the request. This
commit does that.
@dakrone dakrone merged commit 11da09e into elastic:master Nov 22, 2016
@dakrone
Copy link
Member Author

dakrone commented Nov 22, 2016

Thanks @jpountz!

@dakrone dakrone deleted the fix-lenient-overriding branch January 23, 2017 17:22
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
javanna added a commit to javanna/elasticsearch that referenced this pull request Feb 15, 2019
Looks like elastic#21504 introduced a serialization conditional for lenientSet,
but the PR was never backported to 5.x hence whenever simple query
string is serialized between 6.x and 5.6, it can't be read/written properly causing errors on the transport layer.

This commit updates the conditional to only read/write the field against
6.0+ versions which are aware of it.

Closes elastic#38889
@javanna
Copy link
Member

javanna commented Feb 15, 2019

heya @dakrone seems like this PR was supposed to be backported to 5.x but it was not, yet the version conditionals are assuming the backport happened causing issues like #38889. Could you double check and eventually remove the v5.1.1 label ?

javanna added a commit that referenced this pull request Feb 15, 2019
Looks like #21504 introduced a serialization conditional for lenientSet,
but the PR was never backported to 5.x hence whenever simple query
string is serialized between 6.x and 5.6, it can't be read/written properly causing errors on the transport layer.

This commit updates the conditional to only read/write the field against
6.0+ versions which are aware of it.

Closes #38889
javanna added a commit that referenced this pull request Feb 16, 2019
Looks like #21504 introduced a serialization conditional for lenientSet,
but the PR was never backported to 5.x hence whenever simple query
string is serialized between 6.x and 5.6, it can't be read/written properly causing errors on the transport layer.

This commit updates the conditional to only read/write the field against
6.0+ versions which are aware of it.

Closes #38889
@javanna javanna removed the v5.1.1 label Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants