IExecutionExceptionHandler does not provided parsed ParseResult

[abelsromero]

• Issue description

Looking into how to handle exceptions (https://picocli.info/#_invalid_user_input) I'd like to add some behavior depending on an argument. But parseResult in the method handleException in IExecutionExceptionHandler does not contain the actual parsed input. Instead matchedArgs() and matchedOptions return empty and only originalArgs() returns something.

• Current approach

As a workaround, I am calling commandLine.getParseResult() which provides the actual input command parsed.

• Expected behavior

The expectation would be that the workaround is not necessary and the parameter from the method provides such information.

[remkop]

@abelsromero Thank you for raising this. I will take a look.

[remkop]

@abelsromero I am looking at this now. I think I see the cause of the issue. Would you be able to provide a failing test? That would be very helpful.

[abelsromero]

I'll see to get some time this week, just don't expect it until the weekend.

[remkop]

I added a test, but it succeeds even before I make changes to fix the issue. Re-reading your comments now I have some idea on how to improve the test, I will iterate when I have time to work on this again.

[remkop]

I seem unable to reproduce the issue...
Please see the updated test; this test passes.

[abelsromero]

Thanks for the patience and time!

I admit I am confused with your test You mention the test does not reproduce, but the code and running locally shows the values are empty. Maybe I am assuming something that this not true? Oddly however, still empty after running parse 🤔

Here's an example https://github.com/abelsromero/picocli-reproducer with further explanation. See the different in output for both the parseResult injected and the one obtained from commandLine.getParseResult() https://github.com/abelsromero/picocli-reproducer/blob/d17ce257032b78ef2f22d9ee0f3889786cb1fe21/cli/src/main/java/org/abelsromero/picocli/ExecutionExceptionHandler.java#L15-L16

[remkop]

Thanks for the patience and time!

I admit I am confused with your test You mention the test does not reproduce, but the code and running locally shows the values are empty. Maybe I am assuming something that this not true? Oddly however, still empty after running parse 🤔

I do not see empty values (and the test passes, proving that the list is not empty: assertFalse(parseResult.matchedArgs().isEmpty()).
I changed my test, adding printouts similar to your reproducer and getting this result for my test:

	ParseResult from commandLine.getParseResult(): picocli.CommandLine$ParseResult@b3d7190
		matchedArgs(): [field boolean picocli.Issue2228$TestCommand.x]
		matchedOptions(): [field boolean picocli.Issue2228$TestCommand.x]
	ParseResult from ExecutionExceptionHandler method arg: picocli.CommandLine$ParseResult@5fdba6f9
		matchedArgs(): [field boolean picocli.Issue2228$TestCommand.x]
		matchedOptions(): [field boolean picocli.Issue2228$TestCommand.x]

Are you getting different results?

Here's an example https://github.com/abelsromero/picocli-reproducer with further explanation. See the different in output for both the parseResult injected and the one obtained from commandLine.getParseResult() https://github.com/abelsromero/picocli-reproducer/blob/d17ce257032b78ef2f22d9ee0f3889786cb1fe21/cli/src/main/java/org/abelsromero/picocli/ExecutionExceptionHandler.java#L15-L16

@abelsromero I have not had a chance to check out and build your project yet. Can you post the input you provided and the output that you are seeing from your reproducer?

[remkop]

I have been able to run your reproducer project and I get this output:

C:\Users\remko\IdeaProjects\picocli-reproducer>gradlew build run --args="get --fail" 

> Task :cli:run
Getting...
[INFO] From ExecutionExceptionHandler ----
        Exception: java.lang.RuntimeException: Boom!
        CommandLine: picocli.CommandLine@4cb2c100
        ParseResult from method: picocli.CommandLine$ParseResult@4909b8da
                matchedArgs(): []
                matchedOptions(): []
        ParseResult from commandLine.getParseResult(): picocli.CommandLine$ParseResult@3a03464
                matchedArgs(): [field boolean org.abelsromero.picocli.SharedOptions.fail]
                matchedOptions(): [field boolean org.abelsromero.picocli.SharedOptions.fail]

I will investigate further.

[abelsromero]

I do not see empty values (and the test passes, proving that the list is not empty: assertFalse(parseResult.matchedArgs().isEmpty()).

My mistake, I misread the assertion seeing the empty at the end.

I will investigate further.

Thanks! I was doing some extra tests in my reproducer and I think the issue stems from the use of subcommands.
If I run the same but replacing ParentCommand with the code below and run ./build/cli/bin/cli -n=42, I see the parsedResult being informed.

@CommandLine.Command(
//        subcommands = {
//                ListCommand.class,
//                GetCommand.class
//        },
        description = "Test CLI")
public class ParentCommand {

    @CommandLine.Option(names = {"-n", "--num"})
    protected Integer num;
}

I get results

[INFO] From ExecutionExceptionHandler ----
        Exception: picocli.CommandLine$ExecutionException: Parsed command (org.abelsromero.picocli.ParentCommand@33e5ccce) is not a Method, Runnable or Callable
        CommandLine: picocli.CommandLine@5f150435
        ParseResult from method: picocli.CommandLine$ParseResult@483bf400
                matchedArgs(): [field Integer org.abelsromero.picocli.ParentCommand.num]
                matchedOptions(): [field Integer org.abelsromero.picocli.ParentCommand.num]
        ParseResult from commandLine.getParseResult(): picocli.CommandLine$ParseResult@21a06946
                matchedArgs(): [field Integer org.abelsromero.picocli.ParentCommand.num]
                matchedOptions(): [field Integer org.abelsromero.picocli.ParentCommand.num]

[abelsromero]

Modified the test to reproduce

package picocli;

import org.junit.Test;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;
import picocli.CommandLine.ParseResult;

import java.util.List;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertSame;

public class Issue2228 {

    @Command(
        subcommands = {
            SubCommand.class
        })
    static class TestCommand {
    }

    @Command(name = "subsub")
    static class SubCommand implements Runnable {

        @Option(names = "-y")
        public boolean y;

        public void run() {
            throw new IllegalStateException("Y failing, just for fun");
        }
    }

    @Test
    public void testParseResult() {
        final CommandLine commandLine = new CommandLine(new Issue2228.TestCommand());
        final ParseResult[] caughtParseResult = new ParseResult[1];
        commandLine.setExecutionExceptionHandler(new CommandLine.IExecutionExceptionHandler() {
            public int handleExecutionException(Exception ex, CommandLine exCmdLine, ParseResult parseResult) throws Exception {
//                assertSame(commandLine, exCmdLine);
                assertNotNull(parseResult);
                List<CommandLine.Model.ArgSpec> argSpecs = parseResult.matchedArgs();
                List<CommandLine.Model.OptionSpec> optionSpecs = parseResult.matchedOptions();
                caughtParseResult[0] = parseResult;
                return 0;
            }
        });
        commandLine.execute("subsub");
        assertNotNull(caughtParseResult[0]);

        ParseResult after = commandLine.getParseResult();
        assertFalse(after.matchedArgs().isEmpty());
        assertFalse(after.matchedOptions().isEmpty());

        assertFalse(caughtParseResult[0].matchedArgs().isEmpty());
        assertFalse(caughtParseResult[0].matchedOptions().isEmpty());
    }
}

Line 40 assertSame(commandLine, exCmdLine); was commented because it also fails, I assume the CommandLine returned must be the one related to the sub command 🤔

[remkop]

Thank you for that! Now I see what is happening:

The parseResult object passed in to the handleExecutionException method is currently always the parseResult of the top-level command, regardless of which command or subcommand threw an exception.
That explains why the matchedArgs list is empty: because no options were matched for the top-level command, only for the subcommand.

This is inconsistent with the commandLine object that is passed in to the handleExecutionException method, which is the commandLine object of the command that actually threw the exception. It makes sense for the parseResult to do something similar.

I will change it so that the parseResult object passed in to the handleExecutionException method is also the parseResult of the command that actually threw the exception.

[remkop]

@abelsromero I was about to make this change, but now I am having second thoughts.
My concern is existing applications.
There may be existing applications that depend on the current behaviour, and changing the behaviour would break those applications.

Another way to solve this issue would be to clarify the current behaviour in the documentation;
both the javadoc for the IExecutionExceptionHandler interface, as well as the example in the user manual.

Some thing like this:

Parameters:
ex - the Exception thrown by the Runnable, Callable or Method user object of the command
commandLine - the CommandLine representing the command or subcommand where the exception occurred
parseResult - the result of parsing the command line arguments. This is the ParseResult of the *top-level command*. 
              Note that if the exception occurred in a subcommand, you may want to inspect the ParseResult of 
              the subcommand that threw the exception, which can be obtained by calling `commandLine.getParseResult()` 
              on the CommandLine object passed to this method.

Thoughts?

[abelsromero]

Thoughts?

Firstly, if this is a breaking change, it's fine to delay for a major release.

Then, also breaking change, why not remove parseResult from the method signature? If it depends on the command/sub-command and is not guaranteed to match it, that forces the user to run commandLine.getParseResult() anyway.

[remkop]

I am very, very hesitant to break anyone's application, even in major releases.
An alternative I could consider would be adding a new IExecutionExceptionHandler2 interface with a different method signature, but I am not convinced that this would really be an improvement.

The problem is not that the current behaviour is broken, it is simply not defined clearly enough:
I am sure there are use cases that require the full (top-level) parseResult. For example, there may be applications that need to inspect which options were specified on the parent of the command that actually failed.

Note also that the ParseResult class was designed to be navigated top-down, using its subcommand and subcommands methods that give access to the subcommand hierarchy of the chain of command > subcommand > sub-subcommand that was actually invoked. I think it makes sense to have access to the full (top-level) ParseResult.

I can see the inconsistency with the CommandLine parameter pointing to the subcommand where the exception was thrown. The existing documentation for the ParseResult parameter is not specific, so it is natural to assume that it would also be for that subcommand. This violates the principle of least surprise, and I am not happy about that.

However, rather than changing the API, I lean towards clarifying the documentation.

[abelsromero]

I am sure there are use cases that require the full (top-level) parseResult. For example, there may be applications that need to inspect which options were specified on the parent of the command that actually failed.

That's the issue to me, it does not fully address the problem. But said that, matching the current command will be fine for most users. It can be done and see how community reacts.