Skip to content
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.

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

Adds support for EnableExternalDTDLoad property to Checkstyle Plugin #22036

Merged
merged 13 commits into from Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -17,8 +17,11 @@ package org.gradle.api.plugins.quality.checkstyle

import org.gradle.integtests.fixtures.WellBehavedPluginTest
import spock.lang.Issue
import spock.lang.See

import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.containsClass
import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.javaClassWithNewLineAtEnd
import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.multilineJavaClass
import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.simpleCheckStyleConfig

class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
Expand All @@ -30,6 +33,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
def setup() {
buildFile << """
apply plugin: 'groovy'

${mavenCentralRepository()}
tresat marked this conversation as resolved.
Show resolved Hide resolved
"""
}

Expand All @@ -40,12 +45,12 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
apply plugin: 'checkstyle'

dependencies { implementation localGroovy() }
${mavenCentralRepository()}

checkstyle {
configProperties["some"] = new URL("https://gradle.org/")
}
"""

file('src/main/java/Dummy.java') << javaClassWithNewLineAtEnd()
file('config/checkstyle/checkstyle.xml') << simpleCheckStyleConfig()

Expand All @@ -55,4 +60,164 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
then:
executedAndNotSkipped ':checkstyleMain'
}

// region: Enable External DTDs
tresat marked this conversation as resolved.
Show resolved Hide resolved
@Issue("https://github.com/gradle/gradle/issues/21624")
@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
def "can use enable_external_dtd_load feature on extension"() {
given:
buildFile """
apply plugin: 'checkstyle'

checkstyle {
enableExternalDtdLoad = true
}
"""

file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()

when:
fails 'checkstyleMain'

then:
result.assertHasErrorOutput("Checkstyle rule violations were found. See the report at:")
result.assertHasErrorOutput("Checkstyle files with violations: 1")
result.assertHasErrorOutput("Checkstyle violations by severity: [error:2]")
file("build/reports/checkstyle/main.xml").assertContents(containsClass("org.sample.MyClass"))
}

@Issue("https://github.com/gradle/gradle/issues/21624")
@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
def "can use enable_external_dtd_load feature on task"() {
given:
buildFile """
apply plugin: 'checkstyle'

tasks.withType(Checkstyle).configureEach {
enableExternalDtdLoad = true
}
"""

file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()

when:
fails 'checkstyleMain'

then:
result.assertHasErrorOutput("Checkstyle rule violations were found. See the report at:")
result.assertHasErrorOutput("Checkstyle files with violations: 1")
result.assertHasErrorOutput("Checkstyle violations by severity: [error:2]")
file("build/reports/checkstyle/main.xml").assertContents(containsClass("org.sample.MyClass"))
}

@Issue("https://github.com/gradle/gradle/issues/21624")
@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
def "can use enable_external_dtd_load feature on task to override extension value for a task"() {
given:
buildFile """
apply plugin: 'checkstyle'

checkstyle {
enableExternalDtdLoad = false
}

tasks.withType(Checkstyle).configureEach {
enableExternalDtdLoad = true
}
"""

file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()

when:
fails 'checkstyleMain'

then:
result.assertHasErrorOutput("Checkstyle rule violations were found. See the report at:")
result.assertHasErrorOutput("Checkstyle files with violations: 1")
result.assertHasErrorOutput("Checkstyle violations by severity: [error:2]")
file("build/reports/checkstyle/main.xml").assertContents(containsClass("org.sample.MyClass"))
}

@Issue("https://github.com/gradle/gradle/issues/21624")
@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
def "if use enable_external_dtd_load feature NOT enabled, error if feature used in rules XML"() {
given:
buildFile """
apply plugin: 'checkstyle'

