Skip to content

Commit 3121113

Browse files
gretardgr
gretard
authored and
gr
committed
Fix for #32 
1 parent f883317 commit 3121113

File tree

4 files changed

+211
-93
lines changed

4 files changed

+211
-93
lines changed

‎src/sonar-sql-plugin/pom.xml‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
1+
<project xmlns="http://maven.apache.org/POM/4.0.0"
2+
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
3+
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
24
<modelVersion>4.0.0</modelVersion>
35

46
<groupId>org.sonar.plugins</groupId>
@@ -70,6 +72,14 @@
7072
<version>4.13.2</version>
7173
<scope>test</scope>
7274
</dependency>
75+
<!-- https://mvnrepository.com/artifact/org.assertj/assertj-core -->
76+
<dependency>
77+
<groupId>org.assertj</groupId>
78+
<artifactId>assertj-core</artifactId>
79+
<version>3.24.2</version>
80+
<scope>test</scope>
81+
</dependency>
82+
7383
<dependency>
7484
<groupId>commons-cli</groupId>
7585
<artifactId>commons-cli</artifactId>
@@ -110,10 +120,12 @@
110120
<artifactId>spotless-maven-plugin</artifactId>
111121
<version>2.27.1</version>
112122
<configuration>
113-
<!-- optional: limit format enforcement to just the files changed by this feature branch -->
123+
<!-- optional: limit format enforcement to just the files
124+
changed by this feature branch -->
114125

115126
<formats>
116-
<!-- you can define as many formats as you want, each is independent -->
127+
<!-- you can define as many formats as you want, each is
128+
independent -->
117129
<format>
118130
<!-- define the files to apply to -->
119131
<includes>
@@ -132,9 +144,11 @@
132144
</formats>
133145
<!-- define a language-specific format -->
134146
<java>
135-
<!-- no need to specify files, inferred automatically, but you can if you want -->
147+
<!-- no need to specify files, inferred automatically,
148+
but you can if you want -->
136149

137-
<!-- apply a specific flavor of google-java-format and reflow long strings -->
150+
<!-- apply a specific flavor of google-java-format and
151+
reflow long strings -->
138152
<googleJavaFormat>
139153
<style>AOSP</style>
140154
<reflowLongStrings>true</reflowLongStrings>

‎src/sonar-sql-plugin/src/main/java/org/sonar/plugins/sql/sensors/sqlcheck/SQLCheckIssuesReader.java‎

Lines changed: 43 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import java.io.File;
44
import java.io.IOException;
55
import java.nio.file.Files;
6-
import java.util.List;
6+
import java.util.regex.Pattern;
77

88
import org.sonar.api.utils.log.Logger;
99
import org.sonar.api.utils.log.Loggers;
@@ -12,87 +12,56 @@
1212
import org.sonar.plugins.sql.issues.SqlIssuesList;
1313

