Skip to content

Commit 70c2d43

Browse files
somiljain2006somiljain2006
committed
Improve setter resolution logic in BaseConfigurator
1 parent ba36d7a commit 70c2d43

2 files changed

Lines changed: 265 additions & 26 deletions

File tree

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

Lines changed: 167 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import java.lang.reflect.TypeVariable;
2323
import java.lang.reflect.WildcardType;
2424
import java.util.ArrayList;
25-
import java.util.Arrays;
2625
import java.util.Collection;
2726
import java.util.Collections;
2827
import java.util.HashMap;
@@ -76,57 +75,119 @@ public abstract class BaseConfigurator<T> implements Configurator<T> {
7675
}
7776

7877
final Class<T> target = getTarget();
78+
Map<String, TypePair> getterCache = new HashMap<>();
79+
Map<String, List<Method>> methodsByProperty = new HashMap<>();
7980
// Resolve the methods and merging overrides to more concretized signatures
8081
// because the methods can to have been overridden with concretized type
81-
// TODO: Overloaded setters with different types can corrupt this logic
8282
for (Method method : target.getMethods()) {
8383
final String methodName = method.getName();
84-
TypePair type;
8584
if (method.getParameterCount() == 0
8685
&& methodName.startsWith("get")
8786
&& PersistedList.class.isAssignableFrom(method.getReturnType())) {
88-
type = TypePair.ofReturnType(method);
89-
} else if (method.getParameterCount() != 1 || !methodName.startsWith("set")) {
90-
// Not an accessor, ignore
87+
88+
String propertySuffix = methodName.substring(3);
89+
final String name = StringUtils.uncapitalize(propertySuffix);
90+
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)) {
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);
120+
}
121+
}
122+
123+
for (Map.Entry<String, List<Method>> entry : methodsByProperty.entrySet()) {
124+
final String propertySuffix = entry.getKey();
125+
final String name = StringUtils.uncapitalize(propertySuffix);
126+
final List<Method> methods = entry.getValue();
127+
128+
TypePair getterType = getterCache.computeIfAbsent(propertySuffix, prop -> {
129+
Method g = findGetter(target, prop);
130+
return g != null ? TypePair.ofReturnType(g) : null;
131+
});
132+
133+
if (getterType == null) {
134+
LOGGER.log(Level.FINER, "Ignoring property {0}: no compatible getter found", name);
99135
continue;
100136
}
101137

102-
if (!hasGetter(target, s)) {
103-
// Looks like a property but no actual getter method we can use to read value
138+
final Class<?> rawType = getterType.rawType;
139+
140+
List<Method> validSetters = methods.stream()
141+
.filter(m -> {
142+
Class<?> paramType = m.getParameterCount() == 0 ? m.getReturnType() : m.getParameterTypes()[0];
143+
return paramType.equals(rawType)
144+
|| rawType.isAssignableFrom(paramType)
145+
|| paramType.isAssignableFrom(rawType);
146+
})
147+
.toList();
148+
149+
if (validSetters.isEmpty()) {
150+
LOGGER.log(
151+
Level.FINER,
152+
"Ignoring property {0}: Setters exist, but none are compatible with getter type {1}",
153+
new Object[] {name, getterType.rawType.getName()});
104154
continue;
105155
}
106156

107157
LOGGER.log(Level.FINER, "Processing {0} property", name);
108158

159+
Method bestMethod = resolveBestSetter(validSetters, getterType.rawType);
160+
161+
TypePair type = TypePair.ofParameter(bestMethod, 0);
162+
109163
if (Map.class.isAssignableFrom(type.rawType)) {
110164
// yaml has support for Maps, but as nobody seem to like them we agreed not to support them
111165
LOGGER.log(Level.FINER, "{0} is a Map<?,?>. We decided not to support Maps.", name);
112166
continue;
113167
}
114168

115-
Attribute attribute = createAttribute(name, type);
169+
@SuppressWarnings("unchecked")
170+
Attribute<T, ?> attribute = (Attribute<T, ?>) createAttribute(name, type);
116171
if (attribute == null) {
117172
continue;
118173
}
119174

120-
attribute.deprecated(method.getAnnotation(Deprecated.class) != null);
121-
final Restricted r = method.getAnnotation(Restricted.class);
175+
attribute.deprecated(bestMethod.getAnnotation(Deprecated.class) != null);
176+
final Restricted r = bestMethod.getAnnotation(Restricted.class);
122177
if (r != null) {
123178
attribute.restrictions(r.value());
124179
}
125180

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)) {
181+
Attribute<T, ?> prevAttribute = attributes.get(name);
182+
if (prevAttribute == null) {
129183
attributes.put(name, attribute);
184+
} else {
185+
Class<?> prevType = prevAttribute.type;
186+
Class<?> currentType = attribute.type;
187+
188+
if (prevType.isAssignableFrom(currentType)) {
189+
attributes.put(name, attribute);
190+
}
130191
}
131192
}
132193

