Deprecate lenient booleans

buildSrc/src/main/resources/checkstyle_suppressions.xml

@@ -656,7 +656,6 @@
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]cluster[/\\]settings[/\\]ClusterSettingsIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]cluster[/\\]shards[/\\]ClusterSearchShardsIT.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]cluster[/\\]structure[/\\]RoutingIteratorTests.java" checks="LineLength" />
-  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]BooleansTests.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]blobstore[/\\]FsBlobStoreContainerTests.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]blobstore[/\\]FsBlobStoreTests.java" checks="LineLength" />
   <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]breaker[/\\]MemoryCircuitBreakerTests.java" checks="LineLength" />

core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java

@@ -43,6 +43,7 @@
 import static org.elasticsearch.common.settings.Settings.writeSettingsToStream;
 import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
 import static org.elasticsearch.common.xcontent.support.XContentMapValues.lenientNodeBooleanValue;
+import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
 
 /**
  * Create snapshot request
@@ -366,14 +367,14 @@ public CreateSnapshotRequest source(Map<String, Object> source) {
                     throw new IllegalArgumentException("malformed indices section, should be an array of strings");
                 }
             } else if (name.equals("partial")) {
-                partial(lenientNodeBooleanValue(entry.getValue()));
+                partial(lenientNodeBooleanValue(entry.getValue(), name));
             } else if (name.equals("settings")) {
                 if (!(entry.getValue() instanceof Map)) {
                     throw new IllegalArgumentException("malformed settings section, should indices an inner object");
                 }
                 settings((Map<String, Object>) entry.getValue());
             } else if (name.equals("include_global_state")) {
-                includeGlobalState = lenientNodeBooleanValue(entry.getValue());
+                includeGlobalState = lenientNodeBooleanValue(entry.getValue(), name);
             }
         }
         indicesOptions(IndicesOptions.fromMap((Map<String, Object>) source, IndicesOptions.lenientExpandOpen()));

core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

@@ -481,16 +481,16 @@ public RestoreSnapshotRequest source(Map<String, Object> source) {
                     throw new IllegalArgumentException("malformed indices section, should be an array of strings");
                 }
             } else if (name.equals("partial")) {
-                partial(lenientNodeBooleanValue(entry.getValue()));
+                partial(lenientNodeBooleanValue(entry.getValue(), name));
             } else if (name.equals("settings")) {
                 if (!(entry.getValue() instanceof Map)) {
                     throw new IllegalArgumentException("malformed settings section");
                 }
                 settings((Map<String, Object>) entry.getValue());
             } else if (name.equals("include_global_state")) {
-                includeGlobalState = lenientNodeBooleanValue(entry.getValue());
+                includeGlobalState = lenientNodeBooleanValue(entry.getValue(), name);
             } else if (name.equals("include_aliases")) {
-                includeAliases = lenientNodeBooleanValue(entry.getValue());
+                includeAliases = lenientNodeBooleanValue(entry.getValue(), name);
             } else if (name.equals("rename_pattern")) {
                 if (entry.getValue() instanceof String) {
                     renamePattern((String) entry.getValue());

core/src/main/java/org/elasticsearch/action/admin/indices/flush/TransportShardFlushAction.java

@@ -23,7 +23,7 @@
 import org.elasticsearch.action.support.replication.ReplicationResponse;
 import org.elasticsearch.action.support.replication.TransportReplicationAction;
 import org.elasticsearch.cluster.action.shard.ShardStateAction;
-import org.elasticsearch.cluster.block.ClusterBlockLevel;
+import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.inject.Inject;
@@ -68,7 +68,7 @@ protected ReplicaResult shardOperationOnReplica(ShardFlushRequest request, Index
     }
 
     @Override
-    protected boolean shouldExecuteReplication(Settings settings) {
+    protected boolean shouldExecuteReplication(IndexMetaData indexMetaData) {
         return true;
     }
 }

core/src/main/java/org/elasticsearch/action/admin/indices/refresh/TransportShardRefreshAction.java

@@ -24,7 +24,7 @@
 import org.elasticsearch.action.support.replication.ReplicationResponse;
 import org.elasticsearch.action.support.replication.TransportReplicationAction;
 import org.elasticsearch.cluster.action.shard.ShardStateAction;
-import org.elasticsearch.cluster.block.ClusterBlockLevel;
+import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.inject.Inject;
@@ -68,7 +68,7 @@ protected ReplicaResult shardOperationOnReplica(BasicReplicationRequest request,
     }
 
     @Override
-    protected boolean shouldExecuteReplication(Settings settings) {
+    protected boolean shouldExecuteReplication(IndexMetaData indexMetaData) {
         return true;
     }
 }

core/src/main/java/org/elasticsearch/action/support/AutoCreateIndex.java

@@ -24,6 +24,8 @@
 import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.collect.Tuple;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Setting;
@@ -40,6 +42,7 @@
  * a write operation is about to happen in a non existing index.
  */
 public final class AutoCreateIndex {
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(AutoCreateIndex.class));
 
     public static final Setting<AutoCreate> AUTO_CREATE_INDEX_SETTING =
         new Setting<>("action.auto_create_index", "true", AutoCreate::new, Property.NodeScope, Setting.Property.Dynamic);