1414
public class SQLCheckIssuesReader {
15-
private static final Logger LOGGER = Loggers.get(SQLCheckIssuesReader.class);
15+
private static final Logger LOGGER = Loggers.get(SQLCheckIssuesReader.class);
1616

17-
public SqlIssuesList read(String inputFile, File file) throws IOException {
18-
SqlIssuesList list = new SqlIssuesList();
19-
List<String> lines = Files.readAllLines(file.toPath());
17+
private static final Pattern issueDescriptionPattern = Pattern.compile("●\\s+(.*):?\\r?\\n([^●]+)");
18+
private static final Pattern riskPattern = Pattern.compile("\\[([^\\]]+)\\]:\\s+(\\(.+\\))\\s+(\\(.+\\))\\s+(.+)");
2019

21-
for (int i = 0; i < lines.size(); i++) {
22-
String line = lines.get(i);
23-
if (!line.startsWith("SQL Statement: ")) {
24-
continue;
25-
}
26-
String severityDescription = lines.get(i + 1).trim();
27-
for (int j = i + 2; j < lines.size(); j++) {
28-
String issueName = lines.get(j).trim();
29-
;
30-
if (issueName.isEmpty()) {
31-
continue;
32-
}
20+
public SqlIssuesList read(String inputFile, File file) throws IOException {
21+
var list = new SqlIssuesList();
22+
try {
23+
var txt = Files.readString(file.toPath());
24+
var matcher = riskPattern.matcher(txt);
3325

34-
if (lines.get(j).startsWith("------") || lines.get(j).startsWith("SQL Statement")) {
35-
break;
36-
}
37-
if (issueName.startsWith("● ")) {
38-
try {
39-
StringBuilder descriptionSb = new StringBuilder();
40-
for (int z = j + 1; z < lines.size(); z++) {
41-
String tempLine = lines.get(z).trim();
42-
if (tempLine.startsWith("● ") || tempLine.isEmpty()
43-
|| tempLine.startsWith("SQL Statement")) {
44-
break;
45-
}
46-
if (tempLine.equalsIgnoreCase("Problems:")) {
47-
continue;
48-
}
26+
var matchResults = matcher.results().toList();
27+
for (int i = 0; i < matchResults.size(); i++) {
4928

50-
descriptionSb.append(tempLine);
51-
}
52-
if (descriptionSb.length() == 0 || severityDescription.isEmpty() || issueName.isEmpty()) {
53-
continue;
54-
}
55-
SqlIssue issue = new SqlIssue();
56-
issue.description = descriptionSb.toString().trim();
57-
issue.fileName = inputFile;
58-
issue.severity = extractSeverity(severityDescription);
59-
issue.message = extractMessage(severityDescription);
60-
issue.name = extractName(issueName);
61-
issue.repo = Constants.SQL_SQLCHECK_ENGINEID;
62-
issue.isAdhoc = true;
63-
issue.isExternal = true;
64-
issue.key = extractName(issueName);
65-
list.addIssue(issue);
66-
} catch (Exception e) {
67-
LOGGER.info("Unexpected error reading: {}. Exception was: {}", line, e);
68-
}
69-
}
29+
var matchResult = matchResults.get(i);
7030

71-
}
31+
var risk = matchResult.group(2).replace("(", "").replace(")", "");
32+
var patternName = matchResult.group(4);
7233

73-
}
34+
var startIndex = matchResult.end();
35+
var endIndex = i == matchResults.size() - 1 ? txt.length() : matchResults.get(i + 1).start();
7436

75-
return list;
76-
}
37+
var descriptionSearchString = txt.substring(startIndex, endIndex);
38+
var descriptionsMatcher = issueDescriptionPattern.matcher(descriptionSearchString);
39+
descriptionsMatcher.results().forEach(dm -> {
7740

78-
private String extractName(String issueName) {
79-
return issueName.replace("● ", "");
80-
}
41+
var problem = dm.group(1).trim();
42+
if (problem.endsWith(":")) {
43+
problem = problem.substring(0, problem.length() - 1);
44+
}
8145

82-
private String extractMessage(String severityDescription) {
83-
String[] data = severityDescription.split("\\)", 3);
84-
if (data.length < 3) {
85-
return "Unknown";
86-
}
87-
return data[2].trim();
88-
}
46+
var description = dm.group(2).trim().split("==================== Summary ===================")[0];
47+
48+
var issue = new SqlIssue();
49+
issue.description = description;
50+
issue.fileName = inputFile;
51+
issue.severity = risk;
52+
issue.message = String.format("[%s] %s", patternName, problem);
53+
issue.name = String.format("[%s] %s", patternName, problem);
54+
issue.repo = Constants.SQL_SQLCHECK_ENGINEID;
55+
issue.isAdhoc = true;
56+
issue.isExternal = true;
57+
issue.key = String.format("[%s] %s", patternName, problem);
58+
list.addIssue(issue);
59+
});
60+
}
61+
} catch (Exception e) {
62+
LOGGER.warn("Unexpected error parsing file", e);
63+
}
64+
return list;
65+
}
8966

90-
private String extractSeverity(String severityDescription) {
91-
String[] data = severityDescription.split("\\(", 3);
92-
if (data.length < 3) {
93-
return "HIGH RISK";
94-
}
95-
String severity = data[1].replace("(", "").replace(")", "").trim();
96-
return severity;
97-
}
9867
}

‎src/sonar-sql-plugin/src/test/java/org/sonar/plugins/sql/sensors/sqlcheck/SQLCheckIssuesReaderTest.java‎

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,85 @@
11
package org.sonar.plugins.sql.sensors.sqlcheck;
22

3+
import static org.assertj.core.api.Assertions.assertThat;
4+
35
import java.io.File;
46
import java.io.IOException;
57

68
import org.apache.commons.io.FileUtils;
7-
import org.junit.Assert;
89
import org.junit.Rule;
910
import org.junit.Test;
1011
import org.junit.rules.TemporaryFolder;
1112
import org.sonar.api.impl.utils.JUnitTempFolder;
12-
import org.sonar.plugins.sql.issues.SqlIssuesList;
13+
import org.sonar.plugins.sql.issues.SqlIssue;
1314

