SONARJAVA-2943 Rule S2139: Exceptions should be either logged or rethrown but not both#2313
Merged
christophe-zurn-sonarsource merged 5 commits intoNov 2, 2018
Merged
SONARJAVA-2943 Rule S2139: Exceptions should be either logged or rethrown but not both#2313
Conversation
09a89a8 to
ef7ce53
Compare
christophe-zurn-sonarsource
requested changes
Nov 2, 2018
|
|
||
| @Rule(key = "S2139") | ||
| public class LoggedRethrownExceptionsCheck extends IssuableSubscriptionVisitor { | ||
| private static final MethodMatcher JAVA_UTIL_LOGGER = MethodMatcher.create().typeDefinition("java.util.logging.Logger").name("log").withAnyParameters(); |
Contributor
There was a problem hiding this comment.
As you rely on MethodMatcher, the check should ensure it has access to the semantic for the rule to work correctly
|
|
||
| @Rule(key = "S2139") | ||
| public class LoggedRethrownExceptionsCheck extends IssuableSubscriptionVisitor { | ||
| private static final MethodMatcher JAVA_UTIL_LOGGER = MethodMatcher.create().typeDefinition("java.util.logging.Logger").name("log").withAnyParameters(); |
Contributor
There was a problem hiding this comment.
java.util.logging.Logger has other log methods (logp, logrb, throwing, info, warning, ...)
And org.slf4j.Logger logging methods (error, info, ...) could also be added (as it is already used in other checks, ex: LazyArgEvaluationCheck).
|
|
||
| class A { | ||
|
|
||
| public foo() { |
Contributor
There was a problem hiding this comment.
Method is missing return type
| for (StatementTree statementTree : catchTree.block().body()) { | ||
| if (statementTree.is(Tree.Kind.THROW_STATEMENT)) { | ||
| secondaryLocations.add(new Location("", ((ThrowStatementTree) statementTree).expression())); | ||
| isRethrowing = true; |
Contributor
There was a problem hiding this comment.
I think you could just report issue here if error was logged and return, as having statements after a throw is not valid java code
| if (expression.is(Tree.Kind.METHOD_INVOCATION)) { | ||
| MethodInvocationTree mit = (MethodInvocationTree) expression; | ||
| return JAVA_UTIL_LOGGER.matches(mit) && mit.arguments().stream().anyMatch( | ||
| param -> param.is(Tree.Kind.IDENTIFIER) && ((IdentifierTree)param).name().equals(exceptionIdentifier)); |
Contributor
There was a problem hiding this comment.
I would look for any use of the exception symbol inside the arguments, otherwise you will miss the following cases:
logger.log(Level.ALL, "MyError: " + e);
logger.log(Level.ALL, e.getMessage());
throw new MySQLException(contextInfo, e);
…rown but not both
2e3f80a to
e71ab4a
Compare
e71ab4a to
2a277b8
Compare
christophe-zurn-sonarsource
approved these changes
Nov 2, 2018
| public class LoggedRethrownExceptionsCheck extends IssuableSubscriptionVisitor { | ||
| private static final String JAVA_UTIL_LOGGING_LOGGER = "java.util.logging.Logger"; | ||
| private static final String SLF4J_LOGGER = "org.slf4j.Logger"; | ||
| private static final List<MethodMatcher> LOGGING_METHODS = Collections.unmodifiableList(Arrays.asList( |
Contributor
There was a problem hiding this comment.
You could use a MethodMatcherCollection instead
|
|
||
| @Override | ||
| public void visitIdentifier(IdentifierTree tree) { | ||
| if (tree.name().equals(exceptionIdentifier.name())) { |
Contributor
There was a problem hiding this comment.
There is no need to recheck equality if isExceptionIdentifierUsed is already true
christophe-zurn-sonarsource
approved these changes
Nov 2, 2018
christophe-zurn-sonarsource
left a comment
Contributor
There was a problem hiding this comment.
LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.