-
Notifications
You must be signed in to change notification settings - Fork 825
WW-5525 Fixes NPE when checking if expressions is acceptable #1201
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
Conversation
ac1fbc1
to
60a9823
Compare
60a9823
to
9fee06c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the intent behind this PR - to eliminate even the possibility of an NPE - I don't believe we need it anymore
We expect target
will be null for static members and constructors. However, the target
being null doesn't necessarily mean that check is inapplicable. Making that assumption could lead to bugs (pointed out in the comments). It should be up to the consuming method (e.g. #checkDefaultPackageAccess
) to determine how to handle the nullability of target
. If we wanted to force the consuming method to handle the nullability (and prevent NPEs) a better solution might be to wrap it in an Optional
.
The new unit tests look good though, so perhaps we just keep those?
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
Outdated
Show resolved
Hide resolved
491c8f7
to
d9d6bec
Compare
Done, just tests left :) |
|
WW-5525