@@ -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 [true/false] or index name pattern for setting [{}] but got [{}]",
+                        AUTO_CREATE_INDEX_SETTING.getKey(), value);
+                }
             } catch (IllegalArgumentException ex) {
                 try {
                     String[] patterns = Strings.commaDelimitedListToStringArray(value);

core/src/main/java/org/elasticsearch/action/support/IndicesOptions.java

@@ -195,8 +195,8 @@ public static IndicesOptions fromParameters(Object wildcardsString, Object ignor
 
         //note that allowAliasesToMultipleIndices is not exposed, always true (only for internal use)
         return fromOptions(
-                lenientNodeBooleanValue(ignoreUnavailableString, defaultSettings.ignoreUnavailable()),
-                lenientNodeBooleanValue(allowNoIndicesString, defaultSettings.allowNoIndices()),
+                lenientNodeBooleanValue(ignoreUnavailableString, "ignore_unavailable", defaultSettings.ignoreUnavailable()),
+                lenientNodeBooleanValue(allowNoIndicesString, "allow_no_indices", defaultSettings.allowNoIndices()),
                 expandWildcardsOpen,
                 expandWildcardsClosed,
                 defaultSettings.allowAliasesToMultipleIndices(),

core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java

@@ -312,7 +312,7 @@ public void handleException(TransportException exp) {
                 } else {
                     setPhase(replicationTask, "primary");
                     final IndexMetaData indexMetaData = clusterService.state().getMetaData().index(request.shardId().getIndex());
-                    final boolean executeOnReplicas = (indexMetaData == null) || shouldExecuteReplication(indexMetaData.getSettings());
+                    final boolean executeOnReplicas = (indexMetaData == null) || shouldExecuteReplication(indexMetaData);
                     final ActionListener<Response> listener = createResponseListener(primaryShardReference);
                     createReplicatedOperation(request, new ActionListener<PrimaryResult>() {
                         @Override
@@ -876,8 +876,8 @@ public void onFailure(Exception e) {
      * Indicated whether this operation should be replicated to shadow replicas or not. If this method returns true the replication phase
      * will be skipped. For example writes such as index and delete don't need to be replicated on shadow replicas but refresh and flush do.
      */
-    protected boolean shouldExecuteReplication(Settings settings) {
-        return IndexMetaData.isIndexUsingShadowReplicas(settings) == false;
+    protected boolean shouldExecuteReplication(IndexMetaData indexMetaData) {
+        return IndexMetaData.isIndexUsingShadowReplicas(indexMetaData.getSettings()) == false;
     }
 
     class PrimaryShardReference implements ReplicationOperation.Primary<Request, ReplicaRequest, PrimaryResult>, Releasable {

core/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java

@@ -19,6 +19,8 @@
 package org.elasticsearch.cluster.metadata;
 
 import org.elasticsearch.common.Booleans;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
 
@@ -28,12 +30,17 @@
  * based on the number of datanodes in the cluster. This class handles all the parsing and streamlines the access to these values.
  */
 final class AutoExpandReplicas {
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(AutoExpandReplicas.class));
+
     // the value we recognize in the "max" position to mean all the nodes
     private static final String ALL_NODES_VALUE = "all";
     public static final Setting<AutoExpandReplicas> SETTING = new Setting<>(IndexMetaData.SETTING_AUTO_EXPAND_REPLICAS, "false", (value) -> {
         final int min;
         final int max;
-        if (Booleans.parseBoolean(value, true) == false) {
+        if (Booleans.isExplicitFalse(value)) {
+            if (Booleans.isStrictlyBoolean(value) == false) {
+                DEPRECATION_LOGGER.deprecated("Expected [false] for setting [{}] but got [{}]", IndexMetaData.SETTING_AUTO_EXPAND_REPLICAS, value);
+            }
             return new AutoExpandReplicas(0, 0, false);
         }
         final int dash = value.indexOf('-');

core/src/main/java/org/elasticsearch/cluster/metadata/MappingMetaData.java

@@ -215,7 +215,7 @@ private void initMappers(Map<String, Object> withoutType) {
                 String fieldName = entry.getKey();
                 Object fieldNode = entry.getValue();
                 if (fieldName.equals("required")) {
-                    required = lenientNodeBooleanValue(fieldNode);
+                    required = lenientNodeBooleanValue(fieldNode, fieldName);
                 }
             }
             this.routing = new Routing(required);
@@ -232,13 +232,13 @@ private void initMappers(Map<String, Object> withoutType) {
                 String fieldName = entry.getKey();
                 Object fieldNode = entry.getValue();
                 if (fieldName.equals("enabled")) {
-                    enabled = lenientNodeBooleanValue(fieldNode);
+                    enabled = lenientNodeBooleanValue(fieldNode, fieldName);
                 } else if (fieldName.equals("format")) {
                     format = fieldNode.toString();
                 } else if (fieldName.equals("default") && fieldNode != null) {
                     defaultTimestamp = fieldNode.toString();
                 } else if (fieldName.equals("ignore_missing")) {
-                    ignoreMissing = lenientNodeBooleanValue(fieldNode);
+                    ignoreMissing = lenientNodeBooleanValue(fieldNode, fieldName);
                 }
             }
             this.timestamp = new Timestamp(enabled, format, defaultTimestamp, ignoreMissing);

core/src/main/java/org/elasticsearch/common/Booleans.java

@@ -24,30 +24,6 @@
  */
 public class Booleans {
 
-    /**
-     * Returns <code>false</code> if text is in <tt>false</tt>, <tt>0</tt>, <tt>off</tt>, <tt>no</tt>; else, true
-     */
-    public static boolean parseBoolean(char[] text, int offset, int length, boolean defaultValue) {
-        // TODO: the leniency here is very dangerous: a simple typo will be misinterpreted and the user won't know.
-        // We should remove it and cutover to https://github.com/rmuir/booleanparser
-        if (text == null || length == 0) {
-            return defaultValue;
-        }
-        if (length == 1) {
-            return text[offset] != '0';
-        }
-        if (length == 2) {
-            return !(text[offset] == 'n' && text[offset + 1] == 'o');
-        }
-        if (length == 3) {
-            return !(text[offset] == 'o' && text[offset + 1] == 'f' && text[offset + 2] == 'f');
-        }
-        if (length == 5) {
-            return !(text[offset] == 'f' && text[offset + 1] == 'a' && text[offset + 2] == 'l' && text[offset + 3] == 's' && text[offset + 4] == 'e');
-        }
-        return true;
-    }
-
     /**
      * returns true if the a sequence of chars is one of "true","false","on","off","yes","no","0","1"
      *
@@ -78,6 +54,13 @@ public static boolean isBoolean(char[] text, int offset, int length) {
         return false;
     }
 
+    public static Boolean parseBooleanExact(String value, Boolean defaultValue) {
+        if (Strings.hasText(value)) {
+            return parseBooleanExact(value);
+        }
+        return defaultValue;
+    }
+
     /***
      *
      * @return true/false
@@ -96,6 +79,13 @@ public static Boolean parseBooleanExact(String value) {
         throw new IllegalArgumentException("Failed to parse value [" + value + "] cannot be parsed to boolean [ true/1/on/yes OR false/0/off/no ]");
     }
 
+    /**
+     * @return true iff the provided value is either "true" or "false".
+     */
+    public static boolean isStrictlyBoolean(String value) {
+        return "false".equals(value) || "true".equals(value);
+    }
+
     public static Boolean parseBoolean(String value, Boolean defaultValue) {
         if (value == null) { // only for the null case we do that here!
             return defaultValue;

core/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -42,7 +42,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
@@ -668,15 +667,25 @@ public static Setting<Integer> intSetting(String key, int defaultValue, Property
     }
 
     public static Setting<Boolean> boolSetting(String key, boolean defaultValue, Property... properties) {
-        return new Setting<>(key, (s) -> Boolean.toString(defaultValue), Booleans::parseBooleanExact, properties);
+        return new Setting<>(key, (s) -> Boolean.toString(defaultValue), (value) -> parseBoolean(key, value), properties);
     }
 
     public static Setting<Boolean> boolSetting(String key, Setting<Boolean> fallbackSetting, Property... properties) {
-        return new Setting<>(key, fallbackSetting, Booleans::parseBooleanExact, properties);
+        return new Setting<>(key, fallbackSetting, (value) -> parseBoolean(key, value), properties);
     }
 
     public static Setting<Boolean> boolSetting(String key, Function<Settings, String> defaultValueFn, Property... properties) {
-        return new Setting<>(key, defaultValueFn, Booleans::parseBooleanExact, properties);
+        return new Setting<>(key, defaultValueFn, (value) -> parseBoolean(key, value), properties);
+    }
+
+    private static Boolean parseBoolean(String key, String value) {
+        // let the parser handle all cases for non-proper booleans without a deprecation warning by throwing IAE
+        boolean booleanValue = Booleans.parseBooleanExact(value);
+        if (Booleans.isStrictlyBoolean(value) == false) {
+            DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(Setting.class));
+            deprecationLogger.deprecated("Expected a boolean [true/false] for setting [{}] but got [{}]", key, value);
+        }
+        return booleanValue;
     }
 
     public static Setting<ByteSizeValue> byteSizeSetting(String key, ByteSizeValue value, Property... properties) {

core/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -26,6 +26,8 @@
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.loader.SettingsLoader;
 import org.elasticsearch.common.settings.loader.SettingsLoaderFactory;
 import org.elasticsearch.common.unit.ByteSizeUnit;
@@ -74,7 +76,6 @@
  * An immutable settings implementation.
  */
 public final class Settings implements ToXContent {
-
     public static final Settings EMPTY = new Builder().build();
     private static final Pattern ARRAY_PATTERN = Pattern.compile("(.*)\\.\\d+$");
 
@@ -310,7 +311,13 @@ public Long getAsLong(String setting, Long defaultValue) {
      * returns the default value provided.
      */
     public Boolean getAsBoolean(String setting, Boolean defaultValue) {
-        return Booleans.parseBoolean(get(setting), defaultValue);
+        String rawValue = get(setting);
+        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 [true/false] for setting [{}] but got [{}]", setting, rawValue);
+        }
+        return booleanValue;
     }
 
     /**

core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java

@@ -20,6 +20,8 @@
 package org.elasticsearch.common.xcontent;
 
 import org.elasticsearch.common.Booleans;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 
 import java.io.IOException;
 import java.util.Map;
@@ -30,7 +32,6 @@
  * but those that don't may or may not require emitting a startObject and an endObject.
  */
 public interface ToXContent {
-
     interface Params {
         String param(String key);
 
@@ -65,6 +66,7 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) {
     };
 
     class MapParams implements Params {
+        private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(MapParams.class));
 
         private final Map<String, String> params;
 
@@ -88,12 +90,16 @@ public String param(String key, String defaultValue) {
 
         @Override
         public boolean paramAsBoolean(String key, boolean defaultValue) {
-            return Booleans.parseBoolean(param(key), defaultValue);
+            return paramAsBoolean(key, (Boolean) defaultValue);
         }
 
         @Override
         public Boolean paramAsBoolean(String key, Boolean defaultValue) {
-            return Booleans.parseBoolean(param(key), defaultValue);
+            String rawParam = param(key);
+            if (rawParam != null && Booleans.isStrictlyBoolean(rawParam) == false) {
+                DEPRECATION_LOGGER.deprecated("Expected a boolean [true/false] for [{}] but got [{}]", key, rawParam);
+            }
+            return Booleans.parseBoolean(rawParam, defaultValue);
         }
     }