ppkarwasz commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2253733612
##########
src/main/java/org/apache/commons/beanutils2/MethodUtils.java:
##########
@@ -1127,7 +1126,7 @@ private static void setMethodAccessible(final Method
method) {
if (!loggedAccessibleWarning) {
boolean vulnerableJVM = false;
try {
- final String specVersion =
SystemProperties.getJavaSpecificationVersion();
+ final String specVersion =
System.getProperty("java.specification.version");
Review Comment:
The `try…catch` is dead code: Commons BeanUtils has a baseline of Java 8, so
the Java version will never be lower than 4.
##########
src/main/java/org/apache/commons/beanutils2/FluentPropertyBeanIntrospector.java:
##########
@@ -159,6 +159,6 @@ public void introspect(final IntrospectionContext icontext)
throws Introspection
*/
private String propertyName(final Method m) {
final String methodName =
m.getName().substring(getWriteMethodPrefix().length());
- return methodName.length() > 1 ? Introspector.decapitalize(methodName)
: StringUtils.toRootLowerCase(methodName);
+ return methodName.length() > 1 ? Introspector.decapitalize(methodName)
: methodName.toLowerCase(Locale.ROOT);
Review Comment:
```suggestion
return Introspector.decapitalize(methodName);
```
`Introspector.decapitalize()` actually also handles empty or single
character names.
##########
src/main/java/org/apache/commons/beanutils2/converters/AbstractConverter.java:
##########
@@ -66,7 +66,7 @@ public abstract class AbstractConverter<D> implements
Converter<D> {
* @return the given string trimmed and converter to lower-case.
*/
protected static String toLowerCase(final Object value) {
- return StringUtils.toRootLowerCase(toString(value));
+ return toString(value).toLowerCase(Locale.ROOT);
Review Comment:
In Commons BeanUtils, `AbstractConverter.toLowerCase()` seems to be used as
a performance optimization: it lowercases the input string once, allowing
comparisons to use `String.equals()` instead of `String.equalsIgnoreCase()`,
which involves more complex case folding.
For example, in `BooleanConverter`, this lowercasing is used to perform
case-insensitive comparisons between the input and user-defined lists of `true`
values (e.g., *Evet*, *Doğru* in Turkish) and `false` values (e.g., *Hayır*,
*Yanlış*). When using the default constructor, the converter compares against
English words like `true`, `yes`, `false`, and `no`, so using `Locale.ROOT` is
appropriate.
However, this may not be sufficient for converters initialized with
locale-specific values. For instance, the Turkish word `HAYIR` ("no") would be
incorrectly lowercased to `hayir` rather than the correct `hayır` when using
`Locale.ROOT`.
Would it make sense to make the `Locale` configurable in `AbstractConverter`
(perhaps as a JavaBeans property), with a default of `Locale.ROOT`? This would
preserve the current behavior while allowing users to specify a locale when
needed.
**Note**: The case folding algorithm defined in the [Unicode
standard](https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G53253)
and used by `String.equalsIgnoreCase()` does not uniformly lowercase all
characters (e.g., Cherokee characters are uppercased). If the performance
impact is negligible, it might be worth reconsidering whether
`BooleanConverter` should use `equalsIgnoreCase()` directly for correctness
across locales.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