1415
public class SQLCheckIssuesReaderTest {
15-
@Rule
16-
public TemporaryFolder folder = new TemporaryFolder();
16+
@Rule
17+
public TemporaryFolder folder = new TemporaryFolder();
18+
19+
@org.junit.Rule
20+
public JUnitTempFolder temp = new org.sonar.api.impl.utils.JUnitTempFolder();
21+
private final SQLCheckIssuesReader sut = new SQLCheckIssuesReader();
22+
23+
@Test
24+
public void testSqlCheckResultsParsingPriorV1_3() throws IOException {
25+
26+
File baseFile = folder.newFile("results.txt");
27+
FileUtils.copyURLToFile(getClass().getResource("/external/sqlcheck.txt"), baseFile);
28+
29+
var results = sut.read("sqlCheckv1_2.sql", baseFile).getaLLIssues();
30+
31+
var expected = getExpectedIssueImplicitColumnUsage();
32+
33+
assertThat(results).hasSize(4).filteredOn(x -> x.key, expected.key).element(0).usingRecursiveComparison()
34+
.ignoringFields("description").isEqualTo(expected);
35+
36+
}
37+
38+
@Test
39+
public void testSqlCheckResultsParsingV1_3() throws IOException {
40+
41+
File baseFile = folder.newFile("results.txt");
42+
FileUtils.copyURLToFile(getClass().getResource("/external/sqlCheck1.3.txt"), baseFile);
43+
44+
var results = sut.read("sqlCheckv1_3.sql", baseFile).getaLLIssues();
1745

18-
@org.junit.Rule
19-
public JUnitTempFolder temp = new org.sonar.api.impl.utils.JUnitTempFolder();
46+
var expected = getExpectedIssueForSelectStar();
47+
assertThat(results).hasSize(4).filteredOn(x -> x.key, expected.key).element(0).usingRecursiveComparison()
48+
.ignoringFields("description").isEqualTo(expected);
2049

21-
@Test
22-
public void test() throws IOException {
50+
}
2351

24-
File baseFile = folder.newFile("results.txt");
25-
FileUtils.copyURLToFile(getClass().getResource("/external/sqlcheck.txt"), baseFile);
52+
private static SqlIssue getExpectedIssueImplicitColumnUsage() {
53+
var issue = new SqlIssue();
54+
issue.fileName = "sqlCheckv1_2.sql";
55+
issue.isAdhoc = true;
56+
issue.repo = "sqlcheck";
57+
issue.name = "[Implicit Column Usage] Explicitly name columns";
58+
issue.key = "[Implicit Column Usage] Explicitly name columns";
59+
issue.message = "[Implicit Column Usage] Explicitly name columns";
60+
issue.severity = "LOW RISK";
61+
issue.description = "Although using wildcards and unnamed columns satisfies the goal of less typing,this habit creates several hazards. This can break application refactoring andcan harm performance. Always spell out all the columns you need, instead ofrelying on wild-cards or implicit column lists.[Matching Expression: insert into test values]";
62+
return issue;
2663

27-
SQLCheckIssuesReader sut = new SQLCheckIssuesReader();
28-
SqlIssuesList results = sut.read("-", baseFile);
64+
}
2965

30-
Assert.assertEquals(4, results.getaLLIssues().size());
66+
private static SqlIssue getExpectedIssueForSelectStar() {
67+
var issue = new SqlIssue();
68+
issue.fileName = "sqlCheckv1_3.sql";
69+
issue.isAdhoc = true;
70+
issue.repo = "sqlcheck";
71+
issue.name = "[SELECT *] Inefficiency in moving data to the consumer";
72+
issue.key = "[SELECT *] Inefficiency in moving data to the consumer";
73+
issue.message = "[SELECT *] Inefficiency in moving data to the consumer";
74+
issue.severity = "HIGH RISK";
75+
issue.description = "When you SELECT *, you're often retrieving more columns from the database than\r\n"
76+
+ "your application really needs to function. This causes more data to move from\r\n"
77+
+ "the database server to the client, slowing access and increasing load on your\r\n"
78+
+ "machines, as well as taking more time to travel across the network. This is\r\n"
79+
+ "especially true when someone adds new columns to underlying tables that didn't\r\n"
80+
+ "exist and weren't needed when the original consumers coded their data access.";
81+
return issue;
3182

32-
}
83+
}
3384

3485
}

