fixes setting paramName only when it is not null #6332
Conversation
There was a problem hiding this comment.
Thanks for the PR @ankurpathak! I have provided feedback inline.
|
|
||
| @Override | ||
| public void setVariable(String name, @Nullable Object value) { | ||
| if (name == null) |
There was a problem hiding this comment.
Please add { to follow Spring Security's code conventions
There was a problem hiding this comment.
Added { to if part.
There was a problem hiding this comment.
Do we need to add it for else part as well??
There was a problem hiding this comment.
Yes. We do not use implied { for one line statements
There was a problem hiding this comment.
Added for else part as well.
| @@ -112,7 +112,9 @@ private void addArgumentsAsVariables() { | |||
| } | |||
|
|
|||
| for (int i = 0; i < args.length; i++) { | |||
| super.setVariable(paramNames[i], args[i]); | |||
| if (paramNames[i] != null) { | |||
| super.setVariable(paramNames[i], args[i]); | |||
There was a problem hiding this comment.
For the test below to work, you need to remove super.
| private AuthenticationTrustResolver trustResolver; | ||
|
|
||
| @Test | ||
| public void setVariableTest() { |
There was a problem hiding this comment.
please rename to lookupVariableWhenParameterNameNullThenNotSet
There was a problem hiding this comment.
renamed test to lookupVariableWhenParameterNameNullThenNotSet
ankurpathak
force-pushed
the
gh-17565-npe
branch
3 times, most recently
from
e6a0212
to
dad649d
Compare
ankurpathak
force-pushed
the
gh-17565-npe
branch
from
dad649d
to
7d437a4
Compare
|
changed commit message to reflect Fixes: gh-6223. |
rwinch
added
type: bug
|
Thanks @ankurpathak! This is now merged into master |
Fixes: gh-6223