checkstyle {
enableExternalDtdLoad = false
}
"""

file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()

when:
fails 'checkstyleMain'

then:
failedDueToXmlDTDProcessingError()
}

@Issue("https://github.com/gradle/gradle/issues/21624")
@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
def "enable_external_dtd_load feature NOT enabled by default"() {
given:buildFile """
tresat marked this conversation as resolved.
Show resolved Hide resolved
apply plugin: 'checkstyle'
"""

file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()

when:
fails 'checkstyleMain'

then:
failedDueToXmlDTDProcessingError()
}

private failedDueToXmlDTDProcessingError() {
result.assertHasErrorOutput("A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction")
tresat marked this conversation as resolved.
Show resolved Hide resolved
result.assertHasErrorOutput("java.lang.NullPointerException")
tresat marked this conversation as resolved.
Show resolved Hide resolved
}

private String checkStyleCommonXml() {
return """
<module name="FileLength">
<property name="max" value="1"/>
</module>
"""
}

private String checkStyleMainXml() {
// Leave the XML processing instruction at the very first position in the file or else the XML is not valid
return """<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd" [
<!ENTITY common SYSTEM "checkstyle-common.xml">
]>
<module name="Checker">

&common;

<module name="TreeWalker">
<module name="MemberName">
<property name="format" value="^[a-z][a-zA-Z]+\$"/>
</module>
</module>

</module>
"""
}
// endregion: Enable External DTDs
}
Expand Up @@ -17,8 +17,11 @@ package org.gradle.api.plugins.quality.checkstyle

import org.gradle.integtests.fixtures.AbstractIntegrationSpec
import org.gradle.test.fixtures.file.TestFile
import org.hamcrest.Matcher

import static org.gradle.util.Matchers.containsLine
import static org.gradle.util.internal.TextUtil.getPlatformLineSeparator
import static org.hamcrest.CoreMatchers.containsString

class CheckstylePluginMultiProjectTest extends AbstractIntegrationSpec {

Expand Down Expand Up @@ -130,4 +133,18 @@ class CheckstylePluginMultiProjectTest extends AbstractIntegrationSpec {
static String javaClassWithNewLineAtEnd() {
"public class Dummy {}${getPlatformLineSeparator()}"
}

static String multilineJavaClass() {
return """
package org.sample;

