OgnlRuntime#getReadMethod() returns null if the method is a bridge method

[harawata]

Considering the following two classes, OgnlRuntime.getReadMethod(PublicChild.class, "name") should return the getName() method, but it returns null instead.

package pkg1;

class PkgPrivateParent {
  public String getName() {
    return "";
  }
}

package pkg1;

public class PublicChild extends PkgPrivateParent {
}

In the getReadMethod(), candidate methods are checked by isMethodCallable() which returns false for a bridge method even though it is callable.
I simply removed the 'is callable' check from getReadMethod() and there is no broken test.

p.s.
Note that this bug does not exists in OgnlRuntime#getWriteMethod() because it internally uses BeanInfo#getMethodDescriptors()[i]#getMethod()#getModifiers() which returns a different value than Class#getMethods()[i]#getModifiers() for some reason [1].
I added a test case to avoid possible regression in case someone wants to resolve the implementation inconsistency in future.

[1] Try the following code if you are curious.

protected static class ProtectedParent {
    private String name;
    public void setName(String name) {
        this.name = name;
    }
    public String getName() {
        return name;
    }
}

public static class PublicChild extends ProtectedParent {
}

public void testSyntheticReadMethod() throws Exception {
    Method[] methods = PublicChild.class.getMethods();
    for (int i = 0; i < methods.length; i++) {
        if (methods[i].getName().endsWith("Name")) {
            System.out.println(methods[i].getModifiers()); // -> 4161
        }
    }
    BeanInfo beanInfo = Introspector.getBeanInfo(PublicChild.class);
    MethodDescriptor[] methodDescriptors = beanInfo.getMethodDescriptors();
    for (int i = 0; i < methodDescriptors.length; i++) {
        if (methodDescriptors[i].getMethod().getName().endsWith("Name")) {
            System.out.println(methodDescriptors[i].getMethod().getModifiers()); // -> 1
        }
    }
}

[lukaszlenart]

LGTM 👍

[JCgH4164838Gh792C124B5]

Hi @harawata and @lukaszlenart .

Is it possible that removing isMethodCallable() from getReadMethod() may cause OGNL 3.2.x to find/prefer a different "getter" than it did previously (before this change) ?

I am wondering if the scenario @harawata found maybe calls for isMethodCallable() to be modified, or maybe a new isCallableBridgeMethod() to be introduced, assuming logic to properly detect a callable bridge method can be found (and performs adequately) ?

The behaviour for synthetic read and write methods may have some weird corner-cases. When I run a build of ognl-3.2.16-SNAPSHOT using JDK 8 or 11, the build completes just fine. However, running the same build with JDK 7 causes the testSyntheticWriteMethod() test to fail every time. The getModifiers() behaviour (Class and/or BeanInfo) might be different across different JDK versions, if the JDK7 build failure happens for others too (and not just my build environment).

If there is something either of you would like me to try out, please let me know.

[harawata]

Hello, @JCgH4164838Gh792C124B5

I could verify the test failure on Java 7.
I'm sorry that I didn't run the tests on Java 7 (I got used to relying on CI :D).

Note that the bug in getWriteMethod exists for a while (I didn't touch getWriteMethod in this PR).
It's just exposed by the new test I added.

To fix the bug, I would suggest applying the same fix made in #33 to getWriteMethod() (i.e. not to use BeanInfo).
The implementation difference between getReadMethod and getWriteMethod won't do anything good.

---

Is it possible that removing isMethodCallable() from getReadMethod() may cause OGNL 3.2.x to find/prefer a different "getter" than it did previously (before this change) ?

It might, but I doubt it (see below).

I am wondering if the scenario @harawata found maybe calls for isMethodCallable() to be modified, or maybe a new isCallableBridgeMethod() to be introduced, assuming logic to properly detect a callable bridge method can be found (and performs adequately) ?

If there are cases that actually require some callable-check, I have no objection, of course.
The current isMethodCallable() is clearly outdated, though.

• isJdk15() should always return true.
• Modifier.isVolatile() should always return false.
• Synthetic methods are callable as I explained.

[harawata]

The following implementation might make sense, actually.

static boolean isMethodCallable(Method m) {
  return !m.isSynthetic() || m.isBridge();
}

[JCgH4164838Gh792C124B5]

Hello @harawata .

Thanks for confirming that the Java 7 behaviour was not just a side-effect of my build environment. I only noticed it because I happened to try builds across all of my installed JDK versions. :)

Also, thanks for your thoughts and comments on the getReadMethod() and getWriteMethod() processing. I will try to take a look at possible changes, based on your suggestions.