Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-26535] added support for parameterized parameter types #52

Closed
wants to merge 3 commits into from
Closed

[JENKINS-26535] added support for parameterized parameter types #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68d5fb2
added support for parameterized parameter types
daspilker Aug 19, 2019
4c5d515
added more test cases for JENKINS-26535
daspilker Aug 20, 2019
7a7a5b6
Update plugin/src/test/java/org/jenkinsci/plugins/structs/describable…
daspilker Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions plugin/pom.xml
View file
Open in desktop
Expand Up @@ -21,6 +21,11 @@
<artifactId>symbol-annotation</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.7</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>junit</artifactId>
Expand Down
7 changes: 4 additions & 3 deletions plugin/src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.ClassUtils;
import org.apache.commons.lang.ObjectUtils;
import org.apache.commons.lang3.reflect.TypeUtils;
import org.codehaus.groovy.reflection.ReflectionCache;
import org.jenkinsci.Symbol;
import org.jenkinsci.plugins.structs.SymbolLookup;
Expand Down Expand Up @@ -154,7 +155,7 @@ public DescribableModel(Class<T> clazz) {
}

constructor = findConstructor(constructorParamNames.length);

Type[] types = constructor.getGenericParameterTypes();
for (int i = 0; i < constructorParamNames.length; i++) {
addParameter(parameters, types[i], constructorParamNames[i], null);
Expand Down Expand Up @@ -603,11 +604,11 @@ private List<Object> coerceList(String context, Type type, List<?> list, TaskLis
return null;
}

static Set<Class<?>> findSubtypes(Class<?> supertype) {
static Set<Class<?>> findSubtypes(Type supertype) {
Set<Class<?>> clazzes = new HashSet<Class<?>>();
// Jenkins.getDescriptorList does not work well since it is limited to descriptors declaring one supertype, and does not work at all for SimpleBuildStep.
for (Descriptor<?> d : ExtensionList.lookup(Descriptor.class)) {
if (supertype.isAssignableFrom(d.clazz)) {
if (TypeUtils.isAssignable(d.clazz, supertype)) {
clazzes.add(d.clazz);
}
}
Expand Down
64 changes: 32 additions & 32 deletions plugin/src/main/java/org/jenkinsci/plugins/structs/describable/ParameterType.java
View file
Open in desktop
Expand Up @@ -61,47 +61,47 @@ static ParameterType of(Type type) {
if (c.isArray()) {
return new ArrayType(c);
}
// Assume it is a nested object of some sort.
Set<Class<?>> subtypes = DescribableModel.findSubtypes(c);
if ((subtypes.isEmpty() && !Modifier.isAbstract(c.getModifiers())) || subtypes.equals(Collections.singleton(c))) {
// Probably homogeneous. (Might be concrete but subclassable.)
return new HomogeneousObjectType(c);
} else {
// Definitely heterogeneous.
Map<String,List<Class<?>>> subtypesBySimpleName = new HashMap<String,List<Class<?>>>();
for (Class<?> subtype : subtypes) {
String simpleName = subtype.getSimpleName();
List<Class<?>> bySimpleName = subtypesBySimpleName.get(simpleName);
if (bySimpleName == null) {
subtypesBySimpleName.put(simpleName, bySimpleName = new ArrayList<Class<?>>());
}
bySimpleName.add(subtype);
}
if (Types.isSubClassOf(type, Collection.class)) {
return new ArrayType(type, of(Types.getTypeArgument(Types.getBaseClass(type,Collection.class), 0, Object.class)));
}
Class<?> c = Types.erasure(type);
// Assume it is a nested object of some sort.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this is actually reindentation, making the diff look a lot bigger than it actually is. Had to apply the WS ignore setting in GitHub to see it.

Set<Class<?>> subtypes = DescribableModel.findSubtypes(type);
if ((subtypes.isEmpty() && !Modifier.isAbstract(c.getModifiers())) || subtypes.equals(Collections.singleton(c))) {
// Probably homogeneous. (Might be concrete but subclassable.)
return new HomogeneousObjectType(c);
} else {
// Definitely heterogeneous.
Map<String, List<Class<?>>> subtypesBySimpleName = new HashMap<String, List<Class<?>>>();
for (Class<?> subtype : subtypes) {
String simpleName = subtype.getSimpleName();
List<Class<?>> bySimpleName = subtypesBySimpleName.get(simpleName);
if (bySimpleName == null) {
subtypesBySimpleName.put(simpleName, bySimpleName = new ArrayList<Class<?>>());
}
Map<String,DescribableModel<?>> types = new TreeMap<String,DescribableModel<?>>();
for (Map.Entry<String,List<Class<?>>> entry : subtypesBySimpleName.entrySet()) {
if (entry.getValue().size() == 1) { // normal case: unambiguous via simple name
bySimpleName.add(subtype);
}
Map<String, DescribableModel<?>> types = new TreeMap<String, DescribableModel<?>>();
for (Map.Entry<String, List<Class<?>>> entry : subtypesBySimpleName.entrySet()) {
if (entry.getValue().size() == 1) { // normal case: unambiguous via simple name
try {
types.put(entry.getKey(), DescribableModel.of(entry.getValue().get(0)));
} catch (Exception x) {
LOGGER.log(Level.FINE, "skipping subtype", x);
}
} else { // have to diambiguate via FQN
for (Class<?> subtype : entry.getValue()) {
try {
types.put(entry.getKey(), DescribableModel.of(entry.getValue().get(0)));
types.put(subtype.getName(), DescribableModel.of(subtype));
} catch (Exception x) {
LOGGER.log(Level.FINE, "skipping subtype", x);
}
} else { // have to diambiguate via FQN
for (Class<?> subtype : entry.getValue()) {
try {
types.put(subtype.getName(), DescribableModel.of(subtype));
} catch (Exception x) {
LOGGER.log(Level.FINE, "skipping subtype", x);
}
}
}
}
return new HeterogeneousObjectType(c, types);
}
return new HeterogeneousObjectType(c, types);
}
if (Types.isSubClassOf(type, Collection.class)) {
return new ArrayType(type, of(Types.getTypeArgument(Types.getBaseClass(type,Collection.class), 0, Object.class)));
}
throw new UnsupportedOperationException("do not know how to categorize attributes of type " + type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have been removed and I am not sure why. Where is the fallback?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is just not necessary any more and whatever sanity-checking was needed is already done in DescribableModel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback became unreachable, so I removed it. I was surprised that no unit tests failed. There is not much sanity-checking in DescribableModel or DescribableParameter.

I thought about limiting support for Homo/HeterogeneousObjectType to Describable types and keeping the fallback, but that triggered a test failure in DescribableModelTest#recursion since Recursion is not a Describable. I'm not sure if that is on purpose.

} catch (Exception x) {
return new ErrorType(x, type);
}
Expand Down
82 changes: 82 additions & 0 deletions plugin/src/test/java/org/jenkinsci/plugins/structs/describable/DescribableModelTest.java
View file
Open in desktop
Expand Up @@ -1129,4 +1129,86 @@ public void setLegacyMode(Integer legacyMode) {
this.legacyMode = legacyMode;
}
}

public static class Generics {
private final Option<? extends Number> option;
private List<? extends Option> values1;
private List<? extends Option<?>> values2;
private List<? extends Option<? extends Number>> values3;

@DataBoundConstructor
public Generics(Option<? extends Number> option) {
this.option = option;
}

public Option<? extends Number> getOption() {
return option;
}

public List<? extends Option> getValues1() {
return values1;
}

@DataBoundSetter
public void setValues1(List<? extends Option> values1) {
this.values1 = values1;
}

public List<? extends Option<?>> getValues2() {
return values2;
}

@DataBoundSetter
public void setValues2(List<? extends Option<?>> values2) {
this.values2 = values2;
}

public List<? extends Option<? extends Number>> getValues3() {
return values3;
}

@DataBoundSetter
public void setValues3(List<? extends Option<? extends Number>> values3) {
this.values3 = values3;
}
}

public static abstract class Option<T> extends AbstractDescribableImpl<Option<?>> {
}

public static class LongOption extends Option<Long> {
@DataBoundConstructor
public LongOption() {}

@Extension
public static final class DescriptorImpl extends Descriptor<Option<?>> {
@Override public String getDisplayName() {
return "LongOption";
}
}
}

public static class StringOption extends Option<String> {
@DataBoundConstructor
public StringOption() {}

@Extension
public static final class DescriptorImpl extends Descriptor<Option<?>> {
@Override public java.lang.String getDisplayName() {
return "StringOption";
}
}
}

@Test
daspilker marked this conversation as resolved.
Show resolved Hide resolved
@Issue("JENKINS-26535")
public void generics() throws Exception {
schema(Generics.class, "Generics(option: Option{LongOption()}, values1?: Option{LongOption() | StringOption()}[], values2?: Option{LongOption() | StringOption()}[], values3?: Option{LongOption()}[])");
roundTrip(Generics.class, map(
"option", map(CLAZZ, "LongOption"),
"values1", Collections.singletonList(map(CLAZZ, "StringOption")),
"values2", Arrays.asList(map(CLAZZ, "StringOption"), map(CLAZZ, "LongOption")),
"values3", Collections.singletonList(map(CLAZZ, "LongOption"))
));
}
}