Options with primitive types fail in 4.0 without explicit value

[ThomasVitale]

Before Spring Shell 4.0, boolean command options would default to false if not specified when executing the command. In 4.0, they fail instead with the following error message:

Caused by: org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.lang.String] to type [boolean] for value [null]
        at org.springframework.core.convert.support.GenericConversionService.assertNotPrimitiveTargetType(GenericConversionService.java:300)
        at org.springframework.core.convert.support.GenericConversionService.handleResult(GenericConversionService.java:293)
        at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:182)
        at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:165)
        at org.springframework.shell.core.command.adapter.MethodInvokerCommandAdapter.prepareArguments(MethodInvokerCommandAdapter.java:166)
        at org.springframework.shell.core.command.adapter.MethodInvokerCommandAdapter.doExecute(MethodInvokerCommandAdapter.java:90)
        at org.springframework.shell.core.command.AbstractCommand.execute(AbstractCommand.java:162)
        at org.springframework.shell.core.command.CommandExecutor.execute(CommandExecutor.java:71)
        ... 28 more
Caused by: java.lang.IllegalArgumentException: A null value cannot be assigned to a primitive type
        ... 36 more

So, a command definition like the following with Spring Shell 3.4:

    @Command(name = "test", description = "Test command.")
    public void build(
        @Option(description = "Perform a clean build.") boolean clean,
    ) {
        ...
    }

would need to change to the following in 4.0:

    @Command(name = "test", description = "Test command.")
    public void build(
        @Option(description = "Perform a clean build.", defaultValue = "false", longName = "clean") boolean clean,
    ) {
        ...
    }

That's counterintuitive because in Java the primitive type boolean is always false if unspecified.

Furthermore, when providing the option at execution time, a value is also required. This bit is mentioned in the docs, but it would be great to highlight that in the migration guide as a breaking change since it completely change the user experience.

It's common for CLIs to pass boolean options to a command without a value, meaning it's "true". Any chance the previous behaviour could be restored? This breaking change doesn't affect only developers when upgrading to 4.0, but also all users of CLIs implemented with Spring Shell, having to change commands like:

mycli test --clean

to:

mycli test --clean true

This is where the check is implemented:

spring-shell/spring-shell-core/src/main/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java

Lines 125 to 129 in 2a1f020

	if (shortName == ' ' && longName.isEmpty()) {
	throw new IllegalArgumentException(
	"Either shortName or longName (or both) must be provided for option on parameter '"
	+ parameter.getName() + "'");
	}

I'm available to help with a potential PR.

[czpilar]

This behavior was mentioned in #1248 and should be documented as a constraint.

[ThomasVitale]

@czpilar thanks for the reference! That seems to cover the second issue when parsing the input command. I'll have a look if it's possible to customise the parser to restore the previous default behaviour which is what CLI users would expect. But yeah, it would be great to document it in the migration/release notes.

About the first issue I mentioned (mandatory explicit default value for boolean properties via the @Option annotation), I guess that's part of the command registration. I'll have a look if it's possible to customise the implementation from the outside, but I hope it can be changed to rely on Java standard behaviour.

[czpilar]

About the first issue I mentioned (mandatory explicit default value for boolean properties via the @option annotation), I guess that's part of the command registration.

Not providing a defaultValue for an optional Option is, in my opinion, a valid scenario, as I also mentioned in the last sentence of my comment here: #1245 (comment)

[ThomasVitale]

I agree with that comment, thanks for sharing.

Also, not providing a default value for optional options is, in my opinion, a valid scenario, as I may want the option value to be null, or to use the Java default value in the case of primitive types, without showing any warning message.

I also expected the command registration logic to "use the Java default value in the case of primitive types", but that's not happening. If an option is not required and the Java type is boolean, I wouldn't expect to be required to define a default value as I would expect the Java default false.

Example in Spring Shell 3.4:

    @Command(name = "test", description = "Test command.")
    public void build(
        @Option(description = "Some optional option.") boolean myBooleanOption,
    ) {
        ...
    }

