cbeust / jcommander

Command line parsing framework for Java
Apache License 2.0
1.95k stars 332 forks source link

[BUG] Invalid type inference for Sets of Enums #509

Closed yeikel closed 4 months ago

yeikel commented 3 years ago

I have the following parameter :

@Parameter(names = { "--roles"}, description = "List of roles separated by comma")
private Set<Relationship.TypeEnum> roles = new HashSet<>(Arrays.asList(Relationship.TypeEnum.values()));

And when I pass a Role as a String, for example --roles "SUPP", it creates a Set<String>instead of a Set<Relationship.TypeEnum>

image

This behavior does not happen with ArrayLists

image

Harlan1997 commented 3 years ago

I will work on this issue.

Harlan1997 commented 3 years ago

I wrote a test to reproduce this problem.

package com.beust.jcommander;

import org.testng.Assert;
import org.testng.annotations.Test;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;

public class SetOfEnumTest {

    public enum Season{
        SPRING,
        SUMMER,
        AUTUMN,
        WINTER;
    }

    @Test
    public void testParse()
    {
        class Args {
            @Parameter(names = { "--season"}, description = "List of seasons separated by comma")
            private Set<Season> seasons = new HashSet<>();
        }
        Args args = new Args();
        JCommander.newBuilder()
                .addObject(args)
                .build()
                .parse("--season", "SPRING");
        Assert.assertEquals(Season.class, args.seasons.toArray()[0].getClass());
    }

    public static void main(String[] args) {
        new SetOfEnumTest().testParse();
    }
}

The result of this test is:

java.lang.AssertionError: expected [class java.lang.String] but found [class com.beust.jcommander.SetOfEnumTest$Season]
Expected :class java.lang.String
Actual   :class com.beust.jcommander.SetOfEnumTest$Season
Harlan1997 commented 3 years ago

The cause of this problem is that the current approach to convertValue() in JCommander only considers List conversion. To fix it, convertValue() should consider other collections like Set().

if (type.isAssignableFrom(List.class)) -> if (type.isAssignableFrom(List.class) || type.isAssignableFrom(Set.class))
if (type.isAssignableFrom(List.class) && converter == null) -> if ((type.isAssignableFrom(List.class) || type.isAssignableFrom(Set.class)) && converter == null)

Then it will pass the test.

yeikel commented 4 months ago

Fixed with https://github.com/cbeust/jcommander/pull/519/files