OGNL improve getter/setter detection algorithm: #75

JCgH4164838Gh792C124B5 · 2019-06-10T04:57:10Z

OGNL improve getter/setter detection algorithm:

Add detail to isDefaultMethod() method comment.
Add a new isNonDefaultPublicInterfaceMethod() method.
Modify collectAccessors() method to consider public interface methods in addition to default methods.
Modify _getGetMethod() to prefer the first valid public getter over other getters, then the first valid public non-Default interface getter and lastly the first getter of any kind.
Modify _getSetMethod() to prefer the first valid public setter over other setters, then the first valid public non-Default interface setter and lastly the first setter of any kind.

- Add detail to isDefaultMethod() method comment. - Add a new isNonDefaultPublicInterfaceMethod() method. - Modify collectAccessors() method to consider public interface methods in addition to default methods. - Modify _getGetMethod() to prefer the first valid public getter over other getters, then the first valid public non-Default interface getter and lastly the first getter of any kind. - Modify _getSetMethod() to prefer the first valid public setter over other setters, then the first valid public non-Default interface setter and lastly the first setter of any kind.

JCgH4164838Gh792C124B5 · 2019-06-10T05:16:15Z

Hello OGNL Team.

This PR attempts to address a behaviour that @yasserzamani identified when reviewing PR#74 and illegal reflective access warnings running Struts 2.5.x under JDK 11.

Currently OGNL 3.1.x picks the first getter/setter that it can find, even if the method is non-public or the declaring class is non-public. The updated logic in this PR attempts to find the first publicly accessible getter/setter method (both method and declaring class public) if possible, considering methods from the class and interfaces implemented by the class.

After a few failed attempts, this implementation seems to work. It passes the regression tests for OGNL as well as the regression build for Struts 2.5.21-SNAPSHOT when used as part of the build.

Running the 2.5.21. Showcase application under JDK 11 with this PR's implementation also seems to avoid the illegal reflective access warnings on the console too. 😄

Since the logic for choosing the getter/setter has changed, this change presents some risk - since it could choose differently than previous OGNL 3.1.x versions. However, choosing a publicly accessible getter/setter seems like better practice if possible to do so.

Please advise what you think about the proposed change, and whether the logic seems sound or not.

yasserzamani · 2019-06-10T08:21:39Z

osm 😮 LGTM 👍

yasserzamani · 2019-06-10T08:24:40Z

And consequently it seems #74 isn't essential anymore (?)

JCgH4164838Gh792C124B5 · 2019-06-11T02:32:06Z

Thanks for taking a look. 😄
I think the changes for #74 are still desirable for OGNL 3.1.x in general (to avoid breaking existing application usage and keeping options open for JDK11).
At least this PR should help "head in the direction" of minimizing the need for the accessibility modification feature, at least for getter/setter methods.

@SInCE

…calOGNL_3_1_GetSetFixEnhance_1 Update local branch to the latest version to deal with any conflicts and then update the @SInCE tags to a new level.

JCgH4164838Gh792C124B5 · 2019-06-12T04:26:43Z

This PR will be updated (hopefully soon) after merging with 3.1.24, making any necessary changes and confirming functionality.

@SInCE

- Updated @SInCE and related comments to refer to 3.1.25 (post-merge of 3.1.24 changes).

- Minor optimization in _getGetMethod() and _getSetMethod() to stop search once best possible match found. Previously was finding all 3 types of matches before stopping the search.

JCgH4164838Gh792C124B5 · 2019-06-26T02:32:47Z

Hello.

I'm wondering if this PR also needs a control flag to decide whether to use the old or new lookup logic.. Although the new logic should choose a "better" match it's possible that existing applications could encounter unexpected behaviour as a result (due to a different getter or setter chosen with new vs. old logic).

An environment flag (or static library method) allowing users to control whether to use the new or old lookup logic would limit that particular risk.

The PR can be adjusted to add such a control mechanism (or any other changes that people think should be made).

- Provide a Java option (environment) flag allowing users to revert to the original "return first match" getter/setter logic if needed. By default the new "best match" getter/setter logic will be used.

sepe81 · 2019-07-17T07:48:02Z

src/test/java/ognl/OgnlRuntimeTest.java

