Rework EnvironmentContext#getSubject method to not return null subject #3813
Conversation
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1696/ |
sleshchenko
force-pushed
the
anonymousSubject
branch
2 times, most recently
from
b8639d5 to
cd0fb09
Compare
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1700/ |
| @@ -799,7 +799,7 @@ private Subject sessionUser() { | |||
|
|
|||
| private String sessionUserNameOr(String nameIfNoUser) { | |||
| final Subject subject; | |||
| if (EnvironmentContext.getCurrent() != null && (subject = EnvironmentContext.getCurrent().getSubject()) != null) { | |||
| if ((subject = EnvironmentContext.getCurrent().getSubject()) != Subject.ANONYMOUS) { | |||
There was a problem hiding this comment.
shouldn't we use !EnvironmentContext.getCurrent().getSubject().equals(Subject.ANONYMOUS) instead of != ?
(I agree that it's testing for now the memory pointer but it may have side effect in future as I don't see tests)
There was a problem hiding this comment.
@benoitf
When can comparing by == work incorrect? Looks like only in case changing Subject from interface to class and making this field non final and setting some values.... I can't give the full variant :)
Equals can works incorrect if we change anonymous implementation to SubjectImpl("0000-000-00", "Anonynous", null, false) then equals won't show that it is really anonymous that are returned by EnvironmentContext, or it is some Subject with id "0000-000-00" and name "Anonynous". (I agree that it is crazy example)
Do you think that using equals there is correct variant?
There was a problem hiding this comment.
@sleshchenko maybe a test is missing in https://github.com/sleshchenko/che/blob/cd0fb094cfac053c8bb618fac13200ac1880f17b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java
to ensure it will always work as designed
There was a problem hiding this comment.
@benoitf So this method is used only for logging. I don't know any way to test it and I think we can miss it(Mb you know?). Also we could make this method package private and test only its functionality, but I don't like this way in this case. Because it will be more testing of EnvironmentContext#getSubject method than sessionUserNameOr functionality :)
Mb the best way is adding isAnonymous() method to Subject and don't use subject comparing in such use cases.
There was a problem hiding this comment.
yes I was thinking the same on having isAnonymous() so it delegates the test to Subject and not to all callers
There was a problem hiding this comment.
@benoitf Please, review my new changes
There was a problem hiding this comment.
@sleshchenko looks perfect for me
sleshchenko
force-pushed
the
anonymousSubject
branch
2 times, most recently
from
70dedd8 to
93ef1bf
Compare
sleshchenko
force-pushed
the
anonymousSubject
branch
from
93ef1bf to
6eeadd6
Compare
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1740/ |
What does this PR do?
Rework EnvironmentContext#getSubject method to not return null subject. It allows to avoid null pointer exceptions in case missing component which should set subject for request in custom packaging.
What issues does this PR fix or reference?
Previous behavior
(Remove this section if not relevant)
New behavior
(Explain the PR as it should appear in the release notes)
Please review Che's Contributing Guide for best practices.
Tests written?
No
Docs updated?
Please add a matching PR to the docs repo and link that PR to this issue. Both will be merged at the same time.
Changelog
Rework EnvironmentContext#getSubject method to not return null subject