[
https://issues.apache.org/jira/browse/CLI-344?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18010688#comment-18010688
]
Gary D. Gregory edited comment on CLI-344 at 7/29/25 2:37 PM:
--------------------------------------------------------------
In git master and snapshot builds: Fail faster with a more precise
NullPointerException
NPE is the most precise exception and message
Nevertheless, we can make this a touch better using Java's
{{Objects.requireNonNull()}} to fail faster with a message specific to the
argument. The exception is still an NPE, the most precise for this case.
Done in git master and snapshot builds in
[https://repository.apache.org/content/repositories/snapshots/commons-cli/commons-cli/1.10.0-SNAPSHOT/]
was (Author: garydgregory):
In git master and snapshot builds: Fail faster with a more precise
NullPointerException
> Option.processValue() throws NullPointerException when passed null value with
> value separator configured
> --------------------------------------------------------------------------------------------------------
>
> Key: CLI-344
> URL: https://issues.apache.org/jira/browse/CLI-344
> Project: Commons CLI
> Issue Type: Bug
> Components: Options definition
> Affects Versions: 1.10.0
> Reporter: Ruiqi Dong
> Assignee: Gary D. Gregory
> Priority: Major
> Fix For: 1.10.0
>
> Attachments: Option.java, OptionTest.java
>
> Original Estimate: 0.5h
> Remaining Estimate: 0.5h
>
> The {{Option.processValue(String value)}} method does not properly handle
> null input values. When a null value is passed to this method and the Option
> has a value separator configured, it throws a {{NullPointerException}}
> instead of handling the null gracefully with proper validation.
>
> *Steps to Reproduce:*
> Option option = new Option("D", true, "Define property");
> option.setValueSeparator('=');
> option.processValue(null); // Throws NullPointerException
>
> *Expected Behavior:* The method should validate the input and throw an
> {{IllegalArgumentException}} with a clear error message when null is passed,
> consistent with how other methods in the library handle invalid input.
> *Actual Behavior:* A {{NullPointerException}} is thrown at line 839 in
> Option.java when the code attempts to call {{indexOf()}} on the null value:
> java.lang.NullPointerException
> at org.apache.commons.cli.Option.processValue(Option.java:839)
>
> *Root Cause Analysis:*
> The {{processValue()}} method assigns the input value directly to a local
> variable without null checking:
> void processValue(final String value) {
> if (argCount == UNINITIALIZED) {
> throw new IllegalArgumentException("NO_ARGS_ALLOWED");
> }
> String add = value; // No null check
> if (hasValueSeparator()) {
> final char sep = getValueSeparator();
> int index = add.indexOf(sep); // NPE occurs here when add is null
> // ...
> }
> }
>
> *Unit Test Cases:*
> @Test
> public void testProcessValueWithNullThrowsIllegalArgumentException() {
> Option option = new Option("D", true, "Define property");
> option.setValueSeparator('=');
>
> IllegalArgumentException exception = assertThrows(
> IllegalArgumentException.class,
> () -> option.processValue(null),
> "processValue(null) should throw IllegalArgumentException"
> );
>
> assertTrue(exception.getMessage().contains("null"),
> "Exception message should indicate null value issue");
> }
>
> *Test Results:*
> [*INFO*] Running org.apache.commons.cli.{*}OptionTest{*}{*}{*}
> [*ERROR*] *Tests* {*}run: 1{*}, *Failures: 1*, Errors: 0, Skipped: 0, Time
> elapsed: 0.001 s *<<< FAILURE!* -- in
> org.apache.commons.cli.{*}OptionTest{*}{*}{*}
> [*ERROR*]
> org.apache.commons.cli.OptionTest.testProcessValueWithNullThrowsIllegalArgumentException
> -- Time elapsed: 0.001 s <<< FAILURE!
> org.opentest4j.AssertionFailedError: processValue(null) should throw
> IllegalArgumentException ==> Unexpected exception type thrown, expected:
> <java.lang.IllegalArgumentException> but was: <java.lang.NullPointerException>
> at
> org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
> at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:67)
> at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:39)
> at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3153)
> at
> org.apache.commons.cli.OptionTest.testProcessValueWithNullThrowsIllegalArgumentException(OptionTest.java:32)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at java.util.ArrayList.forEach(ArrayList.java:1259)
> at java.util.ArrayList.forEach(ArrayList.java:1259)
> Caused by: java.lang.NullPointerException
> at org.apache.commons.cli.Option.processValue(Option.java:839)
> at
> org.apache.commons.cli.OptionTest.lambda$testProcessValueWithNullThrowsIllegalArgumentException$0(OptionTest.java:34)
> at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:53)
> ... 6 more
>
> *Suggested Fix:*
> Add null validation at the beginning of the method:
> void processValue(final String value) {
> if (value == null) {
> throw new IllegalArgumentException("The value must not be null");
> }
> if (argCount == UNINITIALIZED) {
> throw new IllegalArgumentException("NO_ARGS_ALLOWED");
> }
> // ... rest of the method remains unchanged
> }
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
