-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Deprecation message for lenient Boolean parsing #137885
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
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @mamazzol, I've created a changelog YAML for you. Note that since this PR is labelled |
|
Hi @mamazzol, I've updated the changelog YAML for you. |
mosche
left a comment
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 @mamazzol, a couple of points below.
That's a lot less places than I feared to see 😅 And I think some of these could actually be lenient for good.
server/src/main/java/org/elasticsearch/common/util/Booleans.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/Booleans.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/Booleans.java
Outdated
Show resolved
Hide resolved
modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java
Outdated
Show resolved
Hide resolved
modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/NettyAllocator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/Booleans.java
Outdated
Show resolved
Hide resolved
|
Hi @mamazzol, I've updated the changelog YAML for you. Note that since this PR is labelled |
jdconrad
left a comment
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 couple comments on the scripting portion.
modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/store/LuceneFilesExtensions.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/analysis/wrappers/SettingsInvocationHandler.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/LenientBooleans.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/analysis/wrappers/SettingsInvocationHandler.java
Outdated
Show resolved
Hide resolved
| @SuppressForbidden( | ||
| reason = "TODO Deprecate any lenient usage of Boolean#parseBoolean https://github.com/elastic/elasticsearch/issues/128993" | ||
| ) | ||
| private static boolean allowUnknownLuceneFileExtensions() { |
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.
@mamazzol I noticed this is only used in an assertion (and disabled in production), so we can make this one strict as well
| public static boolean parseAndCheckForDeprecatedUsage(String value, Category category, String name) { | ||
| if (Booleans.isBoolean(value) == false) { | ||
| String key = String.format(Locale.ROOT, "lenient.%s.%s", category, name); | ||
| deprecationLogger.warn(DeprecationCategory.PARSING, key, DEPRECATED_MESSAGE_TEMPLATE, value, category.displayValue(), name); |
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.
Two more points, level has to be critical (our default for deprecations) and we should also put the deprecation category on the signature. For SettingsInvocationHandler the category should be SETTINGS.
mosche
left a comment
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 @mamazzol , lgtm.
As soon as the breaking change is approved, we can merge.
| * Category of use of lenient Boolean parsing. | ||
| */ | ||
| public enum Category { | ||
| public enum UsageCategory { |
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.
just throwing it out as an option, looking at the latest change DeprecationCategory could also be a field of UsageCategory
Wrapping user-facing usages of Boolean.parseBoolean in a util method to send a deprecation log.
#128993