Remove dependency to commons-collections4 (fixes #868)

[reschke]

No description provided.

[xeno6696]

As per comments, this is a clean cut.

[xeno6696]

@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.

[kwwall]

@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.

[jeremiahjstacey]

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.

[jeremiahjstacey]

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

[reschke]

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.

[reschke]

ping?

[xeno6696]

@reschke we need two approvers, we're just waiting on Kevin/Jeremiah.

Even if we merged it today, the release isn't going out until April at least. If you need it now for your own work, compile and use your own, don't wait on us.

[reschke]

@xeno6696 - thanks; I just wanted to make sure that it has chances to get into the next release.

[kwwall]

Change approved, but see my comment. Would strongly like to see a new test case like I alluded to.
@jeremiahjstacey - Do you have any thoughts here?

@reschke - As @xeno6696 mentioned, it probably won't be until April/May until the next ESAPI release. I generally try to do one every 2 months or so, and the last release was on 2024-11-25.

[kwwall]

@reschke - From the screen shot above, it looks like GH rules is blocking the merge because the commit wasn't signed. Can you address that? I prefer not to bypass the rules we put on the ;'develop' branch to improve security. Thanks.

[reschke]

@kwwall - that may take a bit of time, unless I can do that directly in Github.

That said, by all means please apply the diff under your git identity.