Skip to content

[JENKINS-53825] Choose from ambiguous class names via a simple heuristic #40

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

Open
abayer wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[JENKINS-53825] Choose from ambiguous class names via a simple heuristic #40

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
f118413
[JENKINS-53825] Choose from ambiguous class names via a simple heuristic
abayer Oct 3, 2018
5b54f48
Ok, this actually makes the error message order deterministic
abayer Oct 3, 2018
fe96e5f
Initial work on adding "context" to Symbol.
abayer Oct 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions annotation/src/main/java/org/jenkinsci/Symbol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,5 @@
@Documented
public @interface Symbol {
String[] value();
Class<?>[] context() default {};
Copy link
Member

@stephenc stephenc Oct 4, 2018

Choose a reason for hiding this comment

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

I think I like this idea.

Would help to express your plan in the javadoc for this.

I think what you want is that where there are multiple options with the same name, if there is one with the "closest" context, then that one will win.

TBH I like that, because we can do something like @Symbol(value="discoverOriginPRs", context={GitHubSCMSource.class,GitHubSCMNavigator.class}) and we get the win.

The problem, as I see it, is in deciding which is the nearer context if the context is not an exact match.

i.e. If the context is an interface or a parent class.

You'll need to clearly spell out how you decide the nearest context... and if multiple symbols are the same name at the same distance, we fall back to ambiguous

Copy link
Member

@stephenc stephenc Oct 4, 2018

Choose a reason for hiding this comment

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

My original idea was to just have an extension point that the symbol library could consult in the case where it had conflicts.

The extension point would basically be given a list of candidate symbols and the context in which these were requested and remove any that it wants from the list.

We then walk all implementations of the extension in order. If the list is empty, we stop and say "no match". If we get to the end and there is more than one, we stop and say "ambiguous" and if we have exactly one "winner"

And we need to process the list of extensions all the way to the end, as we don't know if a later impl might remove the one option we are left with bringing us to an empty list

Copy link
Member

@stephenc stephenc Oct 4, 2018

Choose a reason for hiding this comment

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

But with this... while requiring changes on the implementation plugins... we get a less "magic" solution... so as long as you define what is "closest" I think this will work really well

}
95 changes: 79 additions & 16 deletions plugin/src/main/java/org/jenkinsci/plugins/structs/SymbolLookup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import javax.annotation.Nonnull;
import javax.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
Expand All @@ -33,9 +35,9 @@
*/
@Extension
public class SymbolLookup {
private final ConcurrentMap<Key,Object> cache = new ConcurrentHashMap<Key, Object>();
private final ConcurrentMap<Key, Object> cache = new ConcurrentHashMap<Key, Object>();

private final ConcurrentMap<Key,Object> noHitCache = new ConcurrentHashMap<Key, Object>();
private final ConcurrentMap<Key, Object> noHitCache = new ConcurrentHashMap<Key, Object>();

static final Object NO_HIT = new Object();

Expand All @@ -55,7 +57,8 @@ private static HashSet<String> pluginsToNames(List<PluginWrapper> plugins) {
return pluginNames;
}

/** Update list of plugins used and purge the noHit cache if plugins have been added
/**
* Update list of plugins used and purge the noHit cache if plugins have been added
*/
private synchronized void checkPluginsForChangeAndRefresh() {
List<PluginWrapper> wrap = pluginManager.getPlugins();
Expand All @@ -69,14 +72,22 @@ private synchronized void checkPluginsForChangeAndRefresh() {
}

/**
* @param type
* Restrict the search to a subset of extensions.
* @param type Restrict the search to a subset of extensions.
*/
public <T> T find(Class<T> type, String symbol) {
return find(type, symbol, null);
}

/**
* @param type Restrict the search to a subset of extensions.
* @param context
* Prefer classes with a matching context on their {@link Symbol}
*/
public <T> T find(Class<T> type, String symbol, Class<?> context) {
try {
Key k = new Key("find",type,symbol);
Key k = new Key("find", type, symbol, context);
Object i = cache.get(k);
if (i!=null) return type.cast(i);
if (i != null) return type.cast(i);

// not allowing @Symbol to use an invalid identifier.
// TODO: compile time check
Expand All @@ -90,25 +101,42 @@ public <T> T find(Class<T> type, String symbol) {
return null;
}

List<Class<?>> candidates = new ArrayList<>();
for (Class<?> e : Index.list(Symbol.class, pluginManager.uberClassLoader, Class.class)) {
if (type.isAssignableFrom(e)) {
Symbol s = e.getAnnotation(Symbol.class);
if (s != null) {
for (String t : s.value()) {
if (t.equals(symbol)) {
i = jenkins.getInjector().getInstance(e);
cache.put(k, i);
return type.cast(i);
candidates.add(e);
}
}
}
}
}

if (!candidates.isEmpty()) {
for (Class<?> e : candidates) {
Symbol s = e.getAnnotation(Symbol.class);
if (context != null && Arrays.stream(s.context()).anyMatch(c -> context.isAssignableFrom(c))) {
i = jenkins.getInjector().getInstance(e);
break;
}
}

if (i == null) {
// If we didn't find a context-aware match, just use the first general match.
i = jenkins.getInjector().getInstance(candidates.get(0));
}

cache.put(k, i);
return type.cast(i);
}

noHitCache.put(k, NO_HIT);
return null;
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Unable to find @Symbol",e);
LOGGER.log(Level.WARNING, "Unable to find @Symbol", e);
return null;
}
}
Expand All @@ -120,8 +148,20 @@ public <T> T find(Class<T> type, String symbol) {
* Restrict the search to a subset of {@link Describable}
*/
public Descriptor<?> findDescriptor(Class<?> type, String symbol) {
return findDescriptor(type, symbol, null);
}

/**
* Looks for a {@link Descriptor} that has the given symbol
*
* @param type
* Restrict the search to a subset of {@link Describable}
* @param context
* Prefer classes with a matching context on their {@link Symbol}
*/
public Descriptor<?> findDescriptor(Class<?> type, String symbol, Class<?> context) {
try {
Key k = new Key("findDescriptor",type,symbol);
Key k = new Key("findDescriptor",type,symbol, context);
Object i = cache.get(k);
if (i!=null) return (Descriptor)i;

Expand All @@ -137,6 +177,7 @@ public Descriptor<?> findDescriptor(Class<?> type, String symbol) {
return null;
}

List<Class<?>> candidates = new ArrayList<>();
for (Class<?> e : Index.list(Symbol.class, pluginManager.uberClassLoader, Class.class)) {
if (Descriptor.class.isAssignableFrom(e)) {
Symbol s = e.getAnnotation(Symbol.class);
Expand All @@ -145,15 +186,33 @@ public Descriptor<?> findDescriptor(Class<?> type, String symbol) {
if (t.equals(symbol)) {
Descriptor d = (Descriptor) jenkins.getInjector().getInstance(e);
if (type.isAssignableFrom(d.clazz)) {
cache.put(k, d);
return d;
candidates.add(e);
}
}
}
}
}
}

Descriptor descriptor = null;

if (!candidates.isEmpty()) {
for (Class<?> e : candidates) {
Symbol s = e.getAnnotation(Symbol.class);
if (context != null && Arrays.stream(s.context()).anyMatch(c -> context.isAssignableFrom(c))) {
descriptor = (Descriptor) jenkins.getInjector().getInstance(e);
break;
}
}

if (descriptor == null) {
descriptor = (Descriptor) jenkins.getInjector().getInstance(candidates.get(0));
}

cache.put(k, descriptor);
return descriptor;
}

noHitCache.put(k, NO_HIT);
return null;
} catch (IOException e) {
Expand All @@ -166,26 +225,30 @@ private static class Key {
private final String tag;
private final Class type;
private final String name;
private final Class<?> context;

public Key(String tag, Class type, String name) {
public Key(String tag, Class type, String name, Class<?> context) {
this.tag = tag;
this.type = type;
this.name = name;
this.context = context;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Key key = (Key) o;
return type==key.type && tag.equals(key.tag) && name.equals(key.name);
return type==key.type && tag.equals(key.tag) && name.equals(key.name)
&& (context != null ? context.equals(key.context) : key.context == null);
}

@Override
public int hashCode() {
int h = type.hashCode();
h = h*31 + tag.hashCode();
h = h*31 + name.hashCode();
h = h*31 + (context != null ? context.hashCode() : 0);
return h;
}
}
Expand Down
50 changes: 39 additions & 11 deletions plugin/src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.ClassUtils;
import org.apache.commons.lang.ObjectUtils;
import org.apache.commons.lang.StringUtils;
import org.codehaus.groovy.reflection.ReflectionCache;
import org.jenkinsci.Symbol;
import org.jenkinsci.plugins.structs.SymbolLookup;
Expand Down Expand Up @@ -50,6 +51,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;

import static org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable.*;

Expand Down Expand Up @@ -399,7 +401,7 @@ private Object coerce(String context, Type type, Object o) throws Exception {
m.put((String) entry.getKey(), entry.getValue());
}

Class<?> clazz = resolveClass(erased, (String) m.remove(CLAZZ), null);
Class<?> clazz = resolveClass(erased, (String) m.remove(CLAZZ), null, getType());
return new DescribableModel(clazz).instantiate(m);
} else if (o instanceof String && erased.isEnum()) {
return Enum.valueOf(erased.asSubclass(Enum.class), (String) o);
Expand Down Expand Up @@ -456,7 +458,8 @@ private Object coerceStringToNumber(@Nonnull String context, @Nonnull Class numb
* @param base
* Signature of the type that the resolved class should be assignable to.
*/
/*package*/ static Class<?> resolveClass(Class<?> base, @Nullable String name, @Nullable String symbol) throws ClassNotFoundException {
/*package*/ static Class<?> resolveClass(Class<?> base, @Nullable String name, @Nullable String symbol,
@Nullable Class<?> contextClass) throws ClassNotFoundException {
// TODO: if both name & symbol are present, should we verify its consistency?

if (name != null) {
Expand All @@ -465,30 +468,55 @@ private Object coerceStringToNumber(@Nonnull String context, @Nonnull Class numb
ClassLoader loader = j != null ? j.getPluginManager().uberClassLoader : Thread.currentThread().getContextClassLoader();
return Class.forName(name, true, loader);
} else {
Class<?> clazz = null;
List<Class<?>> possibleClazzes = new ArrayList<>();
for (Class<?> c : findSubtypes(base)) {
if (c.getSimpleName().equals(name)) {
if (clazz != null) {
throw new UnsupportedOperationException(name + " as a " + base + " could mean either " + clazz.getName() + " or " + c.getName());
}
clazz = c;
possibleClazzes.add(c);
}
}
if (clazz == null) {
if (possibleClazzes.isEmpty()) {
throw new UnsupportedOperationException("no known implementation of " + base + " is named " + name);
}
return clazz;
if (possibleClazzes.size() != 1) {
// Try to heuristically determine the correct class.
List<Class<?>> narrowedClazzes = new ArrayList<>();
for (Class<?> possible : possibleClazzes) {
if (contextClass != null) {
if (contextClass.equals(possible.getEnclosingClass())
|| contextClass.getPackage().equals(possible.getPackage())
|| possible.getPackage().getName().startsWith(contextClass.getPackage().getName())) {
narrowedClazzes.add(possible);
}
}
}

// We found just one that was eligible, return that.
if (narrowedClazzes.size() == 1) {
return narrowedClazzes.get(0);
}

// Couldn't heuristically determine the correct class, error out.
String errorString;
List<String> ambiguousNames = possibleClazzes.stream().map(Class::getName).sorted().collect(Collectors.toList());
if (ambiguousNames.size() == 2) {
errorString = StringUtils.join(ambiguousNames, " or ");
} else {
errorString = StringUtils.join(ambiguousNames, ", ");
}
throw new UnsupportedOperationException(name + " as a " + base + " could mean any of " + errorString);
}
return possibleClazzes.get(0);
}
}

if (symbol != null) {
// The normal case: the Descriptor is marked, but the name applies to its Describable.
Descriptor d = SymbolLookup.get().findDescriptor(base, symbol);
Descriptor d = SymbolLookup.get().findDescriptor(base, symbol, contextClass);
if (d != null) {
return d.clazz;
}
if (base == ParameterValue.class) { // TODO JENKINS-26093 workaround
d = SymbolLookup.get().findDescriptor(ParameterDefinition.class, symbol);
d = SymbolLookup.get().findDescriptor(ParameterDefinition.class, symbol, contextClass);
if (d != null) {
Class<?> c = parameterValueClass(d.clazz);
if (c != null) {
Expand Down
2 changes: 1 addition & 1 deletion ...in/src/main/java/org/jenkinsci/plugins/structs/describable/UninstantiatedDescribable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public Object instantiate() throws Exception {
* depends on this parameter.
*/
public <T> T instantiate(Class<T> base) throws Exception {
Class<?> c = DescribableModel.resolveClass(base, klass, symbol);
Class<?> c = DescribableModel.resolveClass(base, klass, symbol, null);
return base.cast(new DescribableModel(c).instantiate(arguments));
}

Expand Down
37 changes: 37 additions & 0 deletions plugin/src/test/java/org/jenkinsci/plugins/structs/SymbolLookupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ public static class Foo {}
@TestExtension @Symbol("bar")
public static class Bar {}

@TestExtension @Symbol("ambiguous")
public static class FirstAmbiguous {}

@TestExtension @Symbol(value = "ambiguous", context = Bar.class)
public static class SecondAmbiguous {}

@Rule
public JenkinsRule rule = new JenkinsRule();

Expand All @@ -35,6 +41,12 @@ public static class Bar {}
@Inject
Bar bar;

@Inject
FirstAmbiguous firstAmbiguous;

@Inject
SecondAmbiguous secondAmbiguous;

@Inject
FishingNet.DescriptorImpl fishingNetDescriptor;

Expand Down Expand Up @@ -104,4 +116,29 @@ public void descriptorIsDescribable() {
@Symbol("whatever")
public static class SomeConfiguration extends GlobalConfiguration {}

@Issue("JENKINS-53825")
@Test
public void context() {
assertThat(lookup.find(Object.class, "ambiguous"), is(sameInstance(this.firstAmbiguous)));
assertThat(lookup.find(Object.class, "ambiguous", Bar.class), is(sameInstance(this.secondAmbiguous)));
}

@TestExtension("descriptorContext")
@Symbol("ambiguousDescriptor")
public static class FirstAmbiguousConfiguration extends GlobalConfiguration {}

@TestExtension("descriptorContext")
@Symbol(value = "ambiguousDescriptor", context = Bar.class)
public static class SecondAmbiguousConfiguration extends GlobalConfiguration {}

@Issue("JENKINS-53825")
@Test
public void descriptorContext() {
FirstAmbiguousConfiguration first = rule.jenkins.getDescriptorByType(FirstAmbiguousConfiguration.class);
SecondAmbiguousConfiguration second = rule.jenkins.getDescriptorByType(SecondAmbiguousConfiguration.class);

assertThat(lookup.find(Object.class, "ambiguousDescriptor"), is(sameInstance(first)));
assertThat(lookup.find(Object.class, "ambiguousDescriptor", Bar.class), is(sameInstance(second)));
}

}
Loading