Add Boolean#parseBoolean to forbidden-apis

[kvanerum]

Resolves #59190

Added Boolean#parseBoolean to the list of forbidden-apis.
Resolved the violations against this in tests.
Marked other usages to review in #128993.

[cla-checker-service]

💚 CLA has been signed

[mosche]

Thanks so much for your contribution @kvanerum, this is very much appreciated! I have a couple of small comments.
Could I also ask you to sign the Contributor Agreement, pls?

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[kvanerum]

@mosche Thanks for your comments — I’ve reviewed them and added Boolean.valueOf(String s) to the forbidden APIs list.

[mosche]

@elasticmachine test this please

[mosche]

@kvanerum thanks for addressing my comments. This is looking really good, though I have just a few more ... I also triggered tests, let's see where we're at.

[mosche]

@elasticmachine test this please

[mosche]

Ok, just one more test failure that needs to be addressed, looking good otherwise 🎉

[mosche]

@elasticmachine test this please

[mosche]

@elasticmachine update branch

[mosche]

@elasticmachine test this please

[mosche]

lgtm, thanks a lot @kvanerum for pushing this through 🎉

[rjernst]

Rather than wrapping "intentional" lenient boolean parsing with SuppressForbidden, can we use Booleans.parseBooleanLenient?

[mosche]

@rjernst if you compare the logic of the two, there's significant differences. That would be a breaking change unfortunately.

[rjernst]

parseBooleanLenient is no longer used, it was deprecated long ago, and all non-test uses have been removed. I think we could change the implementation to match that of Boolean.parseBoolean.

[mosche]

parseBooleanLenient is no longer used, it was deprecated long ago, and all non-test uses have been removed. I think we could change the implementation to match that of Boolean.parseBoolean.

@rjernst That would of course work. Though note, there's just 4 usages of "permanent" lenient usage (without TODO re #128993). All other places require a deprecation warning in a next step.
I'd be reluctant to change those, imho the way it is now provides a much stronger signal on remaining open work. But you're right, it'd be good to not require @SuppressForbidden in those 4 other places. @kvanerum would you be ok making that change?

[mosche]

Sorry @kvanerum if I wasn't clear enough. This also requires changing the behavior of the old obsolete Booleans.parseBooleanLenient to match Boolean.parseBoolean

Subject: [PATCH] Change Booleans.parseBooleanLenient to wrap Boolean.parseBoolean
---
Index: libs/core/src/main/java/org/elasticsearch/core/Booleans.java
===================================================================
diff --git a/libs/core/src/main/java/org/elasticsearch/core/Booleans.java b/libs/core/src/main/java/org/elasticsearch/core/Booleans.java
--- a/libs/core/src/main/java/org/elasticsearch/core/Booleans.java	(revision 834b2d088d9707800392825366fb9ae0e8f231c3)
+++ b/libs/core/src/main/java/org/elasticsearch/core/Booleans.java	(date 1751618211876)
@@ -97,32 +97,16 @@
     }
 
     /**
-     * Returns {@code false} if text is in "false", "0", "off", "no"; else, {@code true}.
-     *
-     * @deprecated Only kept to provide automatic upgrades for pre 6.0 indices. Use {@link #parseBoolean(String, Boolean)} instead.
-     */
-    @Deprecated
-    public static Boolean parseBooleanLenient(String value, Boolean defaultValue) {
-        if (value == null) { // only for the null case we do that here!
-            return defaultValue;
-        }
-        return parseBooleanLenient(value, false);
-    }
-
-    /**
-     * Returns {@code false} if text is in "false", "0", "off", "no"; else, {@code true}.
+     * Wrapper around Boolean.parseBoolean for lenient parsing of booleans.
      *
-     * @deprecated Only kept to provide automatic upgrades for pre 6.0 indices. Use {@link #parseBoolean(String, boolean)} instead.
+     * Note: Lenient parsing is highly discouraged and should only be used if absolutely necessary.
      */
-    @Deprecated
+    @SuppressForbidden(reason = "allow lenient parsing of booleans")
     public static boolean parseBooleanLenient(String value, boolean defaultValue) {
         if (value == null) {
             return defaultValue;
         }
-        return switch (value) {
-            case "false", "0", "off", "no" -> false;
-            default -> true;
-        };
+        return Boolean.parseBoolean(value);
     }
 
     /**
@@ -138,71 +122,4 @@
     public static boolean isTrue(String value) {
         return "true".equals(value);
     }
-
-    /**
-     * Returns {@code false} if text is in "false", "0", "off", "no"; else, {@code true}.
-     *
-     * @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) {
-        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') == false;
-        }
-        if (length == 3) {
-            return (text[offset] == 'o' && text[offset + 1] == 'f' && text[offset + 2] == 'f') == false;
-        }
-        if (length == 5) {
-            return (text[offset] == 'f'
-                && text[offset + 1] == 'a'
-                && text[offset + 2] == 'l'
-                && text[offset + 3] == 's'
-                && text[offset + 4] == 'e') == false;
-        }
-        return true;
-    }
-
-    /**
-     * returns true if the a sequence of chars is one of "true","false","on","off","yes","no","0","1"
-     *
-     * @param text   sequence to check
-     * @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.
-     */
-    @Deprecated
-    public static boolean isBooleanLenient(char[] text, int offset, int length) {
-        if (text == null || length == 0) {
-            return false;
-        }
-        if (length == 1) {
-            return text[offset] == '0' || text[offset] == '1';
-        }
-        if (length == 2) {
-            return (text[offset] == 'n' && text[offset + 1] == 'o') || (text[offset] == 'o' && text[offset + 1] == 'n');
-        }
-        if (length == 3) {
-            return (text[offset] == 'o' && text[offset + 1] == 'f' && text[offset + 2] == 'f')
-                || (text[offset] == 'y' && text[offset + 1] == 'e' && text[offset + 2] == 's');
-        }
-        if (length == 4) {
-            return (text[offset] == 't' && text[offset + 1] == 'r' && text[offset + 2] == 'u' && text[offset + 3] == 'e');
-        }
-        if (length == 5) {
-            return (text[offset] == 'f'
-                && text[offset + 1] == 'a'
-                && text[offset + 2] == 'l'
-                && text[offset + 3] == 's'
-                && text[offset + 4] == 'e');
-        }
-        return false;
-    }
-
 }
