ArrayUtils.shuffle() throws NullPointerException for null array input

[rajucomp]

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven 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.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• 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.

Description

The shuffle() method in ArrayUtils throws a NullPointerException when a null array is passed as input.

This behavior is currently undocumented and inconsistent with other utility methods in ArrayUtils, which typically tolerate null inputs.

Suggested behavior:

• Explicitly document that shuffle() does not accept null arrays, or
• Add a null check and safely return without throwing an exception when the input array is null.

Failing Test Case

@Test
void testShuffleNull() {
    assertDoesNotThrow(() -> ArrayUtils.shuffle((byte[]) null));
}

[garydgregory]

@rajucomp
Please see my comment.

[garydgregory]

Don't use assertDoesNotThrow(), as it clutters a one-line method.
You can add to testShuffleByte instead.

[rajucomp]

That was just one case. The full test case would test for all array types for null input. I have updated the test to cover all the cases.

[garydgregory]

Hello @rajucomp
See my previous comment; we already have one method per parameter type. Update each method instead of creating a new test method. This lets us test each method in one place. TY.

[rajucomp]

@garydgregory Done.

[rajucomp]

@rajucomp Please see my comment.

Added a comment.

[garydgregory]

Hello @rajucomp
Thank you for your update. Since there are still some missing use cases in the tests here and this PR only doesn't offer a matching change on the main side, I've applied this PR locally, and updated the main code to account for null inputs on the array and the Random parameters. You are credited in changes.xml. I think this PR can be closed unless you think there are other shuffle use cases to handle, then you can rebase on git master and continue. TY!

[rajucomp]

Hi @garydgregory . Thanks for the fix.

I would like some guidance on how to raise PR's for issues and fixes.

1. Looking at the comments on the various issues, I deduced that in order to fix an issue, first a PR with a failing test should be raised to confirm the issue.
2. Once the maintainers i.e. you acknowledge the bug, then we push the fix on the same PR.

I am not sure if I am following the right pattern here. My idea was to first demonstrate the issue and then push the fix for that. This makes it easier for me to track my contributions. Could you please provide some tips here as how to I can better do this and save our times and the back and forth as well?

Additionally, it would be better if we provide the chance to review the fix before the changes are merged into the master branch. An additional set of eyes don't hurt and makes the fix error-free.

There is a small issue that I found in your fix. I have commited the fix in this PR.

Could you review the changes and let me know if all looks good ? Thanks!

[garydgregory]

Hello @rajucomp

I'll get back to this in a little while after updating the Commons Parent POM, which should address the current SPDX failure.

What can be confusing is that Jira tickets are reports from anyone and everyone, and that there might not be consensus on a path forward for any given issue. IOW, just because someone created a Jira ticket means that it should be fixed automatically.

In general, creating a PR with only a failing test should be the exception, not the rule. If someone reports an issue in Jira or the mailing list, we might ask for a POC in the form of an example, a failing test, something that demonstrates the issue.

The usual path is to create a PR that tests and fixes a single issue. The test should fail if the main side of the change is not applied.

If there is any doubt, you can seek advice in the Jira ticket or on the mailing list.

HTH