Skip to content

SONARJAVA-6198 S1451 should accept an empty headerFormat as the absence of any line #5576

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.

Sign up for GitHub

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

Closed
NoemieBenard wants to merge 6 commits into from
Closed

SONARJAVA-6198 S1451 should accept an empty headerFormat as the absence of any line #5576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f47ccc
modify S1451 to accept empty headerFormat
NoemieBenard Apr 16, 2026
a3f72fd
fix incorrect test
NoemieBenard Apr 17, 2026
4eb1254
fix formatting
NoemieBenard Apr 17, 2026
f0a1024
remove S1451 from the list of expected issues in do_not_filter_checks…
NoemieBenard Apr 20, 2026
a7d266f
remove commented test
NoemieBenard Apr 20, 2026
fb81953
rename tests with more explicit names
NoemieBenard Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions java-checks-test-sources/default/src/main/java/checks/FileHeaderCheck/ClassBlankLine.java
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the existing samples do not use descriptive names, but I think this should not stop us here. How about a name like ClassBlankLine.java or something similar?

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

package checks.FileHeaderCheck;

public class ClassBlankLine {
}
// Compliant

5 changes: 5 additions & 0 deletions java-checks-test-sources/default/src/main/java/checks/FileHeaderCheck/ClassNoBlankLine.java
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClassNoHeader.java ?

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package checks.FileHeaderCheck;

public class ClassNoBlankLine {
}
// Compliant
8 changes: 6 additions & 2 deletions java-checks/src/main/java/org/sonar/java/checks/FileHeaderCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,18 @@ public void setContext(JavaFileScannerContext context) {
}
}
} else {
expectedLines = headerFormat.split("(?:\r)?\n|\r");
if (headerFormat.isEmpty()) {
Copy link
Copy Markdown
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional expression is IMO more readable:

expectedLines = headerFormat.isEmpty())
    ? new String[] {}
    :  headerFormat.split("(?:\r)?\n|\r");
``

Copy link
Copy Markdown
Contributor Author

@NoemieBenard NoemieBenard Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up handling the empty case before as it is also necessary to handle it when headerFormat is a regular expression (see #5577)

expectedLines = new String[] {};
} else {
expectedLines = headerFormat.split("(?:\r)?\n|\r");
}
}
visitFile();
}

