Skip to content

Commit eaed8d4

Browse files
somiljain2006somiljain2006
authored
Improve setter resolution logic in BaseConfigurator (#2825)
1 parent ce04abb commit eaed8d4

2 files changed

Lines changed: 631 additions & 31 deletions

File tree

‎plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java‎

Lines changed: 201 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
package io.jenkins.plugins.casc;
22

3+
import static java.lang.reflect.Array.newInstance;
4+
import static java.lang.reflect.Array.set;
5+
36
import edu.umd.cs.findbugs.annotations.NonNull;
47
import hudson.BulkChange;
58
import hudson.model.Describable;
@@ -22,7 +25,6 @@
2225
import java.lang.reflect.TypeVariable;
2326
import java.lang.reflect.WildcardType;
2427
import java.util.ArrayList;
25-
import java.util.Arrays;
2628
import java.util.Collection;
2729
import java.util.Collections;
2830
import java.util.HashMap;
@@ -37,6 +39,7 @@
3739
import org.kohsuke.accmod.AccessRestriction;
3840
import org.kohsuke.accmod.Restricted;
3941
import org.kohsuke.accmod.restrictions.Beta;
42+
import org.kohsuke.accmod.restrictions.NoExternalUse;
4043
import org.kohsuke.accmod.restrictions.None;
4144

4245
/**
@@ -76,57 +79,116 @@ public abstract class BaseConfigurator<T> implements Configurator<T> {
7679
}
7780

7881
final Class<T> target = getTarget();
82+
Map<String, List<Method>> methodsByProperty = new HashMap<>();
7983
// Resolve the methods and merging overrides to more concretized signatures
8084
// because the methods can to have been overridden with concretized type
81-
// TODO: Overloaded setters with different types can corrupt this logic
8285
for (Method method : target.getMethods()) {
8386
final String methodName = method.getName();
84-
TypePair type;
8587
if (method.getParameterCount() == 0
8688
&& methodName.startsWith("get")
8789
&& PersistedList.class.isAssignableFrom(method.getReturnType())) {
88-
type = TypePair.ofReturnType(method);
89-
} else if (method.getParameterCount() != 1 || !methodName.startsWith("set")) {
90-
// Not an accessor, ignore
90+
String name = StringUtils.uncapitalize(methodName.substring(3));
91+
if (exclusions.contains(name)) {
92+
continue;
93+
}
94+
95+
TypePair type = TypePair.ofReturnType(method);
96+
@SuppressWarnings("unchecked")
97+
Attribute<T, ?> attribute = (Attribute<T, ?>) createAttribute(name, type);
98+
99+
if (attribute != null) {
100+
attribute.deprecated(method.getAnnotation(Deprecated.class) != null);
101+
final Restricted r = method.getAnnotation(Restricted.class);
102+
if (r != null) {
103+
attribute.restrictions(r.value());
104+
}
105+
attributes.putIfAbsent(name, attribute);
106+
}
91107
continue;
92-
} else {
93-
type = TypePair.ofParameter(method, 0);
94108
}
95109

96-
final String s = methodName.substring(3);
97-
final String name = StringUtils.uncapitalize(s);
98-
if (exclusions.contains(name)) {
99-
continue;
110+
if (method.getParameterCount() == 1 && methodName.startsWith("set")) {
111+
String propertySuffix = methodName.substring(3);
112+
final String name = StringUtils.uncapitalize(propertySuffix);
113+
114+
if (exclusions.contains(name)) {
115+
continue;
116+
}
117+
methodsByProperty
118+
.computeIfAbsent(propertySuffix, k -> new ArrayList<>())
119+
.add(method);
100120
}
121+
}
101122

102-
if (!hasGetter(target, s)) {
103-
// Looks like a property but no actual getter method we can use to read value
123+
for (Map.Entry<String, List<Method>> entry : methodsByProperty.entrySet()) {
124+
final String propertySuffix = entry.getKey();
125+
final String name = StringUtils.uncapitalize(propertySuffix);
126+
127+
Method g = findGetter(target, propertySuffix);
128+
129+
if (g == null) {
104130
continue;
105131
}
106132

107-
LOGGER.log(Level.FINER, "Processing {0} property", name);
133+
Class<?> getterRawType = g.getReturnType();
134+
135+
List<Method> candidateSetters = entry.getValue().stream()
136+
.filter(m -> {
137+
Class<?> paramType = m.getParameterTypes()[0];
138+
return isSameType(paramType, getterRawType)
139+
|| getterRawType.isAssignableFrom(paramType)
140+
|| paramType.isAssignableFrom(getterRawType);
141+
})
142+
.collect(Collectors.toList());
143+
144+
if (candidateSetters.isEmpty()) {
145+
candidateSetters = entry.getValue();
146+
}
147+
148+
Method bestMethod = resolveBestSetter(candidateSetters, getterRawType);
149+
TypePair type = TypePair.ofParameter(bestMethod, 0);
150+
151+
TypePair getterType = TypePair.ofReturnType(g);
152+
if (type.rawType.isAssignableFrom(getterType.rawType)) {
153+
type = getterType;
154+
}
108155

109156
if (Map.class.isAssignableFrom(type.rawType)) {
110157
// yaml has support for Maps, but as nobody seem to like them we agreed not to support them
111158
LOGGER.log(Level.FINER, "{0} is a Map<?,?>. We decided not to support Maps.", name);
112159
continue;
113160
}
114161

115-
Attribute attribute = createAttribute(name, type);
116-
if (attribute == null) {
162+
final TypePair finalType = type;
163+
164+
@SuppressWarnings("unchecked")
165+
Attribute<T, Object> rawAttribute = (Attribute<T, Object>) createAttribute(name, type);
166+
if (rawAttribute == null) {
117167
continue;
118168
}
169+
rawAttribute.setter((targetInstance, value) -> {
170+
Object finalValue = value;
171+
172+
if (value instanceof Collection<?> collection && finalType.rawType.isArray()) {
173+
Object array = newInstance(rawAttribute.getType(), collection.size());
174+
int i = 0;
175+
for (Object item : collection) {
176+
set(array, i++, item);
177+
}
178+
finalValue = array;
179+
}
180+
bestMethod.invoke(targetInstance, finalValue);
181+
});
119182

120-
attribute.deprecated(method.getAnnotation(Deprecated.class) != null);
121-
final Restricted r = method.getAnnotation(Restricted.class);
183+
rawAttribute.deprecated(bestMethod.getAnnotation(Deprecated.class) != null);
184+
final Restricted r = bestMethod.getAnnotation(Restricted.class);
122185
if (r != null) {
123-
attribute.restrictions(r.value());
186+
rawAttribute.restrictions(r.value());
124187
}
125188

126-
Attribute prevAttribute = attributes.get(name);
127-
// Replace the method if it have more concretized type
128-
if (prevAttribute == null || prevAttribute.type.isAssignableFrom(attribute.type)) {
129-
attributes.put(name, attribute);
189+
Attribute<T, ?> prevAttribute = attributes.get(name);
190+
if (prevAttribute == null || ((Class<?>) prevAttribute.type).isAssignableFrom(rawAttribute.type)) {
191+
attributes.put(name, rawAttribute);
130192
}
131193
}
132194

@@ -136,14 +198,25 @@ public abstract class BaseConfigurator<T> implements Configurator<T> {
136198
/**
137199
* Check if target class has a Getter method for property s
138200
*/
139-
private boolean hasGetter(Class<T> c, String s) {
140-
List<String> candidates = Arrays.asList("get" + s, "is" + s);
201+
private Method findGetter(Class<T> c, String s) {
202+
String getMethod = "get" + s;
203+
String isMethod = "is" + s;
204+
141205
for (Method m : c.getMethods()) {
142-
if (m.getParameterCount() == 0 && candidates.contains(m.getName())) {
143-
return true;
206+
if (m.getParameterCount() == 0) {
207+
if (m.getName().equals(getMethod)) {
208+
return m;
209+
}
210+
211+
if (m.getName().equals(isMethod)) {
212+
Class<?> returnType = m.getReturnType();
213+
if (returnType == boolean.class || returnType == Boolean.class) {
214+
return m;
215+
}
216+
}
144217
}
145218
}
146-
return false;
219+
return null;
147220
}
148221

149222
/**
@@ -439,9 +512,9 @@ public static final class TypePair {
439512
/**
440513
* Erasure of {@link #type}
441514
*/
442-
final Class rawType;
515+
final Class<?> rawType;
443516

444-
public TypePair(Type type, Class rawType) {
517+
public TypePair(Type type, Class<?> rawType) {
445518
this.rawType = rawType;
446519
this.type = type;
447520
}
@@ -476,4 +549,101 @@ public boolean equals(Object obj) {
476549
public int hashCode() {
477550
return getTarget().hashCode();
478551
}
552+
553+
@Restricted(NoExternalUse.class)
554+
Method resolveBestSetter(List<Method> methods, Class<?> getterRawType) {
555+
List<Method> realMethods =
556+
methods.stream().filter(m -> !m.isBridge() && !m.isSynthetic()).collect(Collectors.toList());
557+
if (!realMethods.isEmpty()) {
558+
methods = realMethods;
559+
}
560+
if (methods.size() == 1) {
561+
return methods.get(0);
562+
}
563+
564+
if (getterRawType != null) {
565+
for (Method m : methods) {
566+
if (isSameType(m.getParameterTypes()[0], getterRawType)) {
567+
return m;
568+
}
569+
}
570+
}
571+
List<Method> arrayMethods =
572+
methods.stream().filter(m -> m.getParameterTypes()[0].isArray()).collect(Collectors.toList());
573+
if (!arrayMethods.isEmpty()) {
574+
methods = arrayMethods;
575+
}
576+
577+
Method best = null;
578+
Class<?> bestType = null;
579+
580+
for (Method m : methods) {
581+
Class<?> currentType = m.getParameterTypes()[0];
582+
583+
if (best == null) {
584+
best = m;
585+
bestType = currentType;
586+
continue;
587+
}
588+
589+
if (bestType.isAssignableFrom(currentType)) {
590+
best = m;
591+
bestType = currentType;
592+
} else if (!currentType.isAssignableFrom(bestType)) {
593+
boolean currentMatch = (getterRawType != null && getterRawType.isAssignableFrom(currentType));
594+
boolean bestMatch = (getterRawType != null && getterRawType.isAssignableFrom(bestType));
595+
596+
if (currentMatch == bestMatch
597+
&& ((!currentType.isInterface() && bestType.isInterface())
598+
|| (currentType.isInterface() == bestType.isInterface()
599+
&& currentType.getName().compareTo(bestType.getName()) < 0))) {
600+
601+
best = m;
602+
bestType = currentType;
603+
}
604+
}
605+
}
606+
return best;
607+
}
608+
609+
private boolean isSameType(Class<?> a, Class<?> b) {
610+
if (a == b) {
611+
return true;
612+
}
613+
if (a.isPrimitive() && !b.isPrimitive()) {
614+
return isWrapper(b, a);
615+
}
616+
if (b.isPrimitive() && !a.isPrimitive()) {
617+
return isWrapper(a, b);
618+
}
619+
return false;
620+
}
621+
622+
private boolean isWrapper(Class<?> wrapper, Class<?> primitive) {
623+
if (primitive == int.class) {
624+
return wrapper == Integer.class;
625+
}
626+
if (primitive == boolean.class) {
627+
return wrapper == Boolean.class;
628+
}
629+
if (primitive == long.class) {
630+
return wrapper == Long.class;
631+
}
632+
if (primitive == double.class) {
633+
return wrapper == Double.class;
634+
}
635+
if (primitive == float.class) {
636+
return wrapper == Float.class;
637+
}
638+
if (primitive == byte.class) {
639+
return wrapper == Byte.class;
640+
}
641+
if (primitive == char.class) {
642+
return wrapper == Character.class;
643+
}
644+
if (primitive == short.class) {
645+
return wrapper == Short.class;
646+
}
647+
return false;
648+
}
479649
}

0 commit comments

Comments
 (0)