‎src/sonar-sql-plugin/src/test/resources/external/sqlCheck1.3.txt‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
+-------------------------------------------------+
2+
| SQLCHECK |
3+
+-------------------------------------------------+
4+
> RISK LEVEL :: ALL ANTI-PATTERNS
5+
> SQL FILE NAME :: top_mutexes.sql
6+
> COLOR MODE :: DISABLED
7+
> VERBOSE MODE :: ENABLED
8+
> DELIMITER :: ;
9+
-------------------------------------------------
10+
==================== Results ===================
11+
12+
-------------------------------------------------
13+
SQL Statement at line 1: with top_mutexes as ( select--+ leading(t1 s1 v1 v2 t2 s2) use_hash(s1)
14+
use_nl(v1) use_hash(s2) materialize t1.hsecs ,s1.* ,s2.sleeps as end_sleeps
15+
,s2.wait_time as end_wait_time ,s2.sleeps-s1.sleeps as delta_sleeps ,t2.hsecs -
16+
t1.hsecs as delta_hsecs --,s2.* from v$timer t1 ,v$mutex_sleep s1 ,(select/*+
17+
no_merge */ sum(level) a from dual connect by level<=1e6) v1 ,v$timer t2
18+
,v$mutex_sleep s2 where s1.mutex_type=s2.mutex_type and s1.location=s2.location
19+
) select * from top_mutexes order by delta_sleeps desc;
20+
[top_mutexes.sql]: (HIGH RISK) (QUERY ANTI-PATTERN) SELECT *
21+
● Inefficiency in moving data to the consumer:
22+
When you SELECT *, you're often retrieving more columns from the database than
23+
your application really needs to function. This causes more data to move from
24+
the database server to the client, slowing access and increasing load on your
25+
machines, as well as taking more time to travel across the network. This is
26+
especially true when someone adds new columns to underlying tables that didn't
27+
exist and weren't needed when the original consumers coded their data access.
28+
29+
30+
● Indexing issues:
31+
Consider a scenario where you want to tune a query to a high level of
32+
performance. If you were to use *, and it returned more columns than you
33+
actually needed, the server would often have to perform more expensive methods
34+
to retrieve your data than it otherwise might. For example, you wouldn't be able
35+
to create an index which simply covered the columns in your SELECT list, and
36+
even if you did (including all columns [shudder]), the next guy who came around
37+
and added a column to the underlying table would cause the optimizer to ignore
38+
your optimized covering index, and you'd likely find that the performance of
39+
your query would drop substantially for no readily apparent reason.
40+
41+
● Binding
42+
Problems:
43+
When you SELECT *, it's possible to retrieve two columns of the same name from
44+
two different tables. This can often crash your data consumer. Imagine a query
45+
that joins two tables, both of which contain a column called "ID". How would a
46+
consumer know which was which? SELECT * can also confuse views (at least in some
47+
versions SQL Server) when underlying table structures change -- the view is not
48+
rebuilt, and the data which comes back can be nonsense. And the worst part of it
49+
is that you can take care to name your columns whatever you want, but the next
50+
guy who comes along might have no way of knowing that he has to worry about
51+
adding a column which will collide with your already-developed names.
52+
[Matching Expression: select * at line 18]
53+
54+
[top_mutexes.sql]: (LOW RISK) (QUERY ANTI-PATTERN) Spaghetti Query Alert
55+
● Split up a complex spaghetti query into several simpler queries:
56+
SQL is a very expressive language—you can accomplish a lot in a single query
57+
or statement. But that doesn't mean it's mandatory or even a good idea to
58+
approach every task with the assumption it has to be done in one line of code.
59+
One common unintended consequence of producing all your results in one query is
60+
a Cartesian product. This happens when two of the tables in the query have no
61+
condition restricting their relationship. Without such a restriction, the join
62+
of two tables pairs each row in the first table to every row in the other table.
63+
Each such pairing becomes a row of the result set, and you end up with many more
64+
rows than you expect. It's important to consider that these queries are simply
65+
hard to write, hard to modify, and hard to debug. You should expect to get
66+
regular requests for incremental enhancements to your database applications.
67+
Managers want more complex reports and more fields in a user interface. If you
68+
design intricate, monolithic SQL queries, it's more costly and time-consuming to
69+
make enhancements to them. Your time is worth something, both to you and to your
70+
project. Split up a complex spaghetti query into several simpler queries. When
71+
you split up a complex SQL query, the result may be many similar queries,
72+
perhaps varying slightly depending on data values. Writing these queries is a
73+
chore, so it's a good application of SQL code generation. Although SQL makes it
74+
seem possible to solve a complex problem in a single line of code, don't be
75+
tempted to build a house of cards.
76+
[Matching Expression: c at lines 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 13, 13, 13, 13, 13, 13, 13, 13, 13, 13, 13, 13, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 15, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 17, 18, 18, 18, 18, 18, 18, 18, 18, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20]
77+
78+
79+
==================== Summary ===================
80+
All Anti-Patterns and Hints :: 2
81+
> High Risk :: 1
82+
> Medium Risk :: 0
83+
> Low Risk :: 1
84+
> Hints :: 0

0 commit comments

Comments
 (0)