@@ -136,14 +197,25 @@ public abstract class BaseConfigurator<T> implements Configurator<T> {
136197
/**
137198
* Check if target class has a Getter method for property s
138199
*/
139-
private boolean hasGetter(Class<T> c, String s) {
140-
List<String> candidates = Arrays.asList("get" + s, "is" + s);
200+
private Method findGetter(Class<T> c, String s) {
201+
String getMethod = "get" + s;
202+
String isMethod = "is" + s;
203+
141204
for (Method m : c.getMethods()) {
142-
if (m.getParameterCount() == 0 && candidates.contains(m.getName())) {
143-
return true;
205+
if (m.getParameterCount() == 0) {
206+
if (m.getName().equals(getMethod)) {
207+
return m;
208+
}
209+
210+
if (m.getName().equals(isMethod)) {
211+
Class<?> returnType = m.getReturnType();
212+
if (returnType == boolean.class || returnType == Boolean.class) {
213+
return m;
214+
}
215+
}
144216
}
145217
}
146-
return false;
218+
return null;
147219
}
148220

149221
/**
@@ -439,9 +511,9 @@ public static final class TypePair {
439511
/**
440512
* Erasure of {@link #type}
441513
*/
442-
final Class rawType;
514+
final Class<?> rawType;
443515

444-
public TypePair(Type type, Class rawType) {
516+
public TypePair(Type type, Class<?> rawType) {
445517
this.rawType = rawType;
446518
this.type = type;
447519
}
@@ -476,4 +548,73 @@ public boolean equals(Object obj) {
476548
public int hashCode() {
477549
return getTarget().hashCode();
478550
}
551+
552+
private Method resolveBestSetter(List<Method> methods, Class<?> getterRawType) {
553+
if (methods.size() == 1) {
554+
return methods.get(0);
555+
}
556+
557+
if (LOGGER.isLoggable(Level.FINE)) {
558+
String methodNames = methods.stream()
559+
.map(m -> m.getParameterCount() > 0 ? m.getParameterTypes()[0].getSimpleName() : "No-Args")
560+
.collect(Collectors.joining(", "));
561+
LOGGER.log(Level.FINE, "Multiple setters found for getter type {0}. Candidates: [{1}]", new Object[] {
562+
getterRawType.getSimpleName(), methodNames
563+
});
564+
}
565+
566+
for (Method m : methods) {
567+
Class<?> currentType = m.getParameterTypes()[0];
568+
if (currentType.equals(getterRawType)) {
569+
return m;
570+
}
571+
}
572+
573+
Method best = null;
574+
Class<?> bestType = null;
575+
576+
for (Method m : methods) {
577+
Class<?> currentType = m.getParameterTypes()[0];
578+
579+
if (best == null) {
580+
best = m;
581+
bestType = currentType;
582+
continue;
583+
}
584+
585+
if (bestType.isAssignableFrom(currentType)) {
586+
best = m;
587+
bestType = currentType;
588+
continue;
589+
} else if (currentType.isAssignableFrom(bestType)) {
590+
continue;
591+
}
592+
593+
boolean currentCompatible = getterRawType.isAssignableFrom(currentType);
594+
boolean bestCompatible = getterRawType.isAssignableFrom(bestType);
595+
596+
if (currentCompatible && !bestCompatible) {
597+
best = m;
598+
bestType = currentType;
599+
} else if (currentCompatible == bestCompatible) {
600+
if (!currentType.isInterface() && bestType.isInterface()) {
601+
best = m;
602+
bestType = currentType;
603+
} else if (!currentType.isInterface() || bestType.isInterface()) {
604+
String previousBestName = bestType.getName();
605+
if (currentType.getName().compareTo(bestType.getName()) < 0) {
606+
best = m;
607+
bestType = currentType;
608+
}
609+
LOGGER.log(
610+
Level.FINER,
611+
"Ambiguous setters for property type {0}: {1} vs {2}. Deterministically chose {3}",
612+
new Object[] {
613+
getterRawType.getName(), previousBestName, currentType.getName(), bestType.getName()
614+
});
615+
}
616+
}
617+
}
618+
return best;
619+
}
479620
}

‎plugin/src/test/java/io/jenkins/plugins/casc/BaseConfiguratorTest.java‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package io.jenkins.plugins.casc;
2+
3+
import static org.junit.Assert.assertEquals;
4+
5+
import io.jenkins.plugins.casc.model.Mapping;
6+
import java.util.Map;
7+
import java.util.Set;
8+
import java.util.stream.Collectors;
9+
import org.junit.Test;
10+
11+
public class BaseConfiguratorTest {
12+
13+
public static class Animal {}
14+
15+
public static class Dog extends Animal {}
16+
17+
public static class Cat extends Animal {}
18+
19+
public static class Vehicle {}
20+
21+
public static class Car extends Vehicle {}
22+
23+
@SuppressWarnings("unused")
24+
public static class DummyTarget {
25+
26+
public String getStandard() {
27+
return null;
28+
}
29+
30+
public void setStandard(String val) {}
31+
32+
public Car getRide() {
33+
return null;
34+
}
35+
36+
public void setRide(Vehicle val) {}
37+
38+
public void setRide(Car val) {}
39+
40+
public void setRide(Object val) {}
41+
42+
public void setRide() {}
43+
44+
public Animal getPet() {
45+
return null;
46+
}
47+
48+
public void setPet(Object pet) {}
49+
50+
public void setPet(Animal pet) {}
51+
52+
public Animal getAmbiguous() {
53+
return null;
54+
}
55+
56+
public void setAmbiguous(Dog dog) {}
57+
58+
public void setAmbiguous(Cat cat) {}
59+
}
60+
61+
public static class DummyConfigurator extends BaseConfigurator<DummyTarget> {
62+
@Override
63+
public Class<DummyTarget> getTarget() {
64+
return DummyTarget.class;
65+
}
66+
67+
@Override
68+
protected DummyTarget instance(Mapping mapping, ConfigurationContext context) {
69+
return new DummyTarget();
70+
}
71+
}
72+
73+
@Test
74+
public void testDescribeResolvesBestSetters() {
75+
DummyConfigurator configurator = new DummyConfigurator();
76+
Set<Attribute<DummyTarget, ?>> attributes = configurator.describe();
77+
78+
Map<String, Class<?>> resolvedAttributes =
79+
attributes.stream().collect(Collectors.toMap(Attribute::getName, attr -> (Class<?>) attr.getType()));
80+
81+
assertEquals("Should discover exactly 4 configurable properties", 4, resolvedAttributes.size());
82+
83+
assertEquals("Standard setter should resolve to String", String.class, resolvedAttributes.get("standard"));
84+
85+
assertEquals(
86+
"Exact match should win over subclasses and superclasses", Car.class, resolvedAttributes.get("ride"));
87+
88+
assertEquals(
89+
"Most specific compatible type should win over Object", Animal.class, resolvedAttributes.get("pet"));
90+
91+
assertEquals(
92+
"Alphabetical fallback should deterministically choose Cat over Dog",
93+
Cat.class,
94+
resolvedAttributes.get("ambiguous"));
95+
96+
assertEquals(Set.of("standard", "ride", "pet", "ambiguous"), resolvedAttributes.keySet());
97+
}
98+
}

0 commit comments

Comments
 (0)