You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "basil (via GitHub)" <gi...@apache.org> on 2025/08/01 16:45:19 UTC
[PR] Limit Commons Lang 3 to test scope [commons-beanutils]
basil opened a new pull request, #359:
URL: https://github.com/apache/commons-beanutils/pull/359
Commons BeanUtils 1 does not depend on Commons Lang 3, but Commons BeanUtils 2 currently does. This poses a challenge for us because, while we are fine with depending on Commons BeanUtils, we prefer to avoid a dependency on Commons Lang. This is due to our architecture, which [exposes dependencies to all third-party plugins](https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/).
Since Commons BeanUtils 2 is still in milestone status and has not yet been released, there is an opportunity to address this. This pull request modifies a few minor instances in `src/main` to use native Java Platform functionality instead of Commons Lang 3. The use of Commons Lang 3 in tests remains unchanged.
To test this change, I ran `mvn clean verify`. CC @ppkarwasz
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [ ] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [ ] I used AI to create any part of, or all of, this pull request.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [ ] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best-practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#issuecomment-3146280780
Hi @basil,
> This is due to our architecture, which [exposes dependencies to all third-party plugins](https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/).
Could you elaborate on where exactly this causes issues in your case?
The question of how many call sites justify adding a dependency (like `commons-lang3`) is a recurring and somewhat contentious topic within the Commons PMC. Personally, I agree that this particular dependency may not be necessary here, but ultimately it will require broader discussion and consensus within the team. cc/ @garydgregory
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2267630591
##########
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:
#364 takes this in a different direction, simplifying the code for improved readability and removing 33 lines of code, all without negatively impacting locale functionality (as verified by updated test coverage).
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2254063744
##########
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:
I've cleaned up this pre-Java 1.4 code now. This whole class can probably be replaced with Lang code anyway.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2254999846
##########
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:
Your changes are exactly what I had in mind, although I didn't notice at all the `toLowerCase` is a `static` method, which would cause a breaking change in the library.
Maybe we should leave this method as you originally wrote it, but deprecate it and introduce a new one?
For the ultimate goal, i.e. remove Commons Lang as dependency, I think we should wait for the resolution of the comment above about `MethodUtils`.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2254280591
##########
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:
I think it depends on our broader strategy: are we aiming to consolidate more functionality into Commons Lang, or keep the libraries modular with well-defined scopes?
It might be worth clearly defining the boundaries between the Commons components so users know where to look for a given feature. For example, requiring Commons BeanUtils just for `MethodUtils` feels about the same as requiring Commons Lang solely for `StringUtils`.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2255015098
##########
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:
> Your changes are exactly what I had in mind
Something still doesn't feel right to me, as the new public methods can only be called on an `AbstractConverter`, not a `Converter`. Maybe I need to move them to the interface?
> although I didn't notice at all the toLowerCase is a static method, which would cause a breaking change in the library.
Is that necessarily a problem given that this PR is targetting the `master` branch (2.0.0-M2), which is in milestone status? The entire library is in a new package, which is a breaking change from v1. Since the PR that moved to a new package wasn't rejected, it obviously must be OK to break compatibility in some cases. Is this one of them? For example, is it OK to make breaking changes, like renaming the package or this one, until the first 2.0.0 (non-milestone) version is released?
> Maybe we should leave this method as you originally wrote it, but deprecate it and introduce a new one?
I could do this if compatibility must be retained across milestone releases.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2269499909
##########
pom.xml:
##########
@@ -107,6 +107,7 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.18.0</version>
+ <scope>test</scope>
Review Comment:
-1: version 2 will depend on Lang for other runtime calls like MethodUtils.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#issuecomment-3146369286
Hello all,
I don't think we need to merge this PR as it is stated as a preference and doesn't fix a bug. I also really don't want to not set a precedent to inline calls from dependencies. I thought OSGi and JPMS were supposed to deal with these types of ideas...
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#issuecomment-3152957784
@garydgregory Any update on this PR?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2254930857
##########
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:
@ppkarwasz I attempted to do this in commit 79516554e24b846a0c9f5c5ae3479b361955e501. I'm not entirely sure what you meant by
> make the `Locale` configurable in `AbstractConverter` (perhaps as a JavaBeans property)
so please let me know if this is what you had in mind.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#issuecomment-3180299634
> -1
@garydgregory If you have vetoed this PR, and there is no clear action item that would cause you to withdraw the veto, then should the PR be closed?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2255052241
##########
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:
> Something still doesn't feel right to me, as the new public methods can only be called on an `AbstractConverter`, not a `Converter`. Maybe I need to move them to the interface?
I am not sure this should be in the interface. It is more of an edge case for `BooleanConverter` that users might use or not.
> Is that necessarily a problem given that this PR is targetting the `master` branch (2.0.0-M2), which is in milestone status?
You are of course right. I am looking for breaking changes as a force of habit. I am resolving this conversation, thanks!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#issuecomment-3146492951
> I don't think we need to merge this PR as it […] doesn't fix a bug.
@garydgregory The original code contains a bug due to an unnecessary `try-catch` block for a `SecurityException` in `SystemProperties#getJavaSpecificationVersion`. This method is [documented to return `null` in cases where a `SecurityException` might occur](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/SystemProperties.html#getJavaSpecificationVersion()), making the `try-catch` block redundant and potentially misleading, as it suggests a `SecurityException` could be thrown when it will not. This PR fixes the bug by replacing the call to `SystemProperties#getJavaSpecificationVersion` with `System.getProperty("java.specification.version")`, which can throw a `SecurityException` as documented. This ensures the `try-catch` block is necessary and correctly handles the potential exception.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Limit Commons Lang 3 to test scope [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil closed pull request #359: Limit Commons Lang 3 to test scope
URL: https://github.com/apache/commons-beanutils/pull/359
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org