private String getHeaderFormat() {
String format = headerFormat;
if(format.charAt(0) != '^') {
if (format.charAt(0) != '^') {
Copy link
Copy Markdown
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid reformatting unchanged code.

Copy link
Copy Markdown
Contributor

@sonar-review-alpha sonar-review-alpha Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When isRegularExpression = true and headerFormat is empty, getHeaderFormat() is still called and will throw StringIndexOutOfBoundsException at format.charAt(0) on an empty string.

The PR fixes the non-regex path for empty headerFormat but leaves the regex path unguarded. A user who enables the regex mode and leaves headerFormat empty (the default) will get a runtime exception instead of the "no header required" behaviour.

Consider guarding the same way as the non-regex path, or at minimum returning "^" (match-everything) when the format is empty:

Suggested change
if (format.charAt(0) != '^') {
private String getHeaderFormat() {
String format = headerFormat;
if (format.isEmpty() || format.charAt(0) != '^') {
format = "^" + format;
}
return format;
}
  • Mark as noise

format = "^" + format;
}
return format;
Expand Down
12 changes: 12 additions & 0 deletions java-checks/src/test/java/org/sonar/java/checks/FileHeaderCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ void test() {
.onFile(mainCodeSourcesPath("checks/FileHeaderCheck/Class3.java"))
.withCheck(check)
.verifyNoIssues();

check = new FileHeaderCheck();
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/FileHeaderCheck/ClassBlankLine.java"))
.withCheck(check)
.verifyNoIssues();

check = new FileHeaderCheck();
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/FileHeaderCheck/ClassNoBlankLine.java"))
.withCheck(check)
.verifyNoIssues();
}

@Test
Expand Down
43 changes: 12 additions & 31 deletions sonar-java-plugin/src/test/java/org/sonar/plugins/java/JavaSensorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,19 +374,6 @@ void custom_performance_measure_file_path_can_be_empty() throws IOException {
assertThat(new String(Files.readAllBytes(defaultPerformanceFile), UTF_8)).contains("\"JavaSensor\"");
}

/*@Test
void test_java_version_automatically_accepts_enablePreview_flag_when_maximum_version() throws IOException {
MapSettings settings = new MapSettings();
settings.setProperty("sonar.java.source", JavaVersionImpl.MAX_SUPPORTED);
settings.setProperty("sonar.java.enablePreview", "True");
Path workDir = tmp.newFolder().toPath();
executeJavaSensorForPerformanceMeasure(settings, workDir);
assertThat(logTester.logs(Level.WARN)).isEmpty();
List<String> infoLogs = logTester.logs(Level.INFO);
assertThat(infoLogs).contains("Configured Java source version (sonar.java.source): " + JavaVersionImpl.MAX_SUPPORTED +
", preview features enabled (sonar.java.enablePreview): true");
}*/

@Test
void test_java_version_automatically_disables_enablePreview_flag_when_version_is_less_than_maximum_version() throws IOException {
MapSettings settings = new MapSettings();
Expand All @@ -397,8 +384,7 @@ void test_java_version_automatically_disables_enablePreview_flag_when_version_is
executeJavaSensorForPerformanceMeasure(settings, workDir);
assertThat(logTester.logs(Level.WARN)).contains(
"sonar.java.enablePreview is set but will be discarded as the Java version is less than the max supported version (" +
version + " < " + JavaVersionImpl.MAX_SUPPORTED + ")"
);
version + " < " + JavaVersionImpl.MAX_SUPPORTED + ")");
List<String> infoLogs = logTester.logs(Level.INFO);
assertThat(infoLogs).contains("Configured Java source version (sonar.java.source): " + version +
", preview features enabled (sonar.java.enablePreview): false");
Expand All @@ -412,8 +398,7 @@ void getJavaVersion_does_not_try_to_check_consistency_when_sonar_java_source_is_
Path workDir = tmp.newFolder().toPath();
executeJavaSensorForPerformanceMeasure(settings, workDir);
assertThat(logTester.logs(Level.WARN)).noneMatch(
log -> log.startsWith("sonar.java.enablePreview is set but will be discarded as the Java version is less than the max supported version")
);
log -> log.startsWith("sonar.java.enablePreview is set but will be discarded as the Java version is less than the max supported version"));
List<String> infoLogs = logTester.logs(Level.INFO);
assertThat(infoLogs).noneMatch(log -> log.startsWith("Configured Java source version (sonar.java.source):"));
}
Expand All @@ -429,15 +414,12 @@ void do_not_filter_checks_when_no_autoscan() throws IOException {
"CustomRepository:CustomMainCheck",
"CustomRepository:CustomJspCheck",
"CustomRepository:CustomTestCheck",
// not in SonarWay (FileHeaderCheck)
"java:S1451",
// main check in SonarWay (DefaultPackageCheck)
"java:S1220",
// main check in SonarWay, not supported by autoscan (CombineCatchCheck)
"java:S2147",
// test check in SonarWay (NoTestInTestClassCheck)
"java:S2187"
);
"java:S2187");
}

@Test
Expand All @@ -451,16 +433,15 @@ void filter_checks_when_autoscan_true() throws IOException {
// main check in SonarWay
"java:S1220",
// test check in SonarWay
"java:S2187"
).doesNotContain(
"CustomRepository:CustomMainCheck",
"CustomRepository:CustomJspCheck",
"CustomRepository:CustomTestCheck",
// main check in SonarWay, not supported by autoscan (CombineCatchCheck)
"java:S2147",
// not in SonarWay (FileHeaderCheck)
"java:S1451"
);
"java:S2187")
.doesNotContain(
"CustomRepository:CustomMainCheck",
"CustomRepository:CustomJspCheck",
"CustomRepository:CustomTestCheck",
// main check in SonarWay, not supported by autoscan (CombineCatchCheck)
"java:S2147",
// not in SonarWay (FileHeaderCheck)
"java:S1451");
}

@Test
Expand Down
Loading