Skip to content

Remove dependency to commons-collections4 (fixes #868) #870

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Remove dependency to commons-collections4 (fixes #868) #870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,6 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-collections4</artifactId>
<version>4.5.0-M2</version>
</dependency>
<dependency>
<groupId>org.apache-extras.beanshell</groupId>
<artifactId>bsh</artifactId>
Expand Down
9 changes: 3 additions & 6 deletions src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
import java.util.Vector;
import java.util.Arrays;

import org.apache.commons.collections4.iterators.ArrayListIterator;

public class DelegatingACR extends BaseACR<DynaBeanACRParameter, Object[]> {
protected Method delegateMethod;
protected Object delegateInstance;
Expand Down Expand Up @@ -66,10 +64,9 @@ protected final Class[] getParameters(String[] parameterClassNames) {
if (parameterClassNames == null) {
return new Class[0];
}
Vector<Class> classes = new Vector<Class>();
Iterator<String> classNames = new ArrayListIterator(parameterClassNames);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwwall I have a very hard time imagining how this change would break anything in terms of backwards compatibility. And it eliminates a dependency that clearly was so tenuous that I have a hard time understanding why we kept it. I approve of this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xeno6696, @reschke - Matt, I agree with you this shouldn't really break anything unless there are some bizzaro edge cases in Apache Common Collection 4's ArrayListIterator class that treats thingslike null or empty parameters in the XML file in some unexpected way. (Backwards compatibility ideally should address failure cases in an identical manager, and not just the sunny day scenarios.) I've not looked at the ArrayListIterator source code, but it's probably a safe assumption that it doesn't do anything weird in such cases. However, it would be nice to take the opportunity of of this change and an add a JUnit test that would invoke DelegatedACR.getParameters such edge cases to test it to be sure. Maybe Julian would be so kind as to contribute such a test as well.

Copy link
Collaborator

@jeremiahjstacey jeremiahjstacey Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can agree with testing; however, I'd like to throw out that if we're not tied to a Vector collection type, then the java 8 streams API can both consolidate the code and possibly clarify the desired behavior in edge-case scenarios:

    /**
     * Convert an array of fully qualified class names into an array of Class objects
     * @param parameterClassNames
     * @return The Class objects found that match the specified class names provided.
     */
    protected final Class[] getParameters(String[] parameterClassNames) {
        if (parameterClassNames == null) {
            return new Class[0];
        }

        List<Class> classes = Arrays.stream(parameterClassNames)
        .filter(Objects::nonNull)
        .filter(el -> !el.toString().trim().isEmpty()).map(name -> getClass(name, "parameter"))
        .collect(Collectors.toList());
        
      
        return classes.toArray(new Class[classes.size()]);
    }
  • testNullParameter //Empty Array
  • testArrayWithNullMember //Empty Array
  • testArrayWithBlankMember //Empty Array
  • testArrayWithInvalidClassName // ClassNotFoundException?
  • testArrayWithMixedMembers //contains (Null, empty, valid) --> Returns array of 1 with valid class

I don't know what tests currently exist, but those are the ones that I might consider in this update. I'm not sure of the complexity of stubbing the return from the getClass call either.

Copy link
Collaborator

@jeremiahjstacey jeremiahjstacey Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apololgies, just looked at the getClass method, I believe on invalid classname in the array we would expect an IllegalArgumentException

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of the fix was to be minimal. I checked https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/iterators/ArrayListIterator.html and it does not mention that it does unusual things will null. Empty values are not special in arrays anyway.

If there is special processing on the the way from XML to here, it happens somewhere else.

while(classNames.hasNext()) {
classes.add(getClass(classNames.next(), "parameter"));
Vector<Class> classes = new Vector<>();
for (String className : parameterClassNames) {
classes.add(getClass(className, "parameter"));
}
return classes.toArray(new Class[classes.size()]);
}
Expand Down