Index: server/src/test/java/org/elasticsearch/common/BooleansTests.java
===================================================================
diff --git a/server/src/test/java/org/elasticsearch/common/BooleansTests.java b/server/src/test/java/org/elasticsearch/common/BooleansTests.java
--- a/server/src/test/java/org/elasticsearch/common/BooleansTests.java	(revision 834b2d088d9707800392825366fb9ae0e8f231c3)
+++ b/server/src/test/java/org/elasticsearch/common/BooleansTests.java	(date 1751618232226)
@@ -12,10 +12,7 @@
 import org.elasticsearch.core.Booleans;
 import org.elasticsearch.test.ESTestCase;
 
-import java.util.Locale;
-
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.nullValue;
 
 public class BooleansTests extends ESTestCase {
     private static final String[] NON_BOOLEANS = new String[] {
@@ -87,56 +84,10 @@
         }
     }
 
-    public void testIsBooleanLenient() {
-        String[] booleans = new String[] { "true", "false", "on", "off", "yes", "no", "0", "1" };
-        String[] notBooleans = new String[] { "11", "00", "sdfsdfsf", "F", "T" };
-        assertThat(Booleans.isBooleanLenient(null, 0, 1), is(false));
-
-        for (String b : booleans) {
-            String t = "prefix" + b + "suffix";
-            assertTrue(
-                "failed to recognize [" + b + "] as boolean",
-                Booleans.isBooleanLenient(t.toCharArray(), "prefix".length(), b.length())
-            );
-        }
-
-        for (String nb : notBooleans) {
-            String t = "prefix" + nb + "suffix";
-            assertFalse("recognized [" + nb + "] as boolean", Booleans.isBooleanLenient(t.toCharArray(), "prefix".length(), nb.length()));
-        }
-    }
-
     public void testParseBooleanLenient() {
-        assertThat(Booleans.parseBooleanLenient(randomFrom("true", "on", "yes", "1"), randomBoolean()), is(true));
-        assertThat(Booleans.parseBooleanLenient(randomFrom("false", "off", "no", "0"), randomBoolean()), is(false));
-        assertThat(Booleans.parseBooleanLenient(randomFrom("true", "on", "yes").toUpperCase(Locale.ROOT), randomBoolean()), is(true));
+        assertThat(Booleans.parseBooleanLenient(randomFrom("true", "TRUE", "True"), randomBoolean()), is(true));
+        assertThat(Booleans.parseBooleanLenient(randomFrom("false", "FALSE", "anything"), randomBoolean()), is(false));
         assertThat(Booleans.parseBooleanLenient(null, false), is(false));
         assertThat(Booleans.parseBooleanLenient(null, true), is(true));
-
-        assertThat(
-            Booleans.parseBooleanLenient(randomFrom("true", "on", "yes", "1"), randomFrom(Boolean.TRUE, Boolean.FALSE, null)),
-            is(true)
-        );
-        assertThat(
-            Booleans.parseBooleanLenient(randomFrom("false", "off", "no", "0"), randomFrom(Boolean.TRUE, Boolean.FALSE, null)),
-            is(false)
-        );
-        assertThat(
-            Booleans.parseBooleanLenient(
-                randomFrom("true", "on", "yes").toUpperCase(Locale.ROOT),
-                randomFrom(Boolean.TRUE, Boolean.FALSE, null)
-            ),
-            is(true)
-        );
-        assertThat(Booleans.parseBooleanLenient(null, Boolean.FALSE), is(false));
-        assertThat(Booleans.parseBooleanLenient(null, Boolean.TRUE), is(true));
-        assertThat(Booleans.parseBooleanLenient(null, null), nullValue());
-
-        char[] chars = randomFrom("true", "on", "yes", "1").toCharArray();
-        assertThat(Booleans.parseBooleanLenient(chars, 0, chars.length, randomBoolean()), is(true));
-        chars = randomFrom("false", "off", "no", "0").toCharArray();
-        assertThat(Booleans.parseBooleanLenient(chars, 0, chars.length, randomBoolean()), is(false));
-        chars = randomFrom("true", "on", "yes").toUpperCase(Locale.ROOT).toCharArray();
-        assertThat(Booleans.parseBooleanLenient(chars, 0, chars.length, randomBoolean()), is(true));
     }
 }

[kvanerum]

@mosche Apologies, I misunderstood. I committed the change as you proposed.

[mosche]

@elasticmachine test this please

[mosche]

@elasticmachine update branch

[elasticmachine]

merge conflict between base and head

[mosche]

@elasticmachine test this please

[mosche]

lgtm

[mosche]

Thanks so much for your efforts on this @kvanerum, that's very much appreciated!