-
Notifications
You must be signed in to change notification settings - Fork 103
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.
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
[JENKINS-39414] - Stapler 1.246 Binary Incompatibility. Part 2 #88
Changes from all commits
f7f502d
9e6f320
f5cf094
2141288
92c4f92
7873e47
51ef1ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,10 @@ | |
import java.net.URL; | ||
import java.util.AbstractList; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import javax.annotation.Nonnull; | ||
|
||
/** | ||
* Strategy pattern to provide navigation across class-like objects in other languages of JVM. | ||
|
@@ -78,15 +80,28 @@ public abstract class KlassNavigator<C> { | |
|
||
/** | ||
* List fields of this class. | ||
* | ||
* This list excludes fields from super classes. | ||
* @param clazz Class | ||
* @return List of the fields declared for the class. | ||
* By default this list is empty, {@link KlassNavigator} implementations are responsible to implement it. | ||
* @since 1.246 | ||
*/ | ||
public abstract List<FieldRef> getDeclaredFields(C clazz); | ||
@Nonnull | ||
public List<FieldRef> getDeclaredFields(C clazz) { | ||
return Collections.emptyList(); | ||
} | ||
|
||
/** | ||
* Reports all the methods that can be used for routing requests on this class. | ||
* @param clazz Class | ||
* @return List of the fields functions declared for the class. | ||
* By default this list is empty, {@link KlassNavigator} implementations are responsible to implement it. | ||
* @since 1.246 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I decided not to touch that code. Ruby Runtime has a dep on a greater version |
||
*/ | ||
public abstract List<Function> getFunctions(C clazz); | ||
@Nonnull | ||
public List<Function> getFunctions(C clazz) { | ||
return Collections.emptyList(); | ||
} | ||
|
||
/** | ||
* If the given type is an array that supports index retrieval. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import java.lang.reflect.InvocationTargetException; | ||
import java.lang.reflect.Method; | ||
import java.lang.reflect.Modifier; | ||
import javax.annotation.CheckForNull; | ||
|
||
/** | ||
* @author Kohsuke Kawaguchi | ||
|
@@ -16,6 +17,18 @@ public abstract class MethodRef extends AnnotatedRef { | |
public boolean isRoutable() { | ||
return true; | ||
} | ||
|
||
/** | ||
* Retrieves the referenced method name. | ||
* Some implementations (e.g. Ruby) cannot guarantee availability of names for all cases, | ||
* so sometimes the name may be missing. | ||
* @return Method name. {@code null} if it cannot be determined. | ||
* @since 1.248 | ||
*/ | ||
@CheckForNull | ||
public String getName() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a proposed caller for this, or is it just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this API is generally useful, e.g. for diagnostic purposes. Right now just in |
||
return null; | ||
} | ||
|
||
public abstract Object invoke(Object _this, Object... args) throws InvocationTargetException, IllegalAccessException; | ||
|
||
|
@@ -33,7 +46,12 @@ public boolean isRoutable() { | |
if (m.isBridge()) return false; | ||
return (m.getModifiers() & Modifier.PUBLIC)!=0; | ||
} | ||
|
||
|
||
@Override | ||
public String getName() { | ||
return m.getName(); | ||
} | ||
|
||
@Override | ||
public Object invoke(Object _this, Object... args) throws InvocationTargetException, IllegalAccessException { | ||
return m.invoke(_this,args); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,22 +16,59 @@ | |
|
||
import static org.hamcrest.CoreMatchers.*; | ||
import org.hamcrest.collection.IsEmptyCollection; | ||
import org.jruby.RubyFixnum; | ||
import org.jruby.RubyInteger; | ||
import org.jruby.internal.runtime.methods.CallConfiguration; | ||
import org.jruby.internal.runtime.methods.DynamicMethod; | ||
import org.jruby.runtime.Block; | ||
import org.jruby.runtime.ThreadContext; | ||
import org.jruby.runtime.Visibility; | ||
import org.jruby.runtime.builtin.IRubyObject; | ||
import static org.junit.Assume.assumeThat; | ||
import org.kohsuke.stapler.Function; | ||
import org.kohsuke.stapler.lang.MethodRef; | ||
|
||
/** | ||
* Tests handling of Ruby classes and modules with {@link RubyKlassNavigator}. | ||
* @author Oleg Nenashev. | ||
*/ | ||
public class RubyKlassNavigatorTest { | ||
|
||
/** | ||
* Verifies that declared method retrieval works well. | ||
* Effective use-case - Ruby Runtime Plugin for Jenkins. | ||
* @throws Exception Test failure | ||
*/ | ||
@Test | ||
public void shouldProperlyHandleDeclaredMethodsInRubyModules() throws Exception { | ||
final ScriptingContainer ruby = createRubyInstance(); | ||
final RubyKlassNavigator navigator = new RubyKlassNavigator(ruby.getProvider().getRuntime(), ClassLoader.getSystemClassLoader()); | ||
final MyRubyModule myModule = new MyRubyModule(ruby.getRuntime(), new MyRubyClass(ruby.getRuntime()), true); | ||
|
||
|
||
final Klass<RubyModule> classInstance = new Klass<RubyModule>(myModule, navigator); | ||
|
||
final List<MethodRef> declaredMethods = classInstance.getDeclaredMethods(); | ||
for (MethodRef ref : declaredMethods) { | ||
if (ref instanceof RubyMethodRef) { | ||
// Ruby engine API allows creating methods with null names, not our bug BTW... | ||
if ("doDynamic".equals(ref.getName())) { | ||
//TODO: More consistency checks | ||
return; | ||
} | ||
} | ||
} | ||
Assert.fail("Have not found 'doDynamic' in the returned function list"); | ||
} | ||
|
||
/** | ||
* Verifies that field retrieval do not fail horribly for {@link RubyModule}. | ||
* Effective use-case - Ruby Runtime Plugin for Jenkins. | ||
* @throws Exception Test failure | ||
*/ | ||
@Test | ||
@Issue("JENKINS-39414") | ||
public void shouldProperlyHandleRubyModules() throws Exception { | ||
public void shouldProperlyHandleFieldsInRubyModules() throws Exception { | ||
final ScriptingContainer ruby = createRubyInstance(); | ||
final RubyKlassNavigator navigator = new RubyKlassNavigator(ruby.getProvider().getRuntime(), ClassLoader.getSystemClassLoader()); | ||
final MyRubyModule myModule = new MyRubyModule(ruby.getRuntime(), new MyRubyClass(ruby.getRuntime()), true); | ||
|
@@ -51,19 +88,66 @@ public void shouldProperlyHandleRubyModules() throws Exception { | |
Assert.fail("Have not found 'fooField' in the returned field list"); | ||
} | ||
|
||
private static final class MyRubyModule extends RubyModule { | ||
//TODO: fix the test when Ruby routing gets implemented (https://github.com/stapler/stapler/issues/87) | ||
/** | ||
* Verifies that function retrieval do not fail horribly for {@link RubyModule}. | ||
* Effective use-case - Ruby Runtime Plugin for Jenkins. | ||
* @throws Exception Test failure | ||
*/ | ||
@Test | ||
@Issue("JENKINS-39414") | ||
public void shouldProperlyHandleFunctionsInRubyModules() throws Exception { | ||
final ScriptingContainer ruby = createRubyInstance(); | ||
final RubyKlassNavigator navigator = new RubyKlassNavigator(ruby.getProvider().getRuntime(), ClassLoader.getSystemClassLoader()); | ||
final MyRubyModule myModule = new MyRubyModule(ruby.getRuntime(), new MyRubyClass(ruby.getRuntime()), true); | ||
|
||
private int fooField = 123; | ||
|
||
final Klass<RubyModule> classInstance = new Klass<RubyModule>(myModule, navigator); | ||
|
||
final List<Function> declaredFunctions = classInstance.getFunctions(); | ||
for (Function ref : declaredFunctions) { | ||
if ("doDynamic".equals(ref.getName())) { | ||
//TODO: check fields once implemented in Stapler | ||
return; | ||
} | ||
} | ||
|
||
assumeThat("Routing of declared routable methods is not fully implemented (See https://github.com/stapler/stapler/issues/87)", | ||
ruby, not(anything("Nothing to do in this code"))); | ||
Assert.fail("Have not found 'doDynamic' in the returned function list"); | ||
} | ||
|
||
private static final class MyRubyModule extends RubyModule { | ||
|
||
private int fooField = 123; | ||
|
||
MyRubyModule(Ruby runtime, RubyClass metaClass, boolean objectSpace) { | ||
super(runtime, metaClass, objectSpace); | ||
|
||
// Register a Mock method for the class | ||
getMethodsForWrite().put("doDynamic", new DynamicMethod(this, Visibility.PUBLIC, CallConfiguration.FrameFullScopeFull, "doDynamic") { | ||
@Override | ||
public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) { | ||
return doDynamic("foo"); | ||
} | ||
|
||
@Override | ||
public DynamicMethod dup() { | ||
throw new UnsupportedOperationException("Not supported yet."); | ||
} | ||
}); | ||
} | ||
|
||
public final IRubyObject doDynamic(String token) { | ||
// Just return a routable object | ||
return new RubyFixnum(getRuntime(), 0); | ||
} | ||
} | ||
|
||
private static final class MyRubyClass extends RubyClass { | ||
private static final class MyRubyClass extends RubyClass { | ||
MyRubyClass(Ruby runtime) { | ||
super(runtime); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gratuitous whitespace changes. |
||
} | ||
|
||
private ScriptingContainer createRubyInstance() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we need to test this change. You removed the try catch, but are sure that getDeclaredFields never throws an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a hack in #85 in order to suppress the binary compatibility issue. Now we solve it by providing default implementations in
KlassNavigator
. Hence thisAbstractMethodError
handler is no longer required