Skip to content

Commit cccca83

Browse files
sonar-nigel[bot]Vibe Bot
sonar-nigel[bot]
and
Vibe Bot
authored
SONARJAVA-6039 Fix S2139 false positive when exception is wrapped in constructor call (#5545)
Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
1 parent aa3dd26 commit cccca83

File tree

6 files changed

+67
-46
lines changed

6 files changed

+67
-46
lines changed

‎its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -914,7 +914,7 @@
914914
{
915915
"ruleKey": "S2139",
916916
"hasTruePositives": true,
917-
"falseNegatives": 5,
917+
"falseNegatives": 1,
918918
"falsePositives": 0
919919
},
920920
{

‎its/autoscan/src/test/resources/autoscan/diffs/diff_S2139.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"ruleKey": "S2139",
33
"hasTruePositives": true,
4-
"falseNegatives": 5,
4+
"falseNegatives": 1,
55
"falsePositives": 0
6-
}
6+
}

‎its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2139.json‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
{
2-
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java": [
3-
1070
4-
]
5-
}
1+
{}

‎its/ruling/src/test/resources/eclipse-jetty/java-S2139.json‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,6 @@
11
{
2-
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java": [
3-
1070
4-
],
5-
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java": [
6-
300
7-
],
8-
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/CertificateValidator.java": [
9-
140,
10-
188,
11-
258
12-
],
132
"org.eclipse.jetty:jetty-project:jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java": [
143
484,
154
1838
16-
],
17-
"org.eclipse.jetty:jetty-project:jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlParser.java": [
18-
125
195
]
206
}

