Enum type conversion errors are too technical

[tincore]

trafficstars

Version 3.0.2

Both programmatical and annotation models.

When a @ShellOption is of type enum and the introduced value is not in enumeration (case sensitive) the thrown ConversionUtils ConversionFailedException just bubbles up.

ConversionFailedException: Failed to covert from type [java.lang.String] to type [....]

AFAIK there is no chance to provide a meaningful error like. 'Invalid value introduced, field=xxxxx, possible values aaa,bbb,ccc...'

Event using a CommandExceptionResolver is not easy because the ConversionFailedException is not wrapped in OptionException. This could help identify which of the command option is failing.

[jvalkeal]

I think you're referring to:

my-shell:>e2e reg option-type-enum --arg1 ONE
Hello ONE

my-shell:>e2e reg option-type-enum --arg1 one
2002E:(pos 0): Illegal option value 'one', reason 'Failed to convert from type [java.lang.String] to type [org.springframework.shell.samples.e2e.OptionTypeCommands$OptionTypeEnum] for value [one]'

Yes this is not optimal. We could try to relax to forget case sensitivity which doesn't really fix the other issue notifying user about what are allowed values.

As a user what kind of hooks you'd like to have to give meaningful error to an end user?

[tincore]

Thanks for the response.

I've just checked the main branch sources. It already looks superior to 3.0.x!

I'd say that it would be a good idea to have the possibility of intercept/manipulate additions to commonMessageResults particularly the one in the try catch block as it is the most prone to customization needs.

If the hook is, for instance, placed in CommandOption (currentOption) then I guess that a function MessageResult -> MessageResult with a default identity implementation could do the trick. Just an idea.

at

org.springframework.shell.command.parser.Parser

@Override
		protected void onExitOptionNode(OptionNode node) {
			if (!currentOptions.isEmpty()) {
				for (CommandOption currentOption : currentOptions) {
......
commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg,
								currentOptionArgument.size()));
......
commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg,
								currentOption.getArityMax()));
					}
					} catch (Exception e) {
						commonMessageResults.add(MessageResult.of(ParserMessage.ILLEGAL_OPTION_VALUE, 0, value, e.getMessage())); <------------------- This one specially
					}
......				invalidOptionNodes.add(node);
			}
		}

Some ideas

					} catch (Exception e) {
// One possibility
                                                MessageResult messageResult = currentOption.getMessageResultCustomizer().apply( MessageResult.of(ParserMessage.ILLEGAL_OPTION_VALUE, 0, value, e.getMessage()));

// Other possiblity
                                                MessageResult messageResult = currentOption.onMessageResult( MessageResult.of(ParserMessage.ILLEGAL_OPTION_VALUE, 0, value, e.getMessage()));


						commonMessageResults.add(messageResult);
					}

Now that I'm looking at the code with more attention. Wouldn't it be a good idea to also add hooks to validate post conversion? In this way all message results would be displayed together (custom validations+missing required options etc...). Consumer<CommandOption, T value, List<MessageResult>>

[jvalkeal]

Thanks for a comments!

This new parser framework was created to clean up a mess from previous rewrite and get more close to modern ways how parsers should work, thus having concepts of parser, lexer and ast. Main motivation was to allow having parser configuration(this hasn't really happened yet) which might change the behaviour what should be recognised. Effect is that we now have a better infrastructure to add new behaviour and is easier to test(that's why people are usually nervous to touch parsing code).

I've been thinking if these hooks could somewhat work with ideas from Boot Failure Analyzer https://www.baeldung.com/spring-boot-failure-analyzer. We can't directly use it( but can shamelessly copy/duplicate ideas) but essentially these cryptic errors boil down to analysing what the exception really means. We'd need to look how detailed info we'd be able to dig out from an exceptions(generally speaking) from Spring ConversionService.

[tincore]

Lets say that you have a command that takes 4 required enumerations and, for some reason, the user introduces 2 of them wrong (so internally a converter error happened for each of those 2) and leaves the 4th empty.

I think that it would look great if we can return all the errors as a single block with the capability of influencing the conversion/error message procedure.

I'm not sure but it seems like a failure analyzer would catch the 1st exception that bubbles up and aborting the process.

[jvalkeal]

Sorry I may have been a bit unclear what I meant by "steeling" an idea from a boot's failure-analyzer and that's why I mentioned we can't use is as is. Idea in a boot's failure-analyzer was to translate a complex stacktrace or errors into more meaningful error what a user can understand. In a boot/framework context it usually is to open up complex errors of bean cycle errors or dependency errors.

In spring shell we also may have a complex set of errors depending on what comes out from a parser. That was my idea of a analyzer system(which might then be extensible by a user). Spring Shell itself could provide common set of analyzer implementations for a usual cases.