class MyClass {
int i = 0;
}
"""
}

static Matcher<String> containsClass(String className) {
containsLine(containsString(className.replace(".", File.separator)))
}
}
Expand Up @@ -76,12 +76,14 @@ public class Checkstyle extends SourceTask implements VerificationTask, Reportin
private final Property<JavaLauncher> javaLauncher;
private final Property<String> minHeapSize;
private final Property<String> maxHeapSize;
private final Property<Boolean> enableExternalDtdLoad;

public Checkstyle() {
this.configDirectory = getObjectFactory().directoryProperty();
this.reports = getObjectFactory().newInstance(CheckstyleReportsImpl.class, this);
this.minHeapSize = getObjectFactory().property(String.class);
this.maxHeapSize = getObjectFactory().property(String.class);
this.enableExternalDtdLoad = getObjectFactory().property(Boolean.class);
// Set default JavaLauncher to current JVM in case
// CheckstylePlugin that sets Java launcher convention is not applied
this.javaLauncher = getCurrentJvmLauncher();
Expand Down Expand Up @@ -198,6 +200,9 @@ private void runWithProcessIsolation() {
spec.getForkOptions().setMinHeapSize(minHeapSize.getOrNull());
spec.getForkOptions().setMaxHeapSize(maxHeapSize.getOrNull());
spec.getForkOptions().setExecutable(javaLauncher.get().getExecutablePath().getAsFile().getAbsolutePath());
if (getEnableExternalDtdLoad().isPresent()) {
spec.getForkOptions().getSystemProperties().put("checkstyle.enableExternalDtdLoad", getEnableExternalDtdLoad().get());
}
Copy link
Member

Choose a reason for hiding this comment

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

I would do this slightly differently. Instead of checking if the property is present, let's just put a convention(false) on enableExternalDtdLoad in the constructor. I would then also drop the @Optional annotation below.

That way if someone uses the Checkstyle task without the Checkstyle plugin, they'll have a value. This is similar to what we're doing with the toolchain.

This is against the usual pattern (conventions should be set in the plugin), but as long as we support Checkstyle tasks to be used outside of the plugin, we have to keep them working.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid setting the convention in the constructor, and also to avoid needlessly setting an extra system property set to false (which means the same as absence) when running the checkstyle process.

Are you sure we shouldn't leave the @Optional and use getOrNull() to match the other fork options (these do NOT create system props, but still)?

spec.getClasspath().from(getCheckstyleClasspath());
});
workQueue.submit(CheckstyleAction.class, this::setupParameters);
Expand Down Expand Up @@ -446,4 +451,20 @@ public Property<String> getMinHeapSize() {
public Property<String> getMaxHeapSize() {
return maxHeapSize;
}

/**
* Enable the ability to use custom DTD files in config and load them from some location.
* <strong>Disabled by default due to security concerns.</strong>
* See <a href="https://checkstyle.org/config_system_properties.html#Enable_External_DTD_load">Checkstyle documentation</a> for more details.
*
* @return The property controlling whether to enable the ability to use custom DTD files
tresat marked this conversation as resolved.
Show resolved Hide resolved
*
* @since 7.6
*/
@Incubating
@Optional
@Input
public Property<Boolean> getEnableExternalDtdLoad() {
return enableExternalDtdLoad;
}
}
Expand Up @@ -15,9 +15,13 @@
*/
package org.gradle.api.plugins.quality;

import org.gradle.api.Incubating;
import org.gradle.api.Project;
import org.gradle.api.file.DirectoryProperty;
import org.gradle.api.provider.Property;
import org.gradle.api.resources.TextResource;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.Optional;

import java.io.File;
import java.util.LinkedHashMap;
Expand All @@ -38,10 +42,12 @@ public class CheckstyleExtension extends CodeQualityExtension {
private int maxWarnings = Integer.MAX_VALUE;
private boolean showViolations = true;
private final DirectoryProperty configDirectory;
private final Property<Boolean> enableExternalDtdLoad;

public CheckstyleExtension(Project project) {
this.project = project;
this.configDirectory = project.getObjects().directoryProperty();
this.enableExternalDtdLoad = project.getObjects().property(Boolean.class);
}

/**
Expand Down Expand Up @@ -166,4 +172,20 @@ public boolean isShowViolations() {
public void setShowViolations(boolean showViolations) {
this.showViolations = showViolations;
}

/**
* Enable the ability to use custom DTD files in config and load them from some location on all checkstyle tasks in this project.
* <strong>Disabled by default due to security concerns.</strong>
* See <a href="https://checkstyle.org/config_system_properties.html#Enable_External_DTD_load">Checkstyle documentation</a> for more details.
*
* @return The property controlling whether to enable the ability to use custom DTD files
*
* @since 7.6
*/
@Incubating
@Optional
@Input
public Property<Boolean> getEnableExternalDtdLoad() {
return enableExternalDtdLoad;
}
}
Expand Up @@ -105,6 +105,7 @@ private void configureTaskConventionMapping(Configuration configuration, Checkst
taskMapping.map("maxErrors", (Callable<Integer>) () -> extension.getMaxErrors());
taskMapping.map("maxWarnings", (Callable<Integer>) () -> extension.getMaxWarnings());
task.getConfigDirectory().convention(extension.getConfigDirectory());
task.getEnableExternalDtdLoad().convention(extension.getEnableExternalDtdLoad());
}

private void configureReportsConventionMapping(Checkstyle task, final String baseName) {
Expand Down
Expand Up @@ -24,6 +24,10 @@
<td>configDirectory</td>
<td><literal>project.checkstyle.configDirectory</literal></td>
</tr>
<tr>
<td>enableExternalDtdLoad</td>
<td>false</td>
</tr>
<tr>
<td>ignoreFailures</td>
<td><literal>project.checkstyle.ignoreFailures</literal></td>
Expand Down
Expand Up @@ -32,6 +32,10 @@
<td>configProperties</td>
<td><literal>[:]</literal></td>
</tr>
<tr>
<td>enableExternalDtdLoad</td>
<td><literal>false</literal></td>
</tr>
<tr>
<td>showViolations</td>
<td><literal>true</literal></td>
Expand Down