Add Boolean#parseBoolean to forbidden-apis

[jaymode]

In #22200, existing boolean parsing was made strict so that:

"true" is converted to the boolean value true, "false" is converted to the boolean value false. Everything else raises an error.

However, usages of Boolean.parseBoolean have made their way into the codebase since then and the implementation of the JDKs boolean parsing logic is anything but strict as a string that is a case-insensitive match to true will be true and everything else will be false.

I think we should remove these usages of Boolean.parseBoolean after deprecation in the places where it was previously used and add this API to our forbidden-apis list.

[elasticmachine]

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

[rjernst]

It is unfortunate we let this lapse; a lot of work went into removing uses of lenient boolean parsing. It looks like most of the uses added are in tests, although there appear to be a couple that are user facing. +1 to adding the method to forbidden apis to stop the bleeding.

[elasticsearchmachine]

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

[esousacosta]

Hi,

I'm willing to contribute by working on this. Is that ok?

I started looking into the places we need the Boolean.parseBoolean replaced.

[mosche]

Thanks @esousacosta , your contribution would be much appreciated. Note, for backwards compatibility we'll likely want to deprecate the old behavior first, meaning every other value than true or false should log a deprecation warning rather than throwing an error. I'll discuss and confirm that with the team.

[esousacosta]

No problem, @mosche.

Thank you for letting me know about the deprecation warning. I'll be on lookout here for more news on this matter, then.

[mosche]

@esousacosta a decent first step here could be to:

• add Boolean#parseBoolean to the forbidden apis
• replace all usage in tests (and potentially other cases if there's no backwards compatibility concerns) with org.elasticsearch.core.Booleans#parseBoolean
• add @SuppressForbidden to remaining public facing usage (e.g. for settings or surfacing) with a TODO to add a deprecation warning for any usage of invalid values that would break once migrating to Booleans#parseBoolean + a link to this issue.

Deprecation warnings are a bit more involved, we could discuss these in a follow up. Wdyt?

[esousacosta]

Looks good to me as a plan, @mosche.

I'll start on that and update the issue when I have news 👍

[AayzStha37]

Hi @mosche @jaymode, I have started working on this issue, fixed the usage in the test cases and updated the forbidden API property file. However, can you please help me with how to identify the internal classes and classes that have public-facing usage to apply @mosche's suggestions?

[mosche]

Thanks a lot and sorry for the late reply @AayzStha37

You can run ./gradlew forBiddenApis or one of it's variants to identify all the violations. Identifying if these are internal or public unfortunately isn't trivial, we'd have to go case by case (e.g. on a PR).

The best way forward is probably to start with adding @SuppressForbidden for all remaining violations plus adding a comment pointing to https://github.com/elastic/elasticsearch/issues/128993 to follow up deprecating any lenient usage.

[kvanerum]

Hi @mosche @jaymode ,

I noticed there are usages of Boolean.valueOf(String s) as well. Might be a good idea to also add these to the forbidden-apis?

Kind regards, Kris

[mosche]

Sorry, just back from some time off @kvanerum. Good point regarding Boolean.valueOf(String s), absolutely agree 👍 Thanks for raising above PR, I'll have a look this morning.