That doesn't work in 4.0 because the myBooleanOption option triggers an error as it's treated as null, which is not possible for primitive types. There might be a bug in the command registration logic which doesn't handle primitive types different than Objects, as they are still treated as null. So, I'm forced to define a defaultValue = "false" for all boolean options in my CLI applications.

org.springframework.shell.core.command.CommandExecutionException: Unable to execute command build
        at org.springframework.shell.core.command.CommandExecutor.execute(CommandExecutor.java:74) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.NonInteractiveShellRunner.executeCommand(NonInteractiveShellRunner.java:142) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.NonInteractiveShellRunner.run(NonInteractiveShellRunner.java:81) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.autoconfigure.ShellRunnerAutoConfiguration.lambda$springShellApplicationRunner$0(ShellRunnerAutoConfiguration.java:41) ~[spring-shell-core-autoconfigure-4.0.0.jar!/:4.0.0]
        at org.springframework.boot.SpringApplication.lambda$callRunner$0(SpringApplication.java:788) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.util.function.ThrowingConsumer$1.acceptWithException(ThrowingConsumer.java:82) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.util.function.ThrowingConsumer.accept(ThrowingConsumer.java:60) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.util.function.ThrowingConsumer$1.accept(ThrowingConsumer.java:86) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:800) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:788) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.boot.SpringApplication.lambda$callRunners$0(SpringApplication.java:776) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:186) ~[na:na]
        at java.base/java.util.stream.SortedOps$SizedRefSortingSink.end(SortedOps.java:357) ~[na:na]
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:571) ~[na:na]
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560) ~[na:na]
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:153) ~[na:na]
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:176) ~[na:na]
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265) ~[na:na]
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:632) ~[na:na]
        at org.springframework.boot.SpringApplication.callRunners(SpringApplication.java:776) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.boot.SpringApplication.run(SpringApplication.java:328) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.boot.SpringApplication.run(SpringApplication.java:1365) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at org.springframework.boot.SpringApplication.run(SpringApplication.java:1354) ~[spring-boot-4.0.1.jar!/:4.0.1]
        at io.arconia.cli.ArconiaCli.main(ArconiaCli.java:14) ~[!/:0.9.6-SNAPSHOT]
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) ~[na:na]
        at java.base/java.lang.reflect.Method.invoke(Method.java:565) ~[na:na]
        at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:106) ~[arconia-cli-0.9.6-SNAPSHOT.jar:0.9.6-SNAPSHOT]
        at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:64) ~[arconia-cli-0.9.6-SNAPSHOT.jar:0.9.6-SNAPSHOT]
        at org.springframework.boot.loader.launch.JarLauncher.main(JarLauncher.java:40) ~[arconia-cli-0.9.6-SNAPSHOT.jar:0.9.6-SNAPSHOT]
Caused by: org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.lang.String] to type [boolean] for value [null]
        at org.springframework.core.convert.support.GenericConversionService.assertNotPrimitiveTargetType(GenericConversionService.java:300) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.core.convert.support.GenericConversionService.handleResult(GenericConversionService.java:293) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:182) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:165) ~[spring-core-7.0.2.jar!/:7.0.2]
        at org.springframework.shell.core.command.adapter.MethodInvokerCommandAdapter.prepareArguments(MethodInvokerCommandAdapter.java:166) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.command.adapter.MethodInvokerCommandAdapter.doExecute(MethodInvokerCommandAdapter.java:90) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.command.AbstractCommand.execute(AbstractCommand.java:162) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        at org.springframework.shell.core.command.CommandExecutor.execute(CommandExecutor.java:71) ~[spring-shell-core-4.0.0.jar!/:4.0.0]
        ... 28 common frames omitted
Caused by: java.lang.IllegalArgumentException: A null value cannot be assigned to a primitive type
        ... 36 common frames omitted

[fmbenhassine]

Thank you for reporting this @ThomasVitale and thank you for the follow up @czpilar !

This is definitely awful and should be fixed. However, there are two distinct issues reported here:

1. Primitive types should have their default values as assigned in Java
2. The default parser should be improved to accept boolean options without having to provide a value. I created Improve default parser to accept boolean options without values #1304 for that.

I will edit the title of the issue to focus on 1) and solve 2) in #1304.