[jira] [Resolved] (CLI-343) OptionFormatter.getBothOpt() lacks validation for Options without opt or longOpt

["Gary D. Gregory (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/CLI-343?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Gary D. Gregory resolved CLI-343.
---------------------------------
    Resolution: Not A Problem

> OptionFormatter.getBothOpt() lacks validation for Options without opt or longOpt
> --------------------------------------------------------------------------------
>
>                 Key: CLI-343
>                 URL: https://issues.apache.org/jira/browse/CLI-343
>             Project: Commons CLI
>          Issue Type: Bug
>    Affects Versions: Nightly Builds
>            Reporter: Ruiqi Dong
>            Priority: Minor
>         Attachments: OptionFormatter.java, OptionFormatterTest.java
>
>   Original Estimate: 0.5h
>  Remaining Estimate: 0.5h
>
> *Description:*
> The {{OptionFormatter.getBothOpt()}} method contains a comment stating "Option requires at least one of opt or longOpt", but the method itself does not validate this constraint. Instead, it relies entirely on the {{Option}} class validation.
>  
> *Current Behavior:* The {{getBothOpt()}} method would return an empty string if called with an Option that has neither opt nor longOpt. The method includes a comment acknowledging the constraint but doesn't enforce it. 
> *Expected Behavior:* {{OptionFormatter}} should validate the Option constraint independently, either in its constructor or in the {{getBothOpt()}} method
>  
> *Unit Test Cases:*
> public class OptionFormatterTest {
> @Test
> @DisplayName("getBothOpt() should throw exception when both opt and longOpt are null")
> publicvoidtestGetBothOptWithNullShortAndLongOptShouldThrowException(){
> // Create an Option with both opt and longOpt as null
> // This violates the constraint mentioned in the comment:
> // "Option requires at least one of opt or longOpt"
> Optionoption = Option.builder()
> .required(false)
> .build();
>  
> // This should throw an exception but doesn't in the current implementation
> assertThrows(IllegalArgumentException.class, () -> {
> OptionFormatterformatter = OptionFormatter.builder().build(option);
> formatter.getBothOpt();
> }, "OptionFormatter should throw exception when Option has neither opt nor longOpt");
> }
>  
> @Test
> @DisplayName("OptionFormatter construction should fail when Option has neither opt nor longOpt")
> publicvoidtestOptionFormatterConstructionWithInvalidOption(){
> // Create an Option without any opt or longOpt
> OptioninvalidOption = Option.builder()
> .desc("This option has no way to be specified")
> .build();
>  
> // The builder.build() method should validate and throw exception
> OptionFormatter.Builderbuilder = OptionFormatter.builder();
>  
> assertThrows(IllegalArgumentException.class, () -> {
> builder.build(invalidOption);
> }, "OptionFormatter.Builder.build() should reject Options without opt or longOpt");
> }
> }
>  
> *Test Results:*
> [*INFO*] Running org.apache.commons.cli.help.{*}OptionFormatterTest{*}{*}{*}
> [*ERROR*] *Tests* {*}run: 2{*}, Failures: 0, *Errors: 2*, Skipped: 0, Time elapsed: 0.001 s *<<< FAILURE!* -- in org.apache.commons.cli.help.{*}OptionFormatterTest{*}{*}{*}
> [*ERROR*] org.apache.commons.cli.help.OptionFormatterTest.testGetBothOptWithNullShortAndLongOptShouldThrowException -- Time elapsed: 0 s <<< ERROR!
> java.lang.IllegalArgumentException: Either opt or longOpt must be specified
>  at org.apache.commons.cli.Option$Builder.build(Option.java:134)
>  at org.apache.commons.cli.help.OptionFormatterTest.testGetBothOptWithNullShortAndLongOptShouldThrowException(OptionFormatterTest.java:36)
>  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)
>  
> *Suggested Fix:* 
> Add validation in the {{OptionFormatter}} constructor
> private OptionFormatter(final Option option, final Builder builder) {
>     if (Util.isEmpty(option.getOpt()) && Util.isEmpty(option.getLongOpt())) {
>         throw new IllegalArgumentException("Option must have at least one of opt or longOpt");
>     }
>     // ... rest of constructor
> }



--
This message was sent by Atlassian Jira
(v8.20.10#820010)