Skip to content

Converter not called as expected / difference in call by name vs position #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jclaudelin opened this issue Mar 20, 2023 · 2 comments
Closed
Labels
branch/3.0.x Issue for a branch branch/3.1.x Issue for a branch for/backport For backporting type/bug Is a bug report
Milestone

Comments

@jclaudelin
Copy link

jclaudelin commented Mar 20, 2023

Hello,
Up to recently we have been using Spring Shell 2.1.2 but after trying to upgrade to later version(s) we have noticed a change in behavior how Converters are called. Also, there seems to be a difference if the command invocation uses named parameters or positional parameters.

Given the below @ShellMetods and a custom Converter (String -> MyType) the results will be as in the table below with different versions of Spring Shell:

@ShellComponent
public class Commands {
    @ShellMethod(key="single")
    public void single(@ShellOption(value="--value") MyType single)
    {
        System.out.printf("single: %s%n", single);
    }

    @ShellMethod(key="list")
    public void list(@ShellOption(value="--values") List<MyType> list)
    {
        System.out.printf("list value: %s%n", list);
    }

    @ShellMethod(key="array")
    public void array(@ShellOption(value="--values") MyType[] array)
    {
        System.out.printf("array: %s%n", Arrays.toString(array));
    }

    @ShellMethod(key="set")
    public void set(@ShellOption(value="--values") Set<MyType> set)
    {
        System.out.printf("set: %s%n", set);
    }
}

And

  @Bean
  public Converter<String, MyType> myTypeConverter() {
      return new Converter<String, MyType>() {
          @Override
          public MyType convert(String source) {
              return new MyType(source);
          }
      };
  }

The following invocations are used

> list --values a,b
> list a,b
> array --values a,b
> array a,b

The expected behavior would be that the Converter is invoked for each value 'a', 'b' and the result being collected into the correct collection type.

The result is as below.

  • Green=OK
  • Orange=converter called but with the values as a single String ("a,b")
  • Red=converter not called at all (target method called with a List<String>)

image

Spring Shell list --values a,b list a,b array --values a,b array a,b
2.1.2 OK OK NOK [MyType{value=a,b}] OK
2.1.3 NOK [a, b] OK NOK [MyType{value=a,b}] OK
2.1.4 NOK [a, b] OK NOK [MyType{value=a,b}] OK
2.1.5 NOK [a, b] OK NOK [MyType{value=a,b}] OK
2.1.6 NOK [a,b] NOK [a,b] NOK [MyType{value=a,b}] NOK [MyType{value=a,b}]
2.1.7 NOK [a, b] NOK [a,b] NOK [MyType{value=a,b}] NOK [MyType{value=a,b}]
@github-actions github-actions bot added the status/need-triage Team needs to triage and take a first look label Mar 20, 2023
@jvalkeal
Copy link
Contributor

Thanks for the detailed explanation and yes I see the issues you outlined. I'll dig this a bit deeper before I put more comments here.

@jvalkeal jvalkeal added type/bug Is a bug report and removed status/need-triage Team needs to triage and take a first look labels Mar 24, 2023
@jvalkeal jvalkeal added this to the 2.1.8 milestone Apr 1, 2023
@jvalkeal jvalkeal added for/backport For backporting branch/3.0.x Issue for a branch branch/3.1.x Issue for a branch labels Apr 1, 2023
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue Apr 1, 2023
- In `CommandRegistration` add `ResolvableType` for `OptionSpec` giving
  more spesific handling of a type.
- In `CommandParser` handle source and target types so that we
  have generics with `List`, `Set` and arrays working better.
- In `HandlerMethodArgumentResolver` add better handling for
  `ConversionService` for generic types.
- In `StandardMethodTargetRegistrar` add better types via `ResolvableType`
  now that `CommandRegistration` support it.
- In `OptionConversionCommands` remove converter from `String` to `Set` as
  now things should work as is if generic in a `Set` has a converter.
- Fixes spring-projects#694
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue Apr 2, 2023
- In `CommandRegistration` add `ResolvableType` for `OptionSpec` giving
  more spesific handling of a type.
- In `CommandParser` handle source and target types so that we
  have generics with `List`, `Set` and arrays working better.
- In `HandlerMethodArgumentResolver` add better handling for
  `ConversionService` for generic types.
- In `StandardMethodTargetRegistrar` add better types via `ResolvableType`
  now that `CommandRegistration` support it.
- In `OptionConversionCommands` remove converter from `String` to `Set` as
  now things should work as is if generic in a `Set` has a converter.
- Fixes spring-projects#694
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue Apr 2, 2023
- In `CommandRegistration` add `ResolvableType` for `OptionSpec` giving
  more spesific handling of a type.
- In `CommandParser` handle source and target types so that we
  have generics with `List`, `Set` and arrays working better.
- In `HandlerMethodArgumentResolver` add better handling for
  `ConversionService` for generic types.
- In `StandardMethodTargetRegistrar` add better types via `ResolvableType`
  now that `CommandRegistration` support it.
- In `OptionConversionCommands` remove converter from `String` to `Set` as
  now things should work as is if generic in a `Set` has a converter.
- Backport spring-projects#694
- Fixes spring-projects#699
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue Apr 5, 2023
- In `CommandRegistration` add `ResolvableType` for `OptionSpec` giving
  more spesific handling of a type.
- In `CommandParser` handle source and target types so that we
  have generics with `List`, `Set` and arrays working better.
- In `HandlerMethodArgumentResolver` add better handling for
  `ConversionService` for generic types.
- In `StandardMethodTargetRegistrar` add better types via `ResolvableType`
  now that `CommandRegistration` support it.
- In `OptionConversionCommands` remove converter from `String` to `Set` as
  now things should work as is if generic in a `Set` has a converter.
- Backport spring-projects#694 spring-projects#699
- Fixes spring-projects#700
@jvalkeal jvalkeal closed this as completed Apr 6, 2023
jvalkeal added a commit that referenced this issue Apr 6, 2023
- Remove non jdk8 code used in #694
@jclaudelin
Copy link
Author

Hello,
Thanks for the correction, just wanted to confirm that everything works as expected in version 2.1.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/3.0.x Issue for a branch branch/3.1.x Issue for a branch for/backport For backporting type/bug Is a bug report
Projects
None yet
Development

No branches or pull requests

2 participants