-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fixes two NPE conditions in DefaultEntityResolver return statement #1400
Fixes two NPE conditions in DefaultEntityResolver return statement #1400
Conversation
@@ -163,7 +163,7 @@ public Object findOne(Repositories repositories, StoreInfo info, String reposito | |||
public Object findOne(Repositories repositories, StoreInfo info, Class<?> domainObjClass, String repository, Serializable id) | |||
throws HttpRequestMethodNotSupportedException { | |||
|
|||
Optional<Object> domainObj = null; | |||
Optional<Object> domainObj = Optional.empty(); |
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.
Fixes the two cases of NPE in return statement:
- if AccessDeniedException is thrown.
- if (invoker == null)
@@ -55,7 +55,7 @@ | |||
import internal.org.springframework.content.rest.utils.StoreUtils; | |||
|
|||
public class DefaultEntityResolver implements EntityResolver { | |||
|
|||
private static final Logger logger = org.slf4j.LoggerFactory.getLogger(DefaultEntityResolver.class); |
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.
Logging would be nice, but not mandatory.
domainObj = findOneByReflection(repositories, domainObjClass, id); | ||
} catch (AccessDeniedException ace) { |
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.
Catching the ACE explicitly is be good for code clarity. Regardless if you want to or not to throw ACE. The If you throw it the Servlet context calling this class should be able to process ACE to 403 HTTP response.
@@ -172,13 +175,17 @@ public Object findOne(Repositories repositories, StoreInfo info, Class<?> domain | |||
invoker = resolveRootResourceInformation(info, repository, id, new ModelAndViewContainer(), new FakeWebBinderFactory()); | |||
if (invoker != null) { | |||
domainObj = invoker.invokeFindById(id); | |||
} else { | |||
logger.warn("Could not resolve RootResourceInformation"); |
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.
domainObj
NPE condition in the original code.
} catch (Exception e) { | ||
|
||
e.printStackTrace(); | ||
logger.error("invoking Repository findById(id) method threw exception", e.getCause()); |
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.
domainObj
NPE condition in the original code.
Fixes:
First change fixes the two NullPointerExeception cases in method last return statement.
a) when
invoker == null
b) AccessDeniedException or any Expection other than ConverterNotFoundException is thrown.
2.&3. are not necessary, but I think improve the code.