Skip to content
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.

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

WW-4817 Makes better decisions on methods first call #36

Merged

Conversation

yasserzamani
Copy link
Contributor

if check permission and run synchronized on method first call in order to fix WW-4817

@lukaszlenart
Copy link
Collaborator

Tested locally and looks good, didn't notice any problems :)

@yasserzamani
Copy link
Contributor Author

Thanks for your test. Yes, I think these changes do no have any functional test in point of view of outside of the invokeMethod method. Previously it got decision then cached, now it caches then decides :)

Just one more thing that I think maybe we do not have to call _securityManager.checkPermission(getPermission(method)); for each method call and the first one for caching is enough but I decided to keep it unchanged logically as I was not sure.

@lukaszlenart
Copy link
Collaborator

Please open PR against 3.0.x branch as well and then please open it again against the master branch and then you can improve the _securityManager.checkPermission(getPermission(method)); call :)

@yasserzamani
Copy link
Contributor Author

Sure :) Do you mean that calling _securityManager.checkPermission(getPermission(method)); in each method call is not necessary (should be removed from 3.0.x, 3.1.x and master) and the first one (i.e. in method first call and filling cache) is enough?

@lukaszlenart
Copy link
Collaborator

No, I meant to keep changes as minimal as possible in 3.1.x and 3.0.x - so your current approach is fine. But in your third PR against the master branch you can optimize this call just to first method call - the master branch is still an experimental and a bit unstable and it targets 3.2.x series.

@lukaszlenart lukaszlenart merged commit d4f23f0 into orphan-oss:ognl-3-1-x Aug 1, 2017
lukaszlenart added a commit that referenced this pull request Aug 1, 2017
lukaszlenart added a commit that referenced this pull request Aug 1, 2017
WW-4817 Similar to #36 for master
@yasserzamani yasserzamani deleted the ognl-3-1-x-WW-4817 branch August 4, 2017 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants