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

Deprecate lenient booleans #22716

Merged

Conversation

Projects
None yet
2 participants
@danielmitterdorfer
Copy link
Member

danielmitterdorfer commented Jan 20, 2017

Elasticsearch 6.0 removes support for lenient
booleans (see #22200). With this commit we
deprecate all usages of non-strict booleans in
Elasticsearch 5.x so users can already spot
improper usages.

Relates #22200
Relates #22696

Deprecate lenient booleans
Elasticsearch 6.0 removes support for lenient
booleans (see #22000). With this commit we
deprecate all usages of non-strict booleans in
Elasticsearch 5.x so users can already spot
improper usages.

Relates #22000
Relates #22696
@danielmitterdorfer

This comment has been minimized.

Copy link
Member Author

danielmitterdorfer commented Jan 20, 2017

@nik9000, @dakrone: Related to #22200, this PR adds deprecation warnings in 5.x for all occurrences of lenient booleans. As you've reviewed #22200, could you please also review this one?

@dakrone
Copy link
Member

dakrone left a comment

LGTM, I left some minor comments

@@ -116,6 +119,10 @@ private AutoCreate(String value) {
List<Tuple<String, Boolean>> expressions = new ArrayList<>();
try {
autoCreateIndex = Booleans.parseBooleanExact(value);
if (Booleans.isStrictlyBoolean(value) == false) {
DEPRECATION_LOGGER.deprecated("Expected a boolean for setting [{}] but got [{}]",

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

I think one should be Expected a boolean [true/false] or index name pattern for setting [{}] but got [{}] since it technically takes more than just a boolean. Otherwise anyone reading the deprecation log could think it doesn't support index name patterns

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Jan 23, 2017

Author Member

Makes sense. I'll change the wording.

lenientNodeBooleanValue(ignoreUnavailableString, defaultSettings.ignoreUnavailable()),
lenientNodeBooleanValue(allowNoIndicesString, defaultSettings.allowNoIndices()),
lenientNodeBooleanValue(ignoreUnavailableString, "ignore_unavailable", defaultSettings.ignoreUnavailable()),
lenientNodeBooleanValue(allowNoIndicesString, "allow_no_indices", defaultSettings.allowNoIndices()),

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

Wish these were static vars instead of hardcoded strings since they're scattered all over this file and the potential for typos is high, but it's not really in the scope of this PR :-/

boolean booleanValue = Booleans.parseBooleanExact(value);
if (Booleans.isStrictlyBoolean(value) == false) {
DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(Setting.class));
deprecationLogger.deprecated("Expected a boolean for setting [{}] but got [{}]", key, value);

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

"Expected a boolean" -> "Expected a boolean [true/false]"

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

I see this is all over the place, so if you don't want to add it, that's okay with me. It's up to you :)

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Jan 23, 2017

Author Member

I think your suggestion is more user-friendly, so I'll change it in all places.

Boolean booleanValue = Booleans.parseBooleanExact(rawValue, defaultValue);
if (rawValue != null && Booleans.isStrictlyBoolean(rawValue) == false) {
DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(Settings.class));
deprecationLogger.deprecated("Expected a boolean for setting [{}] but got [{}]", setting, rawValue);

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

Same here about adding [true/false] to the message

} else if (token == Token.VALUE_STRING) {
return Booleans.parseBoolean(textCharacters(), textOffset(), textLength(), false /* irrelevant */);
rawValue = new String(textCharacters(), textOffset(), textLength());

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

I believe this might need a charset passed into the String constructor?

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Jan 23, 2017

Author Member

We can only specify a Charset when constructing a String from a byte[] but not when constructing from a char[]. The reason is that in Java a char always represents a Unicode scalar value whereas a byte does not (necessarily).

booleanValue = (Boolean) node;
} else if (node instanceof Number) {
interpretedAsLenient = true;
booleanValue = ((Number) node).intValue() != 0;

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

So glad we're getting rid of this so someone doesn't think -1 == false when it actually means true...

This comment has been minimized.

Copy link
@danielmitterdorfer
booleanValue = ((Number) node).intValue() != 0;
} else {
String value = node.toString();
booleanValue = !(value.equals("false") || value.equals("0") || value.equals("off"));

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

Can you change this ! to == false so eyes don't skip over it? :)

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

Actually, I think this is missing parsing "no" as false, in which case this would be breaking, perhaps this should use the helpers in Booleans instead?

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 20, 2017

Member

Hmm I see the original behavior had this inconsistency also (not parsing "no" like all the other boolean methods do)... not sure whether we should try to re-add the consistency or keep the broken behavior.

I'm in favor of re-adding the "no" parsing to be consistent as a semi-bugfix, what do you think? (it's going away in 6.0 anyway...)

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Jan 23, 2017

Author Member

You are right. This is the original behavior and I just moved code around. I have really mixed feelings about fixing it now because it could lead to bugs in client applications that are very hard to find. Despite this I am still slightly in favor of fixing it too because this problem would appear anyway at latest in 6.0. However, I will fix it in a separate commit after merging this PR.

@danielmitterdorfer danielmitterdorfer merged commit 9946abf into elastic:5.x Jan 23, 2017

1 check passed

CLA Commit author has signed the CLA
Details

@danielmitterdorfer danielmitterdorfer removed the request for review from nik9000 Jan 23, 2017

danielmitterdorfer added a commit that referenced this pull request Feb 21, 2017

Generate 5.2.x BWC indices with the 5.2 script
In #22716 we significantly increased the variety of boolean
representations that are created by the BWC script. The script and the
related tests have only been backported to Elasticsearch 5.3 though.
Hence, if we generate an index with a great variety of boolean
representation but `RestoreBackwardsCompatIT#testOldSnapshot()` tests
only one representation it has to fail.

With this commit we regenerate the BWC indices for 5.2.0 and 5.2.1 with
the older version of the script (which is present on the 5.2 branch) and
only generates one representation of boolean fields as expected by the
test.
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.