Skip to content

MrBean: Fix detection of inherited default method in Java 8+ interface #109

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

Merged
merged 6 commits into from
Sep 22, 2020
Merged

MrBean: Fix detection of inherited default method in Java 8+ interface #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c18cdeb
Verify default methods in interface are supported
robbytx Sep 3, 2020
286b5d9
Compile test sources against Java 1.8
robbytx Sep 4, 2020
c5176ab
Verify non-public class method behavior
robbytx Sep 3, 2020
92e61bb
Verify current behavior of non-public methods
robbytx Sep 3, 2020
e0bfe3f
Use Class.getMethod to locate the precise implementation, including d…
robbytx Sep 3, 2020
e800726
Add failing test case
robbytx Sep 3, 2020
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
10 changes: 10 additions & 0 deletions mrbean/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ ${project.groupId}.mrbean.*;version=${project.version}
<artifactId>replacer</artifactId>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<!-- Enable testing with Java 8 features, especially default methods in interfaces -->
<testSource>1.8</testSource>
<testTarget>1.8</testTarget>
</configuration>
</plugin>

<plugin>
<!-- We will shade ASM, to simplify deployment, avoid version conflicts -->
<groupId>org.apache.maven.plugins</groupId>
Expand Down
20 changes: 16 additions & 4 deletions mrbean/src/main/java/com/fasterxml/jackson/module/mrbean/BeanBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,16 +181,28 @@ protected boolean hasConcreteOverride(Method m0, JavaType implementedType)
{
final String name = m0.getName();
final Class<?>[] argTypes = m0.getParameterTypes();
try {
// getMethod returns the most-specific method implementation, for public methods only (which is any method in an interface)
Method effectiveMethod = implementedType.getRawClass().getMethod(name, argTypes);
if (BeanUtil.isConcrete(effectiveMethod)) {
Copy link
Member

Choose a reason for hiding this comment

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

Quick question on this one: is the other case (not concrete) not conclusive?
That is, why not just return BeanUtil.isConcrete(effectiveMethod) as is, instead of continuing?
I am assuming here that if method is found, it is the visible declaration.

I suspect most of the time continuing would only be wasteful and is unlikely to give wrong answer....
but I have occasionally used pattern of re-abstract'ing methods (esp. toString() / equals() from Object) to force re-implementing by sub-classes.

Copy link
Contributor Author

@robbytx robbytx Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, that's an interesting thought. Yes... if the method is returned, then I think you have the definitive answer as to whether the method is concrete for the implemented type.

I have occasionally used pattern of re-abstract'ing methods (esp. toString() / equals() from Object) to force re-implementing by sub-classes.

I think if the implemented type returned one of those "re-abstracted" methods, then it would be abstract, and the MrBean-generated class would be required to provide an implementation. I'm not sure how MrBean would handle this, probably it would throw an error, since it's not a property?

So, to clarify what we're talking about, I think it would be correct to simply return BeanUtil.isConcrete(effectiveMethod) here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool.

return true;
}
} catch (NoSuchMethodException e) {
// method must be non-public, fallback to using getDeclaredMethod
}

for (JavaType curr = implementedType; (curr != null) && !curr.isJavaLangObject();
curr = curr.getSuperClass()) {
// 29-Nov-2015, tatu: Avoiding exceptions would be good, so would linear scan
// be better here?
try {
Method effectiveMethod = curr.getRawClass().getDeclaredMethod(name, argTypes);
if (effectiveMethod != null && BeanUtil.isConcrete(effectiveMethod)) {
if (BeanUtil.isConcrete(effectiveMethod)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently any "re-abstracted" methods are skipped over by this check. If the concrete method exists on a class other than java.lang.Object, then I think MrBean will find it, and return true, which may be incorrect.

I'll write a test case to confirm this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Much appreciated! It's good that we can keep modules up to date with "new" versions of JDK (Java 8 isn't that new of course, but it wasn't around when Mr Bean was written and still isn't the baseline).

return true;
}
} catch (NoSuchMethodException e) { }
} catch (NoSuchMethodException e) {
// method must exist on a superclass, continue searching...
}
}
return false;
}
Expand Down Expand Up @@ -245,7 +257,7 @@ protected final static boolean returnsBoolean(Method m)
Class<?> rt = m.getReturnType();
return (rt == Boolean.class || rt == Boolean.TYPE);
}

/*
/**********************************************************
/* Internal methods, bytecode generation
Expand Down Expand Up @@ -354,7 +366,7 @@ protected void createUnimplementedMethod(ClassWriter cw, String internalClassNam
/* Internal methods, other
/**********************************************************
*/

