Skip to content

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

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
498cac3
Verify default methods in interface are supported
robbytx Sep 3, 2020
22b8550
Add failing test case
robbytx Sep 3, 2020
66df757
Verify non-public class method behavior
robbytx Sep 3, 2020
f8a9a0d
Verify current behavior of non-public methods
robbytx Sep 3, 2020
73b8f0c
Use Class.getMethod to locate the precise implementation, including d…
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
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 @@ -176,16 +176,28 @@ protected boolean hasConcreteOverride(Method m0, JavaType implementedType)
{
final String name = m0.getName();
final Class<?>[] argTypes = m0.getParameterTypes();
try {
Copy link
Member

Choose a reason for hiding this comment

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

I guess I am specifically trying to understand this part here; why is the extra check needed?
Is that because existing code seems to only look for supertypes (and not type itself) or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit convoluted (and took me some time to spot the issue), so let me illustrate with the BeanWithInheritedDefault example from the unit test:

In BeanBuilder.implement,ArrayList<JavaType> implTypes contains [BeanWithInheritedDefault, BeanWithDefaultForOtherInterface, Bean, OtherInterface], and these reduce two relevant executions of the loop with Method m:

  1. BeanWithDefaultForOtherInterface.anyValuePresent, which is concrete itself, and is properly skipped at line 108
  2. OtherInterface.anyValuePresent, which is not, and therefore which calls the hasConcreteOverride method I've modified.

In the original implementation of hasConcreteOverride, it checks for a concrete declared method on the implementedType (in this case BeanWithInheritedDefault, which doesn't have it), and then it checks on superclasses of which there is none. Critically, this code fails to locate the concrete implementation of this method in BeanWithDefaultForOtherInterface, and therefore it incorrectly return false from the method.

I started to explore solutions that would either (a) traverse the interface hierarchy, or (b) optimize the loop in implement to track known-concrete methods, but I ultimately realized this simple addition to hasConcreteOverride solves the search problem for all public methods, which coincidentally applies to all Java interfaces and their default methods. I think it also makes the hasConcreteOverride more efficient, since it is only necessary to search the superclass hierarchy when the method is in question is non-public.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. So the logic was ok pre-Java8, but the introduction of interface default implementations threw the monkey wrench... makes sense.

// 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)) {
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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getDeclaredMethod (like getMethod) never returns null

if (BeanUtil.isConcrete(effectiveMethod)) {
return true;
}
} catch (NoSuchMethodException e) { }
} catch (NoSuchMethodException e) {
// method must exist on a superclass, continue searching...
}
}
return false;
}
Expand Down Expand Up @@ -240,7 +252,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 @@ -288,7 +300,7 @@ private DynamicType.Builder<?> createSetter(DynamicType.Builder<?> builder,
/* 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();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we are expected to implement non-public getters... although probably makes sense (setters def. need to be implemented).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure whether non-public methods were supported until I wrote the test, which I decided to do after exploring the differences between getMethod and getDeclaredMethod. Once I realized that non-public methods were working the same as public ones, I figured it was worthwhile to create a test to verify that behavior wasn't regressed, per Hyrum's law.

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 @@ -43,9 +52,9 @@ public abstract static class PeruvianCoffeeBean extends CoffeeBean {}
public void testOverrides() throws Exception
{
ObjectMapper mapper = newMrBeanMapper();
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 @@ -54,6 +63,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 @@ -28,13 +28,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 @@ -55,7 +55,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 = newMrBeanMapper();
Expand All @@ -170,7 +184,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 @@ -210,6 +244,6 @@ public void testNonPublic() throws Exception
} catch (JsonMappingException e) {
verifyException(e, "is not public");
}
}
}

}