Update isMethodCallable() logic, re-introduce its usage for getReadMethod()

[JCgH4164838Gh792C124B5]

Follow-up to PR #104.
Small updates to OgnlRuntime, attempting to ensure more consistent behaviour for JDK7-11.

• Update OgnlRuntime isMethodCallable(). Removed (now) unnecessary isJDK15 check, added clearer logic and explanatory JavaDoc comments.
• Added OgnlRuntime isMethodCallable_BridgeOrNonSynthetic() for specialized usage such as choosing valid Read and Write methods.
• Made checks in getReadMethod() and getWiteMethod symmetric again.
• Added new unit tests to confirm expected behaviour with respect to bridge methods and synthetic methods.

[JCgH4164838Gh792C124B5]

Hello OGNL Team.

This PR follows some discussion following the introduction of PR#104. It was discovered that the testSyntheticWriteMethod() introduced with that PR failed under JDK 7, and @harawata had some suggestions for possible improvements to OgnlRuntime.isMethodCallable() as well.

Changing isMethodCallable() to also include bridge methods caused additional unit test failures to occur, and even after several debugging attempts I was unable to isolate the underlying reason (just that it appeared to cause OGNL to choose the wrong method signature in some cases).

So, this PR leaves the behaviour of isMethodCallable() unchanged (rejects all synthetic/bridge methods), and introduced a isMethodCallable_BridgeOrNonSynthetic() that can be used in specific cases (after careful consideration). Currently the new method is only used so that bridge methods are considered as valid by OGNL for Read or Write methods.

The changes appear to be safe, and the build tests succeed for JDK7, JDK8 and JDK11 locally. Let me know if you think the changes look OK, or if things could be improved for this PR.

[harawata]

Nice work, @JCgH4164838Gh792C124B5 !

I was wondering if you could find a test case that verifies the effect of isMethodCallable_BridgeOrNonSynthetic check in getReadMethod.
Current tests pass even if the isMethodCallable_BridgeOrNonSynthetic checks were removed from getReadMethod, right?

[JCgH4164838Gh792C124B5]

Hello @harawata .

I was not able to think up a direct way to confirm the effect of isMethodCallable_BridgeOrNonSynthetic() (or isMethodCallable() for that matter), beyond the new unit tests that were introduced with this PR. I am open to ideas, if anyone has suggestions. 😅

The new unit tests, combined with the previous tests that check getReadMethod() and getWriteMethod() calls, appear to confirm the expected behaviour for the changes. The updated logic still passes the testSyntheticBridgeReadMethod() testSyntheticBridgeWriteMethod() tests too (the tests you introduced with PR #104, but renamed to clarify that bridge methods are involved).

As you pointed out, the current tests in this PR do pass even if isMethodCallable_BridgeOrNonSynthetic() is removed from getReadMethod(). Before PR #104, getReadMethod() called isMethodCallable(), which prevented any synthetic or bridge methods from being considered. The changes in this PR should prevent non-bridge synthetic methods from consideration for read/write method lookups, but from what I could determine that seems to be the safer course of action for OGNL.

Other than bridge methods, I could not find any examples of synthetic methods that that have consistent/known names (just compiler-chosen names like access$x, where x is an integer). There does not seem to be any valid use case for trying to call non-bridge synthetic methods, but let me know if you know otherwise ?

Even if there are use cases, it seems it would still be risky to allow OGNL to call non-bridge synthetic methods (which seem to be purely for the compiler's internal use).

Please let me know if the reasoning above seems sound or not ?

[harawata]

Thanks for the update, @JCgH4164838Gh792C124B5 !

I agree that there is no valid use case for calling non-bridge synthetic methods.
I suspect that Class#getMethods() does not return non-bridge synthetic methods by design, but couldn't find any documentation directly backing the theory.
Anyway, although it would be ideal if we could find a test case, I have no strong objection as better-safe(or redundant)-than-sorry is not a bad strategy and the check logic may be very light weight.

[lukaszlenart]

LGTM 👍