protected String decap(String name) {
char c = name.charAt(0);
if (name.length() > 1
Expand Down
10 changes: 8 additions & 2 deletions mrbean/src/test/java/com/fasterxml/jackson/module/mrbean/TestAbstractClasses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ public abstract static class Bean
int y;

protected Bean() { }

public abstract String getX();

public String getFoo() { return "Foo!"; }
public void setY(int value) { y = value; }

// also verify non-public methods
protected abstract String getZ();
private String customMethod() { return "Private methods rock!"; }
}

/*
Expand All @@ -32,10 +36,12 @@ protected Bean() { }
public void testSimpleInteface() throws Exception
{
ObjectMapper mapper = newMrBeanMapper();
Bean bean = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13 }", Bean.class);
Bean bean = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13, \"z\" : \"def\" }", Bean.class);
assertNotNull(bean);
assertEquals("abc", bean.getX());
assertEquals(13, bean.y);
assertEquals("Foo!", bean.getFoo());
assertEquals("def", bean.getZ());
assertEquals("Private methods rock!", bean.customMethod());
}
}
17 changes: 14 additions & 3 deletions ...n/src/test/java/com/fasterxml/jackson/module/mrbean/TestAbstractClassesWithOverrides.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,28 @@ public abstract static class Bean
int y;

protected Bean() { }

public abstract String getX();

public abstract String roast(int temperature);

public String getFoo() { return "Foo!"; }
public void setY(int value) { y = value; }

// also verify non-public methods
protected abstract String getZ();
private Object customMethod() { return protectedAbstractMethod(); }
protected abstract Object protectedAbstractMethod();
}

public abstract static class CoffeeBean extends Bean {
@Override public String roast(int temperature) {
return "The coffee beans are roasting at " + temperature + " degrees now, yummy";
}

@Override protected Object protectedAbstractMethod() {
return "Private methods invoking protected abstract methods is the bomb!";
}
}

public abstract static class PeruvianCoffeeBean extends CoffeeBean {}
Expand All @@ -45,10 +54,10 @@ public void testOverrides() throws Exception
ObjectMapper mapper = new ObjectMapper();
mapper.registerModule(new MrBeanModule());

Bean bean = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13 }", CoffeeBean.class);
Bean bean = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13, \"z\" : \"def\" }", CoffeeBean.class);
verifyBean(bean);

Bean bean2 = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13 }", PeruvianCoffeeBean.class);
Bean bean2 = mapper.readValue("{ \"x\" : \"abc\", \"y\" : 13, \"z\" : \"def\" }", PeruvianCoffeeBean.class);
verifyBean(bean2);
}

Expand All @@ -57,6 +66,8 @@ private void verifyBean(Bean bean) {
assertEquals("abc", bean.getX());
assertEquals(13, bean.y);
assertEquals("Foo!", bean.getFoo());
assertEquals("def", bean.getZ());
assertEquals("The coffee beans are roasting at 123 degrees now, yummy", bean.roast(123));
assertEquals("Private methods invoking protected abstract methods is the bomb!", bean.customMethod());
}
}
50 changes: 42 additions & 8 deletions ...n/src/test/java/com/fasterxml/jackson/module/mrbean/TestSimpleMaterializedInterfaces.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ public interface BeanWithY extends Bean
{
public int getY();
}

public interface PartialBean {
public boolean isOk();
// and then non-getter/setter one:
public int foobar();
}

public interface BeanHolder {
public Bean getBean();
}
Expand All @@ -53,7 +53,21 @@ public interface ArrayBean {
interface NonPublicBean {
public abstract int getX();
}


public interface OtherInterface {
public boolean anyValuePresent();
}

public interface BeanWithDefaultForOtherInterface extends Bean, OtherInterface {
public default boolean anyValuePresent() {
return getX() > 0 || getA() != null;
}
}

public interface BeanWithInheritedDefault extends BeanWithDefaultForOtherInterface {
// in this interface, anyValuePresent() is an inherited (rather than declared) concrete method
}

/*
/**********************************************************
/* Unit tests, low level
Expand Down Expand Up @@ -149,8 +163,8 @@ public void testBeanHolder() throws Exception
assertNotNull(bean);
assertEquals("b", bean.getA());
assertEquals(-4, bean.getX());
}
}

public void testArrayInterface() throws Exception
{
ObjectMapper mapper = new ObjectMapper();
Expand All @@ -172,7 +186,27 @@ public void testSubInterface() throws Exception
assertEquals(1, bean.getX());
assertEquals(2, bean.getY());
}


public void testDefaultMethodInInterface() throws Exception
{
ObjectMapper mapper = newMrBeanMapper();
BeanWithDefaultForOtherInterface bean = mapper.readValue("{\"a\":\"value\",\"x\":123 }", BeanWithDefaultForOtherInterface.class);
assertNotNull(bean);
assertEquals("value", bean.getA());
assertEquals(123, bean.getX());
assertTrue(bean.anyValuePresent());
}

public void testInheritedDefaultMethodInInterface() throws Exception
{
ObjectMapper mapper = newMrBeanMapper();
BeanWithInheritedDefault bean = mapper.readValue("{\"a\":\"value\",\"x\":123 }", BeanWithInheritedDefault.class);
assertNotNull(bean);
assertEquals("value", bean.getA());
assertEquals(123, bean.getX());
assertTrue(bean.anyValuePresent());
}

/*
/**********************************************************
/* Unit tests, higher level, error handling
Expand Down Expand Up @@ -212,6 +246,6 @@ public void testNonPublic() throws Exception
} catch (JsonMappingException e) {
verifyException(e, "is not public");
}
}
}

}