Skip to content

Commit ad56ce1

Browse files
rombirlirombirli
authored
SONARJAVA-6113 : implement rule S8444 : Excessive logic before super() should not bloat constructor (#5452)
1 parent 4ff6e79 commit ad56ce1

11 files changed

Lines changed: 459 additions & 61 deletions

File tree

‎its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public void javaCheckTestSources() throws Exception {
199199
softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values());
200200
softly.assertThat(newTotal).isEqualTo(knownTotal);
201201
softly.assertThat(rulesCausingFPs).hasSize(10);
202-
softly.assertThat(rulesNotReporting).hasSize(15);
202+
softly.assertThat(rulesNotReporting).hasSize(16);
203203

204204
/**
205205
* 4. Check total number of differences (FPs + FNs)

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"ruleKey": "S8444",
3+
"hasTruePositives": false,
4+
"falseNegatives": 0,
5+
"falsePositives": 0
6+
}

‎java-checks-test-sources/default/src/main/files/non-compiling/checks/PresuperLogicShoudntBloatConstructorSample.java‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package org.sonar.java.checks;
2+
3+
public class PresuperLogicShoudntBloatConstructorSample {
4+
public static class File {
5+
public File(String path) {
6+
// Simulate file initialization logic
7+
}
8+
}
9+
10+
public static class NonCompliantSecureFile extends File {
11+
public NonCompliantSecureFile(String path) {
12+
if (path == null || path.isBlank()) { // Noncompliant {{Excessive logic in this "pre-construction" phase makes the code harder to read and maintain.}}
13+
// ^[el=+19;ec=7]
14+
throw new IllegalArgumentException("Path cannot be empty");
15+
}
16+
if (path.contains("..")) {
17+
throw new IllegalArgumentException("Relative path traversal is forbidden");
18+
}
19+
if (path.startsWith("/root") || path.startsWith("/etc")) {
20+
throw new SecurityException("Access to system directories is restricted");
21+
}
22+
if (path.length() > 255) {
23+
throw new IllegalArgumentException("Path exceeds maximum length");
24+
}
25+
if (!path.matches("^[a-zA-Z0-9/._-]+$")) {
26+
throw new IllegalArgumentException("Path contains illegal characters");
27+
}
28+
String sanitizedPath = path.trim().replace("//", "/");
29+
if (sanitizedPath.endsWith("/")) {
30+
sanitizedPath = sanitizedPath.substring(0, sanitizedPath.length() - 1);
31+
}
32+
super(sanitizedPath);
33+
}
34+
}
35+
36+
public static class NonCompliantSecureFileNestedStatements extends File {
37+
public NonCompliantSecureFile(String path) {
38+
if (true) { // Noncompliant {{Excessive logic in this "pre-construction" phase makes the code harder to read and maintain.}}
39+
// ^[el=+21;ec=7]
40+
if (path == null || path.isBlank()) {
41+
throw new IllegalArgumentException("Path cannot be empty");
42+
}
43+
if (path.contains("..")) {
44+
throw new IllegalArgumentException("Relative path traversal is forbidden");
45+
}
46+
if (path.startsWith("/root") || path.startsWith("/etc")) {
47+
throw new SecurityException("Access to system directories is restricted");
48+
}
49+
if (path.length() > 255) {
50+
throw new IllegalArgumentException("Path exceeds maximum length");
51+
}
52+
if (!path.matches("^[a-zA-Z0-9/._-]+$")) {
53+
throw new IllegalArgumentException("Path contains illegal characters");
54+
}
55+
String sanitizedPath = path.trim().replace("//", "/");
56+
if (sanitizedPath.endsWith("/")) {
57+
sanitizedPath = sanitizedPath.substring(0, sanitizedPath.length() - 1);
58+
}
59+
}
60+
super(sanitizedPath);
61+
}
62+
}
63+
64+
public static class CompliantSecureFile extends File {
65+
public CompliantSecureFile(String path) {
66+
// Compliant: Logic is encapsulated in static helpers
67+
validatePathSecurity(path);
68+
validatePathFormat(path);
69+
String sanitizedPath = normalizePath(path);
70+
super(sanitizedPath);
71+
}
72+
}
73+
74+
public static class EdgeCaseSecureFile extends File {
75+
public EdgeCaseSecureFile(String path) {
76+
// Compliant: There are 3 statements before super() : if, throw, var declaration
77+
if (path.length() > 255 || !path.matches("^[a-zA-Z0-9/._-]+$")) {
78+
throw new IllegalArgumentException("Path format or length is invalid");
79+
}
80+
String sanitizedPath = normalizePath(path);
81+
super(sanitizedPath);
82+
}
83+
84+
public EdgeCaseSecureFile(String path, boolean b) {
85+
// Non-compliant: There are 4 statements before super() : if, throw, var declaration, method call
86+
validatePathSecurity(path); // Noncompliant {{Excessive logic in this "pre-construction" phase makes the code harder to read and maintain.}}
87+
// ^[el=+5;ec=49]
88+
if (path.length() > 255 || !path.matches("^[a-zA-Z0-9/._-]+$")) {
89+
throw new IllegalArgumentException("Path format or length is invalid");
90+
}
91+
String sanitizedPath = normalizePath(path);
92+
super(sanitizedPath);
93+
}
94+
95+
public EdgeCaseSecureFile(String path, int i) {
96+
// Compliant: There are 3 statements before super() : if, if, try-catch block
97+
if (true) {
98+
if (true) {
99+
try {}
100+
catch (Exception e) {}
101+
finally {}
102+
}
103+
}
104+
super(sanitizedPath);
105+
}
106+
107+
108+
public EdgeCaseSecureFile(String path, float f) {
109+
// Compliant: There are 4 statements before super() : if, if, try-catch block, if
110+
if (true) { // Noncompliant {{Excessive logic in this "pre-construction" phase makes the code harder to read and maintain.}}
111+
// ^[el=+9;ec=7]
112+
if (true) {
113+
try {
114+
if (true) {}
115+
}
116+
catch (Exception e) {}
117+
finally {}
118+
}
119+
}
120+
super(sanitizedPath);
121+
}
122+
}
123+
124+
125+
private static void validatePathSecurity(String path) {
126+
if (path == null || path.contains("..")) {
127+
throw new IllegalArgumentException("Invalid or dangerous path sequence");
128+
}
129+
if (path.startsWith("/root") || path.startsWith("/etc")) {
130+
throw new SecurityException("Access to system directories is restricted");
131+
}
132+
}
133+
134+
private static void validatePathFormat(String path) {
135+
if (path.length() > 255 || !path.matches("^[a-zA-Z0-9/._-]+$")) {
136+
throw new IllegalArgumentException("Path format or length is invalid");
137+
}
138+
}
139+
140+
private static String normalizePath(String path) {
141+
String cleaned = path.trim().replace("//", "/");
142+
return cleaned.endsWith("/") ? cleaned.substring(0, cleaned.length() - 1) : cleaned;
143+
}
144+
}

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

Lines changed: 9 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,15 @@
1616
*/
1717
package org.sonar.java.checks;
1818

19-
import java.util.Collections;
20-
import java.util.HashSet;
2119
import java.util.List;
2220
import java.util.Set;
2321

22+
import java.util.stream.Collectors;
2423
import org.sonar.check.Rule;
25-
import org.sonar.java.model.ExpressionUtils;
26-
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
27-
import org.sonar.plugins.java.api.JavaVersion;
28-
import org.sonar.plugins.java.api.JavaVersionAwareVisitor;
2924
import org.sonar.plugins.java.api.semantic.MethodMatchers;
3025
import org.sonar.plugins.java.api.semantic.Symbol;
3126
import org.sonar.plugins.java.api.semantic.Type;
3227
import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
33-
import org.sonar.plugins.java.api.tree.BlockTree;
3428
import org.sonar.plugins.java.api.tree.ExpressionStatementTree;
3529
import org.sonar.plugins.java.api.tree.ExpressionTree;
3630
import org.sonar.plugins.java.api.tree.IdentifierTree;
@@ -40,9 +34,10 @@
4034
import org.sonar.plugins.java.api.tree.StatementTree;
4135
import org.sonar.plugins.java.api.tree.ThrowStatementTree;
4236
import org.sonar.plugins.java.api.tree.Tree;
37+
import org.sonar.plugins.java.api.tree.VariableTree;
4338

4439
@Rule(key = "S8433")
45-
public class FlexibleConstructorBodyValidationCheck extends IssuableSubscriptionVisitor implements JavaVersionAwareVisitor {
40+
public class FlexibleConstructorBodyValidationCheck extends FlexibleConstructorVisitor {
4641

4742
private static final MethodMatchers VALIDATION_METHODS = MethodMatchers.or(
4843
MethodMatchers.create()
@@ -68,69 +63,25 @@ public class FlexibleConstructorBodyValidationCheck extends IssuableSubscription
6863
);
6964

7065
@Override
71-
public List<Tree.Kind> nodesToVisit() {
72-
return Collections.singletonList(Tree.Kind.CONSTRUCTOR);
73-
}
74-
75-
@Override
76-
public boolean isCompatibleWithJavaVersion(JavaVersion version) {
77-
return version.isJava25Compatible();
78-
}
79-
80-
@Override
81-
public void visitNode(Tree tree) {
82-
MethodTree constructor = (MethodTree) tree;
83-
BlockTree body = constructor.block();
84-
85-
if (body == null || body.body().isEmpty()) {
86-
return;
87-
}
88-
89-
// Find the super() or this() call
90-
int constructorCallIndex = findConstructorCallIndex(body);
91-
92-
// Get statements after the constructor call
93-
List<StatementTree> statements = body.body();
94-
if (constructorCallIndex == statements.size() - 1
66+
void validateConstructor(MethodTree constructor, List<StatementTree> body, int constructorCallIndex) {
67+
if (constructorCallIndex == body.size() - 1
9568
|| (constructorCallIndex == -1 && hasNoExplicitSuperClass(constructor))) {
9669
// No statements after constructor call or no superclass and no constructor call
9770
return;
9871
}
99-
10072
// Collect constructor parameters for analysis
101-
Set<Symbol> parameters = new HashSet<>();
102-
constructor.parameters().forEach(param -> parameters.add(param.symbol()));
73+
Set<Symbol> parameters = constructor.parameters().stream().map(VariableTree::symbol).collect(Collectors.toSet());
10374

10475
// Analyze statements after the constructor call for movable validation
105-
for (int i = constructorCallIndex + 1; i < statements.size(); i++) {
106-
StatementTree statement = statements.get(i);
76+
for (int i = constructorCallIndex + 1; i < body.size(); i++) {
77+
StatementTree statement = body.get(i);
10778

10879
if (isValidationStatement(statement) && canBeMovedToPrologue(statement, parameters)) {
10980
reportIssue(statement, "Move this validation logic before the super() or this() call.");
11081
}
11182
}
11283
}
11384

114-
/**
115-
* Find the index of an explicit super() or this() call in the constructor body.
116-
*
117-
* @param body the constructor body to search
118-
* @return the index of the explicit super() or this() call, or -1 if no explicit call is found (implicit super())
119-
*/
120-
private static int findConstructorCallIndex(BlockTree body) {
121-
List<StatementTree> statements = body.body();
122-
for (int i = 0; i < statements.size(); i++) {
123-
if (statements.get(i) instanceof ExpressionStatementTree expressionStatementTree
124-
&& expressionStatementTree.expression() instanceof MethodInvocationTree methodInvocationTree
125-
&& methodInvocationTree.methodSelect() instanceof IdentifierTree identifierTree
126-
&& ExpressionUtils.isThisOrSuper(identifierTree.name())){
127-
return i;
128-
}
129-
}
130-
// No explicit super() or this() call
131-
return -1;
132-
}
133-
13485
private static boolean hasNoExplicitSuperClass(MethodTree constructor) {
13586
Type superClass = constructor.symbol().enclosingClass().superClass();
13687
return (superClass == null || superClass.is("java.lang.Object"));
@@ -202,7 +153,7 @@ public void visitIdentifier(IdentifierTree tree) {
202153
Symbol symbol = tree.symbol();
203154

204155
// Allow parameters, local variables and static fields / methods
205-
if (symbol.isLocalVariable() || symbol.isStatic()|| parameters.contains(symbol)) {
156+
if (symbol.isLocalVariable() || symbol.isStatic() || parameters.contains(symbol)) {
206157
return;
207158
}
208159

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2025 SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.java.checks;
18+
19+
import java.util.Collections;
20+
import java.util.List;
21+
import org.sonar.java.model.ExpressionUtils;
22+
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
23+
import org.sonar.plugins.java.api.JavaVersion;
24+
import org.sonar.plugins.java.api.JavaVersionAwareVisitor;
25+
import org.sonar.plugins.java.api.tree.BlockTree;
26+
import org.sonar.plugins.java.api.tree.ExpressionStatementTree;
27+
import org.sonar.plugins.java.api.tree.IdentifierTree;
28+
import org.sonar.plugins.java.api.tree.MethodInvocationTree;
29+
import org.sonar.plugins.java.api.tree.MethodTree;
30+
import org.sonar.plugins.java.api.tree.StatementTree;
31+
import org.sonar.plugins.java.api.tree.Tree;
32+
33+
public abstract class FlexibleConstructorVisitor extends IssuableSubscriptionVisitor implements JavaVersionAwareVisitor {
34+
35+
/**
36+
* Validate the constructor body, providing the constructor method tree, the list of statements in the constructor body, and the index of any explicit super() or this() call
37+
* (or -1 if no explicit call is found).
38+
* @param constructor the constructor method tree being validated
39+
* @param body the list of statements in the constructor body
40+
* @param constructorCallIndex the index of any explicit super() or this() call in the body, or -1 if no explicit call is found (implicit super())
41+
*/
42+
abstract void validateConstructor(MethodTree constructor, List<StatementTree> body, int constructorCallIndex);
43+
44+
@Override
45+
public final List<Tree.Kind> nodesToVisit() {
46+
return Collections.singletonList(Tree.Kind.CONSTRUCTOR);
47+
}
48+
49+
@Override
50+
public final boolean isCompatibleWithJavaVersion(JavaVersion version) {
51+
return version.isJava25Compatible();
52+
}
53+
54+
@Override
55+
public final void visitNode(Tree tree) {
56+
MethodTree constructor = (MethodTree) tree;
57+
BlockTree block = constructor.block();
58+
if (block == null || block.body().isEmpty()) {
59+
// No body or empty body, nothing to validate
60+
return;
61+
}
62+
List<StatementTree> body = block.body();
63+
64+
// Find the super() or this() call
65+
int constructorCallIndex = findConstructorCallIndex(body);
66+
validateConstructor(constructor, body, constructorCallIndex);
67+
}
68+
69+
/**
70+
* Find the index of an explicit super() or this() call in the constructor body.
71+
*
72+
* @param body the constructor body to search
73+
* @return the index of the explicit super() or this() call, or -1 if no explicit call is found (implicit super())
74+
*/
75+
private static int findConstructorCallIndex(List<StatementTree> body) {
76+
for (int i = 0; i < body.size(); i++) {
77+
if (body.get(i) instanceof ExpressionStatementTree expressionStatementTree
78+
&& expressionStatementTree.expression() instanceof MethodInvocationTree methodInvocationTree
79+
&& methodInvocationTree.methodSelect() instanceof IdentifierTree identifierTree
80+
&& ExpressionUtils.isThisOrSuper(identifierTree.name())) {
81+
return i;
82+
}
83+
}
84+
// No explicit super() or this() call
85+
return -1;
86+
}
87+
}

0 commit comments

Comments
 (0)