-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Make boolean conversion strict #22200
Make boolean conversion strict #22200
Conversation
This PR removes all leniency in the conversion of Strings to booleans: "true" is converted to the boolean value `true`, "false" is converted to the boolean value `false`. Everything else raises an error.
+1 This:
has always offended me; it's too easy to make a typo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. I think it is a good choice.
return true; | ||
} | ||
|
||
throw new IllegalArgumentException("Failed to parse value [" + value + "] cannot be parsed to boolean [ true/1/on/yes OR false/0/off/no ]"); | ||
throw new IllegalArgumentException("Failed to parse value [" + value + "] as boolean (only 'true' and 'false' are allowed)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Failed to parse value [" + value + "] as only [true] or [false] are allowed."
is a easier to read.
*/ | ||
public static boolean isExplicitFalse(String value) { | ||
return value != null && (value.equals("false") || value.equals("0") || value.equals("off") || value.equals("no")); | ||
return value != null && value.equals("false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"false".equals(value)
?
@@ -196,7 +196,13 @@ public long paramAsLong(String key, long defaultValue) { | |||
|
|||
@Override | |||
public boolean paramAsBoolean(String key, boolean defaultValue) { | |||
return Booleans.parseBoolean(param(key), defaultValue); | |||
String rawParam = param(key); | |||
// treat the sheer presence of a parameter as "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Treat empty string as true
because that allows the presence of the url parameter to mean "turn this on"".
String[] booleans = new String[]{"true", "false", "on", "off", "yes", "no", "0", "1"}; | ||
String[] notBooleans = new String[]{"11", "00", "sdfsdfsf", "F", "T"}; | ||
String[] booleans = new String[]{"true", "false"}; | ||
String[] notBooleans = new String[]{"11", "00", "sdfsdfsf", "F", "T", "on", "off", "yes", "no", "0", "1"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+++++++++++
assertThat(Booleans.isBoolean(null, 0, 1), is(false)); | ||
|
||
for (String b : booleans) { | ||
String t = "prefix" + b + "suffix"; | ||
assertThat("failed to recognize [" + b + "] as boolean", Booleans.isBoolean(t.toCharArray(), "prefix".length(), b.length()), Matchers.equalTo(true)); | ||
assertThat("failed to recognize [" + b + "] as boolean", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertTrue
?
assertThat(Booleans.parseBoolean("true", randomFrom(Boolean.TRUE, Boolean.FALSE, null)), is(true)); | ||
assertThat(Booleans.parseBoolean("false", randomFrom(Boolean.TRUE, Boolean.FALSE, null)), is(false)); | ||
expectThrows(IllegalArgumentException.class, | ||
() -> Booleans.parseBoolean("true".toUpperCase(Locale.ROOT),randomFrom(Boolean.TRUE, Boolean.FALSE, null))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/"true".toUpperCase(Locale.ROOT)
/TRUE
/?
@@ -1,6 +1,12 @@ | |||
[[breaking_60_settings_changes]] | |||
=== Settings changes | |||
|
|||
==== General changess | |||
|
|||
Previously, Elasticsearch recognized the strings "true","false","on","off","yes","no","0","1" as booleans. Elasticsearch 6.0 recognizes only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
, false,
on,
off`....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With backticks, I mean.
@nik9000 Thanks for your comments. They all make sense and I'll address them. |
I don't care about strict booleans in settings, mappings, query string params, etc. The one place I do care about it is in boolean fields in documents. My initial thought was: we allow coercion on other field types by default, why are we suddenly strict on boolean fields? Why not also accept 0/1 as false/true? Of course in Perl, the numbers 0 and 1 might be rendered as strings in JSON, so you'd need to accept "0" and "1" as well. But "0" is true in many languages, and in fact 0 is true in Ruby as well. In other words, whatever coercion you allow, somebody is going to be surprised. Much better to shout loud, shout early. So what I'd like to see:
|
* Returns <code>true</code> iff the value is either of the following: | ||
* <tt>false</tt>, <tt>0</tt>, <tt>off</tt>, <tt>no</tt> | ||
* otherwise <code>false</code> | ||
* @return <code>true</code> iff the value is <tt>false</tt>, otherwise <code>false</code>. | ||
*/ | ||
public static boolean isExplicitFalse(String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could be renamed to isFalse() / isTrue() now
Everywhere. In the API and in documents.
That makes this much more complicated but I think it is worth it. @danielmitterdorfer, for testing see this trick that lets you create an index on the current version that "looks like" it was made on a previous version. It makes testing this kind of thing much easier. |
I've addressed all initial review comments now (although I did not comment on each one). Thanks to @nik9000 for the pointer to the BWC trick. I'll start now to add BWC. |
To clarify, I meant this comment for boolean fields only, not for other booleans associated with an index (including mappings, settings etc). |
@nik9000 Can you spare some cycles on the review? |
Sure! I was hoping someone else would give it a deep dive but I will. Not today, though. I'll save it for tomorrow I think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a bunch of comments, mixing the mundane ("please add a space here") with the important ("we should remove the default here" and "I think we should support this behavior you've documented as unsupported").
try { | ||
partial(nodeBooleanValue(entry.getValue())); | ||
} catch (IllegalArgumentException ex) { | ||
throw new IllegalArgumentException("Could not convert [partial] to boolean.", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is worth having a version of nodeBooleanValue
that tasks a string for the name of the field being converted. I know we have a few methods like that in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's a good idea. I'll need to check the call-sites.
* @param offset offset to start | ||
* @param length length to check | ||
* | ||
* @deprecated Only kept to provide automatic upgrades for pre 6.0 indices. Use {@link #isBoolean(char[], int, int)} instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is only kept for BWC. You use this to parse _source
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked usages of all Booleans.*lenient()
but I didn't see the code you're referring to. However, if it's just about the wording of the Javadoc I'm fine with changing it. The gist of the Javadocs should be: "Don't use it, we just need it during a transition phase and it will be removed."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you use it indirectly through parser. isBooleanValueLenient
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that one. XContentParser#isBooleanValueLenient()
is here for BWC as well so I think the comment is fine.
*/ | ||
@Deprecated | ||
public Boolean getAsBooleanLenientForPreEs6Indices(Version indexVersion, String setting, Boolean defaultValue) { | ||
if (indexVersion.before(Version.V_6_0_0_alpha1_UNRELEASED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test for this you can assert that the minimum supported version is before 6.0.0_alpha1
with a message to remove the whole method after the assertion fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever! I'll do that.
*/ | ||
boolean isBooleanValue() throws IOException; | ||
@Deprecated | ||
boolean isBooleanValueLenient() throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as last comment.
//Only emit a warning if the setting's value is not a proper boolean | ||
final String value = get(setting, "false"); | ||
if (Booleans.isBoolean(value) == false) { | ||
@SuppressWarnings("deprecation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it does anything to suppress deprecated warnings when you are in a deprecated method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale around this whole suppression was: deprecated methods get the @Deprecate
annotation but I don't want to spit out unnecessary additional deprecation warnings when I (intentionally) call deprecated API in these methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me.
|
||
public void testAddWithValidSourceValueIsAccepted() throws Exception { | ||
XContentParser parser = createParser(XContentFactory.jsonBuilder() | ||
.startObject() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for indenting!
.addMapping("type1", XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties") | ||
.startObject("field2").field("type", "text").field("store", "no").endObject() | ||
.endObject().endObject().endObject()) | ||
.addMapping("type1", XContentFactory.jsonBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -160,8 +160,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli | |||
final HashMap<String, String> params = new HashMap<>(); | |||
params.put("format", randomAsciiOfLength(8)); | |||
params.put("filter_path", randomAsciiOfLength(8)); | |||
params.put("pretty", randomAsciiOfLength(8)); | |||
params.put("human", randomAsciiOfLength(8)); | |||
params.put("pretty", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
randomFrom("true", "false", "", null)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the test didn't care what the value was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct; the test did not care about that but the handler still parses the value so I think it makes sense to randomize it.
mappings immediately and can also read existing documents but for example the following operations will not be possible (even on | ||
pre-6.0 indices): | ||
|
||
* Adding new documents to existing indices that violate the strict `boolean` coercion rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is possible, right? Or, at least, we want it to be. If the index is pre-6.0 adding a boolean field like "field": 0
should accept it as a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this still works (just verified locally). :) I'll remove that section from the docs.
pre-6.0 indices): | ||
|
||
* Adding new documents to existing indices that violate the strict `boolean` coercion rules. | ||
* Creating new indices from index templates that violate the strict `boolean` coercion rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rewrite this in some way, merging it with the sentence above, saying something like "even new documents created with old templates."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten that part (and the sentence above is gone now).
@nik9000 Thanks for your comments, very helpful! I've pushed a few more commits that address all your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'm glad it'll get another set of eyes before merging.
@@ -1217,7 +1217,8 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti | |||
* {@link #isIndexUsingShadowReplicas(org.elasticsearch.common.settings.Settings)}. | |||
*/ | |||
public static boolean isOnSharedFilesystem(Settings settings) { | |||
return settings.getAsBoolean(SETTING_SHARED_FILESYSTEM, isIndexUsingShadowReplicas(settings)); | |||
Version version = settings.getAsVersion(SETTING_VERSION_CREATED, Version.CURRENT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it is a good idea to have a default here? This shouldn't be missing in production, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that one and will change it.
* @param <U> the type of the third argument | ||
* @param <R> the return type | ||
* | ||
* @since 1.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is @since
anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a leftover; I'll remove it.
@@ -256,7 +257,7 @@ public void addIndexStore(String type, Function<IndexSettings, IndexStore> provi | |||
* @param name Name of the SimilarityProvider | |||
* @param similarity SimilarityProvider to register |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to have an interface for building the similarity. That way you can document what the two Settings
are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this too but I figured that given it is temporary (i.e. this will be again a BiFunction
after the cleanup task #22298 is done) it might be ok. But I might need to ponder once more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left pretty minor comments which are pretty much optional, happy to have this go in!
@@ -1217,7 +1217,8 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti | |||
* {@link #isIndexUsingShadowReplicas(org.elasticsearch.common.settings.Settings)}. | |||
*/ | |||
public static boolean isOnSharedFilesystem(Settings settings) { | |||
return settings.getAsBoolean(SETTING_SHARED_FILESYSTEM, isIndexUsingShadowReplicas(settings)); | |||
Version version = settings.getAsVersion(SETTING_VERSION_CREATED, Version.CURRENT); | |||
return settings.getAsBooleanLenientForPreEs6Indices(version, SETTING_SHARED_FILESYSTEM, isIndexUsingShadowReplicas(settings)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the settings
object here already have access to the Version
by retrieving the version inside the getAsBooleanLenientForPreEs6Indices
method, so it can be skipped retrieving it outside of the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an additional arity version that didn't require the version and automatically looked it up from itself, that could save you some lookups (if you want)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the indexCreatedVersion
of IndexMetaData
now (just had to turn this into an instance method) and have also provided an overloaded version without parameters.
} | ||
|
||
public static Boolean parseBoolean(String value, Boolean defaultValue) { | ||
if (value == null || value.length() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think value.length() == 0
here should be String.hasText(value)
, so it checks for " "
and uses the default value for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used Strings.hasText(value)
in all places where it's possible.
if (value == null || value.length() == 0) { | ||
return defaultValue; | ||
} | ||
return parseBoolean(value); | ||
} | ||
|
||
public static Boolean parseBoolean(String value, Boolean defaultValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where we need a @Nullable
version of parseBoolean
and a non-nullable version? Do we use the null places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are some places indeed. For example, we use it when parsing REST request parameters in RestActions#urlParamsToQueryBuilder(RestRequest)
.
* @deprecated Only kept to provide automatic upgrades for pre 6.0 indices. Use {@link #parseBoolean(char[], int, int, boolean)} instead | ||
*/ | ||
@Deprecated | ||
public static boolean parseBooleanLenient(char[] text, int offset, int length, boolean defaultValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be public, or can we make it private to this class and force everyone to go through the String version of the parseBooleanLenient
? (I did a cursory glance and didn't see usages, but it's possible I missed some)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind, I see where we use it :-/
* @since 1.8 | ||
*/ | ||
@FunctionalInterface | ||
public interface TriFunction<S, T, U, R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why the S
, T
, and U
instead of something like A
, B
, and C
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because TriFunction
is really just java.util.BiFunction
plus an additional parameter.
try { | ||
return nodeBooleanValue(node, defaultValue); | ||
} catch (IllegalArgumentException ex) { | ||
throw new IllegalArgumentException("Could not convert [" + name + "] to boolean.", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's super minor, but I think we usually don't include punctuation at the end of exception messages (the .
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
if (node instanceof Number) { | ||
return ((Number) node).intValue() != 0; | ||
public static boolean nodeBooleanValue(Object node, boolean defaultValue) { | ||
String nodeValue = node != null ? node.toString() : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also minor, but I think I'd prefer node == null ? null : node.toString()
because it requires less negative-resolving in my brain, up to you though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity I ran a grep over the codebase and your idiom wins (685 vs. 240 matches). Personally I prefer "my" style but as I value consistency more, I'll happily change it. :)
// TODO: remove this leniency in 6.0 | ||
if (BOOLEAN_STRINGS.contains(node.toString()) == false) { | ||
DEPRECATION_LOGGER.deprecated("Expected a boolean for property [{}] but got [{}]", name, node); | ||
//TODO 22298: Remove this method and have all call-sites use <code>XContentMapValues.nodeBooleanValue(node)</code> directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add an assert that it's not ES 7.x here so we know to remove it
private static void parseAnalyzersAndTermVectors(FieldMapper.Builder builder, String name, Map<String, Object> fieldNode, Mapper.TypeParser.ParserContext parserContext) { | ||
|
||
private static void parseAnalyzersAndTermVectors(FieldMapper.Builder builder, String name, Map<String, Object> fieldNode, Mapper | ||
.TypeParser.ParserContext parserContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems strange to split the string here rather than moving Mapper.etc
to a new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That was a leftover.
@@ -117,7 +135,8 @@ private static void parseAnalyzersAndTermVectors(FieldMapper.Builder builder, St | |||
} | |||
|
|||
if (searchAnalyzer == null && searchQuoteAnalyzer != null) { | |||
throw new MapperParsingException("analyzer and search_analyzer on field [" + name + "] must be set when search_quote_analyzer is set"); | |||
throw new MapperParsingException("analyzer and search_analyzer on field [" + name + "] must be set when search_quote_analyzer" + | |||
" is set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not break at the existing +
next to name
instead of adding a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not? I've changed it.
…It basically means that no settings should be used before Log4j's status logger has been configured without configuration by LogConfigurator.configureWithoutConfig() method.
This PR removes all leniency in the conversion of Strings to booleans: "true" is converted to the boolean value
true
, "false" is converted to the boolean valuefalse
. Everything else raises an error.This applies to:
Exceptions are:
?pretty
is interpreted as?pretty=true
.Noteworthy / to discuss:
1
and0
for thekey
, and the strings"true"
and"false"
for thekey_as_string
(I think that's fine).Example scenario (the
lowercase
field ofmy_custom_pattern_analyzer
is a boolean field):Upgrade to Elasticsearch 6
Create the following index:
Result: Creating the index will fail, because the
lowercase
setting is invalid for a 6.x index.Analysis: When a new index is created,
MetaDataCreateIndexService
will load all matching index templates, merge them settings and apply request settings overrides. By the time the index is created we have no clue where the affected setting came from and before that we don't have enough information to know a value's type.Discussion:
I can think of the following options to address this:
To me, option 1 seems the only feasible one (and the doc update is contained in the PR) but I want to put that up for discussion.