Regression: Unknown options do not throw an exception (since 1.61)

[elusiveeagle]

trafficstars

Howdy!

First off, thanks for the ongoing support of JCommander! It's very handy to have this library.

I believe I've identified a regression defect in JCommander 1.71 (latest release), so here I am reporting it! If I am mistaken, then please correct me and off I'll go!

I work on a project in which we recently upgraded our dependency on JCommander from 1.48 to 1.71. Prior to committing the library change, however, we wanted to verify manually and with automated tests that our solution would still behave the same. Upon running our automated tests, we noticed that several of the tests responsible for verifying that unknown options specified on the command-line threw exceptions now fail. After further investigation and deep-diving here on GitHub, it appears that this behavior is due to a regression defect introduced in 1.61 when the need for option prefixes was removed. Furthermore, it looks like this got through JCommander's tests because of some false positive tests.

My understanding of JCommander's behavior is that which can be seen in version 1.48, namely: When acceptUnknownOptions is false (default), JCommander should throw an exception when an unknown option is encountered (regardless of the presence of a main parameter).

I have not seen any JCommander documentation that states that version 1.71 (or even 1.61+ for that matter) now treats unknown options in a new way as my tests now show. If I missed some documentation or if this new behavior for unknown options is by design, please let me know.

Now, however, JCommander does things differently:

1. If an unknown option is encountered when there is no main parameter defined, then JCommander no longer throws a ParameterException with message Unknown option and instead throws a ParameterException with message Was passed main parameter '<unknown_option>' but no main parameter was defined in your arg class.
2. If an unknown option is encountered when there is a main parameter defined, then JCommander 'eats' the unknown option as if it were accepted, even if acceptUnknownOptions is false (default).

Here are several tests I wrote and used that show the issues above: https://gist.github.com/elusiveeagle/075c6f638db217d6c66745b125cb4ca7. Execute the tests with both JCommander 1.48 and 1.71 and you can see the differences.

For some historical reference, you can see the following issues and commits that implemented the original unknown option behavior I have seen in version 1.48:

• Version 1.20
  • Changelog note: Fixed: Throw if an unknown option is found
  • Issue: https://github.com/cbeust/jcommander/issues/85
  • Commit: https://github.com/cbeust/jcommander/commit/4ba2a3c6a66626c4629266729a16e7f7e07b9ad0
• Version 1.27
  • Changelog note: Fixed: if using a different option prefix, unknown option are mistakenly reported as "no main parameter defined" (kurmasz)
  • Issue: https://github.com/cbeust/jcommander/issues/115
  • Commit: https://github.com/cbeust/jcommander/commit/9e22cffb2d22829cb5e2d1bf4d64d87df69d2983

Then in version 1.61, the need to use option prefixes was removed, which appears to have resulted in this regression defect. Method getOptionPrefixes() was removed and isOption() was significantly refactored.

• Version 1.61
  • Issue: https://github.com/cbeust/jcommander/issues/321
  • Commit: https://github.com/cbeust/jcommander/commit/113ba56cd5ced2168926400ee57bcc61b0d2117d

Additionally I wanted to be sure I brought attention to a test in JCommanderTest.java that appears to generate a false positive: https://github.com/cbeust/jcommander/blob/master/src/test/java/com/beust/jcommander/JCommanderTest.java#L850. Both versions 1.48 and 1.71 throw a ParameterException, but the message is very different. The exception message in 1.48 states Unknown option: -lon, whereas the exception in 1.71 states Was passed main parameter '-lon' but no main parameter was defined in your arg class. Had this test, which was added as part of Issue #85, correctly used TestNG's expectedExceptionMessageRegExp attribute of the Test annotation to specifically pass when the message stated Unknown option, then this would have been caught when the change in 1.61 was introduced.

Hope that helps! Please let me know if you have any questions. Thanks!

[milleruntime]

I was attempting to upgrade to 1.72 (closest to 1.71 that is in maven central) and had a similar issue. I tested multiple versions and narrowed it down to working fine in 1.60 and failing against one of our tests in 1.64 (the next version in maven central). I am seeing the ParameterException thrown in 1.60 but then not thrown in 1.64 when encountering an Unknown option.

We have the Main Parameter called unrecognizedOptions as a fail safe catch all. At setup, we check if unrecognizedOptions contains anything and if it does we log an Error. So this one possible work around for the change in behavior. It would be nice to know if this is a bug and if not, the reasoning behind the change.

[milleruntime]

I looked into the code that is discussed above and it would seem that it is behaving as intended. The side effect of these changes is that a ParameterException will no longer be thrown, ever, I believe since it is now unreachable code: https://github.com/cbeust/jcommander/blob/master/src/main/java/com/beust/jcommander/JCommander.java#L737 This is because if an unknown option is encountered and acceptUnknownOptions = false it will fail the isOption() check and get counted as a Main parameter. If acceptUnknownOptions = true, it will pass the isOption() check but get added to m_unknownArgs.

Perhaps the documentation should define the intended behavior of setting acceptUnknownOptions since it seems to have changed.

[rakus]

I hit the same problem when I wanted to update a old project from JCommander 1.30 to 1.78.

If it is intended that unknown options don't throw a ParameterException, then the documentation should be updated to explain how unknown options should be detected and handled.

As noted in the original description, the test method com.beust.jcommander.JCommanderTest.shouldThrowIfUnknownOption() should be reviewed.

[LucCappellaro]

According to Usage (DefaultUsageFormatter) there seems to be an order, first the options then the main parameters. This is common: "All options precede non-option arguments" (getopt).

This could help to distinguish options from non-options and raise exceptions again. And maybe introduce an explicit separator: "-- always marks the end of options" (getopt).