+            // Unavailable (SecurityException, etc.)
+        }
+        if (optionDefinedInEnvironment) {
+            System.out.println("System property " + OgnlRuntime.USE_FIRSTMATCH_GETSET_LOOKUP + " value: " + flagValueFromEnvironment);


IMO it would be better to use logging instead of sysout even for testing.

Hi.
Given the nature of this library, having tests output to sysout seems sufficient for now (AFAIK it's the only mechanism that has been available up until now, and the results are clearly visible in the Maven test run output).
Maybe someone will introduce a testing-only logging replacement for the sysouts in the tests in a future PR. 😄

lukaszlenart · 2019-09-02T07:59:42Z

@JCgH4164838Gh792C124B5 conflicts :(

…calOGNL_3_1_GetSetFixEnhance_1 # Conflicts: # src/java/ognl/OgnlRuntime.java # src/test/java/ognl/OgnlRuntimeTest.java - Manually resolved merge conflicts for OgnlRuntime.java and OgnlRuntimeTest.java.

JCgH4164838Gh792C124B5 · 2019-09-09T03:28:13Z

Fixed the merge conflicts but now Travis CI complains about usage of source level 1.5. 😕
Will change source/target to 1.6 (same as Yasser did for #79) and see if that makes Travis CI happy.

JCgH4164838Gh792C124B5 · 2019-09-09T03:39:35Z

Hello @lukaszlenart .

Manual resolution of merge conflicts appears to have worked. 😄
A follow-up commit to use source/target 1.6 was required to get Travis CI checks to complete (could be set back to 1.5 if needed, though - compiles fine locally with JDK 7).

lukaszlenart · 2019-09-12T06:48:59Z

LGTM 👍

Resolved merge conflicts manually (only pom.xml, decided to include modified comment from 3.1.x). ---------- Merge pull request orphan-oss#75 from JCgH4164838Gh792C124B5/localOGNL_3_1_GetSetFixEnhance_1 OGNL improve getter/setter detection algorithm: (cherry picked from commit c2a6cb5) # Conflicts: # pom.xml

Merge branch 'ognl-3-1-x' of https://github.com/jkuhnert/ognl into lo…

db6f0ff

…calOGNL_3_1_GetSetFixEnhance_1 Update local branch to the latest version to deal with any conflicts and then update the @SInCE tags to a new level.

JCgH4164838Gh792C124B5 added 2 commits June 12, 2019 22:28

Updated commit:

d5bf014

- Updated @SInCE and related comments to refer to 3.1.25 (post-merge of 3.1.24 changes).

Update:

a2c0c95

- Minor optimization in _getGetMethod() and _getSetMethod() to stop search once best possible match found. Previously was finding all 3 types of matches before stopping the search.

lukaszlenart mentioned this pull request Jun 24, 2019

Proposed OGNL enhancement (alt. reflective access for JDK9+, enh. invocation control): #74

Merged

Update:

4f8dec7

- Provide a Java option (environment) flag allowing users to revert to the original "return first match" getter/setter logic if needed. By default the new "best match" getter/setter logic will be used.

sepe81 reviewed Jul 17, 2019

View reviewed changes

Merge branch 'ognl-3-1-x' of https://github.com/jkuhnert/ognl into lo…

0dea426

…calOGNL_3_1_GetSetFixEnhance_1 # Conflicts: # src/java/ognl/OgnlRuntime.java # src/test/java/ognl/OgnlRuntimeTest.java - Manually resolved merge conflicts for OgnlRuntime.java and OgnlRuntimeTest.java.

Update source/target from 1.5 to 1.6 (Travis CI build failing with 1.5)

da3eb4b

JCgH4164838Gh792C124B5 mentioned this pull request Sep 9, 2019

Cache clearing enhancement (clear additional cache, synchronization fix): #77

Merged

lukaszlenart merged commit c2a6cb5 into orphan-oss:ognl-3-1-x Sep 12, 2019

JCgH4164838Gh792C124B5 deleted the localOGNL_3_1_GetSetFixEnhance_1 branch September 28, 2019 16:33

JCgH4164838Gh792C124B5 mentioned this pull request Oct 5, 2019

Forward-port PR#69, PR#74 and PR#75 together from 3.1.x to 3.2.x (cherry-pick) #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OGNL improve getter/setter detection algorithm: #75

OGNL improve getter/setter detection algorithm: #75

JCgH4164838Gh792C124B5 commented Jun 10, 2019

JCgH4164838Gh792C124B5 commented Jun 10, 2019

yasserzamani commented Jun 10, 2019

yasserzamani commented Jun 10, 2019

JCgH4164838Gh792C124B5 commented Jun 11, 2019

JCgH4164838Gh792C124B5 commented Jun 12, 2019

JCgH4164838Gh792C124B5 commented Jun 26, 2019

sepe81 Jul 17, 2019

JCgH4164838Gh792C124B5 Jul 20, 2019

lukaszlenart commented Sep 2, 2019

JCgH4164838Gh792C124B5 commented Sep 9, 2019

JCgH4164838Gh792C124B5 commented Sep 9, 2019

lukaszlenart commented Sep 12, 2019

OGNL improve getter/setter detection algorithm: #75

OGNL improve getter/setter detection algorithm: #75

Conversation

JCgH4164838Gh792C124B5 commented Jun 10, 2019

JCgH4164838Gh792C124B5 commented Jun 10, 2019

yasserzamani commented Jun 10, 2019

yasserzamani commented Jun 10, 2019

JCgH4164838Gh792C124B5 commented Jun 11, 2019

JCgH4164838Gh792C124B5 commented Jun 12, 2019

JCgH4164838Gh792C124B5 commented Jun 26, 2019

sepe81 Jul 17, 2019

Choose a reason for hiding this comment

JCgH4164838Gh792C124B5 Jul 20, 2019

Choose a reason for hiding this comment

lukaszlenart commented Sep 2, 2019

JCgH4164838Gh792C124B5 commented Sep 9, 2019

JCgH4164838Gh792C124B5 commented Sep 9, 2019

lukaszlenart commented Sep 12, 2019