‎java-checks-test-sources/default/src/main/java/checks/LoggedRethrownExceptionsCheckSample.java‎

Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,9 @@ public void foo() throws Exception {
1515
Logger logger = java.util.logging.Logger.getAnonymousLogger("");
1616
try {
1717
doSomething();
18-
} catch (SQLException e) { // Noncompliant {{Either log this exception and handle it, or rethrow it with some contextual information.}}
19-
// ^^^^^^^^^^^^^^
18+
} catch (SQLException e) { // Compliant
2019
logger.log(Level.ALL, "", e);
21-
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<
2220
throw new MySQLException(contextInfo, e);
23-
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<
2421
}
2522

2623
try {
@@ -32,14 +29,14 @@ public void foo() throws Exception {
3229

3330
try {
3431
doSomething();
35-
} catch (SQLException e) { // Noncompliant
32+
} catch (SQLException e) { // Compliant
3633
logger.log(Level.ALL, "MyError: " + e);
3734
throw new MySQLException(contextInfo, e);
3835
}
3936

4037
try {
4138
doSomething();
42-
} catch (SQLException e) { // Noncompliant
39+
} catch (SQLException e) { // Compliant
4340
logger.log(Level.ALL, e.getMessage());
4441
throw new MySQLException(contextInfo, e);
4542
}
@@ -54,84 +51,84 @@ public void foo() throws Exception {
5451

5552
try {
5653
doSomething();
57-
} catch (SQLException e) { // Noncompliant
54+
} catch (SQLException e) { // Compliant
5855
logger.logp(Level.ALL, "foo", "bar", e.getMessage());
5956
throw new MySQLException(contextInfo, e);
6057
}
6158

6259
try {
6360
doSomething();
64-
} catch (SQLException e) { // Noncompliant
61+
} catch (SQLException e) { // Compliant
6562
logger.logrb(Level.ALL, "foo", "bar", "", e.getMessage());
6663
throw new MySQLException(contextInfo, e);
6764
}
6865

6966
try {
7067
doSomething();
71-
} catch (SQLException e) { // Noncompliant
68+
} catch (SQLException e) { // Compliant
7269
logger.config(e.getMessage());
7370
throw new MySQLException(contextInfo, e);
7471
}
7572

7673
try {
7774
doSomething();
78-
} catch (SQLException e) { // Noncompliant
75+
} catch (SQLException e) { // Compliant
7976
logger.info(e.getMessage());
8077
throw new MySQLException(contextInfo, e);
8178
}
8279

8380
try {
8481
doSomething();
85-
} catch (SQLException e) { // Noncompliant
82+
} catch (SQLException e) { // Compliant
8683
logger.throwing("foo", "bar", e);
8784
throw new MySQLException(contextInfo, e);
8885
}
8986

9087
try {
9188
doSomething();
92-
} catch (SQLException e) { // Noncompliant
89+
} catch (SQLException e) { // Compliant
9390
logger.severe(e.getMessage());
9491
throw new MySQLException(contextInfo, e);
9592
}
9693

9794
try {
9895
doSomething();
99-
} catch (SQLException e) { // Noncompliant
96+
} catch (SQLException e) { // Compliant
10097
logger.warning(e.getMessage());
10198
throw new MySQLException(contextInfo, e);
10299
}
103100

104101
try {
105102
doSomething();
106-
} catch (SQLException e) { // Noncompliant
103+
} catch (SQLException e) { // Compliant
107104
slf4jLogger.debug(e.getMessage());
108105
throw new MySQLException(contextInfo, e);
109106
}
110107

111108
try {
112109
doSomething();
113-
} catch (SQLException e) { // Noncompliant
110+
} catch (SQLException e) { // Compliant
114111
slf4jLogger.error(e.getMessage());
115112
throw new MySQLException(contextInfo, e);
116113
}
117114

118115
try {
119116
doSomething();
120-
} catch (SQLException e) { // Noncompliant
117+
} catch (SQLException e) { // Compliant
121118
slf4jLogger.info(e.getMessage());
122119
throw new MySQLException(contextInfo, e);
123120
}
124121

125122
try {
126123
doSomething();
127-
} catch (SQLException e) { // Noncompliant
124+
} catch (SQLException e) { // Compliant
128125
slf4jLogger.trace(e.getMessage());
129126
throw new MySQLException(contextInfo, e);
130127
}
131128

132129
try {
133130
doSomething();
134-
} catch (SQLException e) { // Noncompliant
131+
} catch (SQLException e) { // Compliant
135132
slf4jLogger.warn(e.getMessage());
136133
throw new MySQLException(contextInfo, e);
137134
}
@@ -190,6 +187,41 @@ public void foo() throws Exception {
190187
} catch (SQLException e) { // Compliant
191188
}
192189

190+
try {
191+
doSomething();
192+
} catch (SQLException e) { // Noncompliant {{Either log this exception and handle it, or rethrow it with some contextual information.}}
193+
logger.log(Level.ALL, "", e);
194+
throw e;
195+
}
196+
197+
try {
198+
doSomething();
199+
} catch (SQLException e) { // Noncompliant
200+
logger.log(Level.ALL, "MyError: " + e);
201+
throw e;
202+
}
203+
204+
try {
205+
doSomething();
206+
} catch (SQLException e) { // Noncompliant
207+
slf4jLogger.error(e.getMessage());
208+
throw e;
209+
}
210+
211+
try {
212+
doSomething();
213+
} catch (SQLException e) { // Noncompliant
214+
logger.log(Level.ALL, "", e);
215+
throw new SQLException("operation context", e);
216+
}
217+
218+
try {
219+
doSomething();
220+
} catch (SQLException e) { // Compliant
221+
logger.log(Level.ALL, "", e);
222+
throw new RuntimeException("operation failed", e);
223+
}
224+
193225
Exception e1 = new Exception();
194226
logger.log(Level.ALL, "", logger); // Compliant, not inside catch
195227
throw new MySQLException(contextInfo, e1);

‎java-checks/src/main/java/org/sonar/java/checks/LoggedRethrownExceptionsCheck.java‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.sonar.plugins.java.api.tree.ExpressionTree;
2929
import org.sonar.plugins.java.api.tree.IdentifierTree;
3030
import org.sonar.plugins.java.api.tree.MethodInvocationTree;
31+
import org.sonar.plugins.java.api.tree.NewClassTree;
3132
import org.sonar.plugins.java.api.tree.StatementTree;
3233
import org.sonar.plugins.java.api.tree.ThrowStatementTree;
3334
import org.sonar.plugins.java.api.tree.Tree;
@@ -36,6 +37,7 @@
3637

3738
@Rule(key = "S2139")
3839
public class LoggedRethrownExceptionsCheck extends IssuableSubscriptionVisitor {
40+
private static final String MESSAGE = "Either log this exception and handle it, or rethrow it with some contextual information.";
3941
private static final String JAVA_UTIL_LOGGING_LOGGER = "java.util.logging.Logger";
4042
private static final String SLF4J_LOGGER = "org.slf4j.Logger";
4143
private static final MethodMatchers LOGGING_METHODS = MethodMatchers.or(
@@ -62,12 +64,13 @@ public void visitNode(Tree tree) {
6264
List<Location> secondaryLocations = new ArrayList<>();
6365
for (StatementTree statementTree : catchTree.block().body()) {
6466
IdentifierTree exceptionIdentifier = catchTree.parameter().simpleName();
65-
if (isLogging && statementTree.is(Tree.Kind.THROW_STATEMENT) &&
66-
isExceptionUsed(exceptionIdentifier, ((ThrowStatementTree) statementTree).expression())) {
67-
68-
secondaryLocations.add(new Location("Thrown exception.", ((ThrowStatementTree) statementTree).expression()));
69-
reportIssue(catchTree.parameter(), "Either log this exception and handle it, or rethrow it with some contextual information.", secondaryLocations, 0);
70-
return;
67+
if (isLogging && statementTree.is(Tree.Kind.THROW_STATEMENT)) {
68+
ExpressionTree thrown = ((ThrowStatementTree) statementTree).expression();
69+
if ((!thrown.is(Tree.Kind.NEW_CLASS) || isSameExceptionType((NewClassTree) thrown, catchTree)) && isExceptionUsed(exceptionIdentifier, thrown)) {
70+
secondaryLocations.add(new Location("Thrown exception.", thrown));
71+
reportIssue(catchTree.parameter(), MESSAGE, secondaryLocations, 0);
72+
return;
73+
}
7174
}
7275
if (isLoggingMethod(statementTree, exceptionIdentifier)) {
7376
secondaryLocations.add(new Location("Logging statement.", statementTree));
@@ -76,6 +79,10 @@ public void visitNode(Tree tree) {
7679
}
7780
}
7881

82+
private static boolean isSameExceptionType(NewClassTree thrown, CatchTree catchTree) {
83+
return thrown.symbolType().equals(catchTree.parameter().symbol().type());
84+
}
85+
7986
private static boolean isLoggingMethod(StatementTree statementTree, IdentifierTree exceptionIdentifier) {
8087
if (!statementTree.is(Tree.Kind.EXPRESSION_STATEMENT)) {
8188
return false;

0 commit comments

Comments
 (0)