Skip to content

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

Merged
lukaszlenart merged 1 commit intoorphan-oss:masterfrom
harawata:synthetic-read-method
Aug 17, 2020
Merged

OgnlRuntime#getReadMethod() returns null if the method is a bridge method#104
lukaszlenart merged 1 commit intoorphan-oss:masterfrom
harawata:synthetic-read-method

Conversation

@harawata
Copy link
Copy Markdown
Contributor

@harawata harawata commented Aug 14, 2020

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
        }
    }
}

@harawata harawata mentioned this pull request Aug 14, 2020
Closed
lukaszlenart
lukaszlenart approved these changes Aug 17, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lukaszlenart lukaszlenart merged commit 9d16af0 into orphan-oss:master Aug 17, 2020
@JCgH4164838Gh792C124B5
Copy link
Copy Markdown
Contributor

JCgH4164838Gh792C124B5 commented Oct 12, 2020

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
Copy link
Copy Markdown
Contributor Author

harawata commented Oct 12, 2020

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
Copy link
Copy Markdown
Contributor Author

harawata commented Oct 12, 2020

The following implementation might make sense, actually.

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

@harawata harawata mentioned this pull request Oct 13, 2020
Merged
@JCgH4164838Gh792C124B5
Copy link
Copy Markdown
Contributor

JCgH4164838Gh792C124B5 commented Oct 17, 2020

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.

@lukaszlenart lukaszlenart mentioned this pull request Oct 25, 2020
Merged
harawata added a commit to harawata/ognl that referenced this pull request Jan 8, 2022
@harawata
Include bridge method in collectMethods()
20077a1 
The test passes on Java 8, but yields warning on 9+ and exception on 16+.
It's essentially the same as orphan-oss#104 .
OGNL should call `StringBuilder#codePointAt()` (which is a bridge method) rather than the method defined in its non-public parent class `AbstractStringBuilder`.
@harawata harawata mentioned this pull request Jul 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukaszlenart lukaszlenart lukaszlenart approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@harawata @JCgH4164838Gh792C124B5 @lukaszlenart