From e3dfe55fde4ad92bc1d1531d2d21e9eec5976150 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Thu, 15 Sep 2022 15:06:40 -0400 Subject: [PATCH 01/10] Adds support for EnableExternalDTDLoad property to Checkstyle Plugin --- .../CheckstylePluginIntegrationTest.groovy | 167 +++++++++++++++++- .../CheckstylePluginMultiProjectTest.groovy | 17 ++ .../api/plugins/quality/Checkstyle.java | 21 +++ .../plugins/quality/CheckstyleExtension.java | 22 +++ .../api/plugins/quality/CheckstylePlugin.java | 1 + ....gradle.api.plugins.quality.Checkstyle.xml | 4 + ...pi.plugins.quality.CheckstyleExtension.xml | 4 + subprojects/docs/src/docs/release/notes.md | 40 +++-- 8 files changed, 264 insertions(+), 12 deletions(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index 5e170f444aba..cef3c2a6afee 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -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 { @@ -30,6 +33,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { def setup() { buildFile << """ apply plugin: 'groovy' + + ${mavenCentralRepository()} """ } @@ -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() @@ -55,4 +60,164 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { then: executedAndNotSkipped ':checkstyleMain' } + + // region: Enable External DTDs + @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 """ + 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") + result.assertHasErrorOutput("java.lang.NullPointerException") + } + + private String checkStyleCommonXml() { + return """ + + + + """ + } + + private String checkStyleMainXml() { + // Leave the XML processing instruction at the very first position in the file or else the XML is not valid + return """ + + ]> + + + &common; + + + + + + + + + """ + } + // endregion: Enable External DTDs } diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginMultiProjectTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginMultiProjectTest.groovy index 3e48a183c3c4..ad7ba43bcc0a 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginMultiProjectTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginMultiProjectTest.groovy @@ -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 { @@ -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 containsClass(String className) { + containsLine(containsString(className.replace(".", File.separator))) + } } diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java index 6219ce301a49..bf7bfa356735 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java @@ -76,12 +76,14 @@ public class Checkstyle extends SourceTask implements VerificationTask, Reportin private final Property javaLauncher; private final Property minHeapSize; private final Property maxHeapSize; + private final Property 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(); @@ -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()); + } spec.getClasspath().from(getCheckstyleClasspath()); }); workQueue.submit(CheckstyleAction.class, this::setupParameters); @@ -446,4 +451,20 @@ public Property getMinHeapSize() { public Property getMaxHeapSize() { return maxHeapSize; } + + /** + * Enable the ability to use custom DTD files in config and load them from some location. + * Disabled by default due to security concerns. + * See Checkstyle documentation 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 getEnableExternalDtdLoad() { + return enableExternalDtdLoad; + } } diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java index 4b7a56f8435d..371827b7c543 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java @@ -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; @@ -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 enableExternalDtdLoad; public CheckstyleExtension(Project project) { this.project = project; this.configDirectory = project.getObjects().directoryProperty(); + this.enableExternalDtdLoad = project.getObjects().property(Boolean.class); } /** @@ -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. + * Disabled by default due to security concerns. + * See Checkstyle documentation 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 getEnableExternalDtdLoad() { + return enableExternalDtdLoad; + } } diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstylePlugin.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstylePlugin.java index bba484de6163..019c4e37a5d9 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstylePlugin.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstylePlugin.java @@ -105,6 +105,7 @@ private void configureTaskConventionMapping(Configuration configuration, Checkst taskMapping.map("maxErrors", (Callable) () -> extension.getMaxErrors()); taskMapping.map("maxWarnings", (Callable) () -> extension.getMaxWarnings()); task.getConfigDirectory().convention(extension.getConfigDirectory()); + task.getEnableExternalDtdLoad().convention(extension.getEnableExternalDtdLoad()); } private void configureReportsConventionMapping(Checkstyle task, final String baseName) { diff --git a/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.Checkstyle.xml b/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.Checkstyle.xml index 79bd49ef2900..28b3b18aad7a 100644 --- a/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.Checkstyle.xml +++ b/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.Checkstyle.xml @@ -24,6 +24,10 @@ configDirectory project.checkstyle.configDirectory + + enableExternalDtdLoad + false + ignoreFailures project.checkstyle.ignoreFailures diff --git a/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.CheckstyleExtension.xml b/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.CheckstyleExtension.xml index 16d400d6a996..bf1fe94b90c1 100644 --- a/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.CheckstyleExtension.xml +++ b/subprojects/docs/src/docs/dsl/org.gradle.api.plugins.quality.CheckstyleExtension.xml @@ -32,6 +32,10 @@ configProperties [:] + + enableExternalDtdLoad + false + showViolations true diff --git a/subprojects/docs/src/docs/release/notes.md b/subprojects/docs/src/docs/release/notes.md index b793bd8f0f50..d1598c502e91 100644 --- a/subprojects/docs/src/docs/release/notes.md +++ b/subprojects/docs/src/docs/release/notes.md @@ -111,7 +111,7 @@ The `init` task now adds compile-time Maven dependencies to Gradle's `api` confi when converting a Maven project. This sharply reduces the number of compilation errors. For more information about Maven conversions, see the [Build Init Plugin](userguide/build_init_plugin.html#sec:pom_maven_conversion). -#### Introduced network timeout configuration for wrapper download +#### Introduced network timeout configuration for wrapper download It is now possible to configure the network timeout for downloading Gradle wrapper files. The default value is 10000ms and can be changed in several ways: @@ -138,7 +138,7 @@ networkTimeout=30000 For more information about the Gradle wrapper, see [Gradle Wrapper](userguide/gradle_wrapper.html#sec:adding_wrapper). -#### Introduced flag for individual task `rerun` +#### Introduced flag for individual task `rerun` All tasks can now use the `--rerun` option. This option works like `--rerun-tasks`, except `--rerun` only effects a single task. For example, you can force tests to @@ -170,7 +170,7 @@ The `dependencies`, `buildEnvironment`, `projects` and `properties` tasks are no The [Maven Publish Plugin](userguide/publishing_maven.html) is now compatible with the configuration cache. Note that when using credentials, the configuration cache requires [safe (empty) credential containers](userguide/configuration_cache.html#config_cache:requirements:safe_credentials). -#### Clarified the ordering of disambiguation rule checks in `resolvableConfigurations` reports +#### Clarified the ordering of disambiguation rule checks in `resolvableConfigurations` reports Attribute disambiguation rules control the variant of a dependency selected by Gradle when: @@ -207,7 +207,7 @@ The following Attributes have disambiguation rules defined. For more information, see [Attribute Disambiguation Rules](userguide/variant_attributes.html#sec:abm_disambiguation_rules). -#### TODO: Extended configuration cache support for external processes +#### TODO: Extended configuration cache support for external processes [Allow buildScan.background to launch external processes with configuration cache enabled gradle#20536](https://github.com/gradle/gradle/issues/20536) #### TODO: Extended configuration cache support for internal plugin @@ -232,7 +232,7 @@ You can now provide a reason message when conditionally disabling a task using t ```groovy tasks.register("slowBenchmark") { def slowBenchmarksEnabled = providers.gradleProperty("my.build.benchmark.slow").map { it.toBoolean() }.orElse(false) - onlyIf("slow benchmarks are enabled with my.build.benchmark.slow") { + onlyIf("slow benchmarks are enabled with my.build.benchmark.slow") { slowBenchmarksEnabled.get() } } @@ -261,7 +261,7 @@ can be passed from the command line as follows: gradle myCustomTask --integer-option=123 ``` -#### TODO: Expanded Java Toolchain support for Service Provider Interfaces +#### TODO: Expanded Java Toolchain support for Service Provider Interfaces Provides a way for plugins to register a provider of Java Toolchain that will allow auto provisioning for any toolchain specification. Service Provider Interface (SPI) TODO: link and definition. @@ -293,8 +293,8 @@ testing { val test by getting(JvmTestSuite::class) { useJUnitJupiter() dependencies { - implementation(module(group = "com.google.guava", - name = "guava", + implementation(module(group = "com.google.guava", + name = "guava", version = "31.1-jre")) } } @@ -354,7 +354,7 @@ and improves IDE support for the Groovy DSL. For more information about the test suite `dependencies` block, see [Differences Between Test Suite and Top-Level Dependencies](userguide/jvm_test_suite_plugin.html#differences_between_the_test_suite_dependencies_and_the_top_level_dependencies_blocks). -#### Introduced support for Java 9+ network debugging +#### Introduced support for Java 9+ network debugging You can run a Java test or application child process with [debugging options](userguide/java_testing.html#sec:debugging_java_tests) @@ -375,6 +375,24 @@ accepting connections via network on Java 9 and above. On Java 9 and above, use the special host address value `*` to make the debugger server listen on all network interfaces. Otherwise, use the address of one of the machine's network interfaces. + +### Checkstyle Plugin + +#### Added Support for `EnableExternalDTDLoad` property to Checkstyle Plugin + +Gradle 7.6 adds a new `enableExternalDTDLoad` property to the [`Checkstyle`](javadoc/org/gradle/api/plugins/quality/Checkstyle.html) task which enables the ability use custom DTD files inside of Checkstyle config XMLs. + +This property can be set on a particular Checkstyle task, or by using the [`CheckstyleExtension`](javadoc/org/gradle/api/plugins/quality/CheckstyleExtension.html) to apply to all Checkstyle tasks in the current project. + +``` +checkstyle { + enableExternalDTDLoad = true +} +``` + +This flag is disabled by default, as there are potential security concerns with enabling it. +For more information, and an example usage scenario, see [the Checkstyle documentation on this property](https://checkstyle.org/config_system_properties.html#Enable_External_DTD_load) and [the discussion on the Checkstyle issue where this property was disabled](https://github.com/checkstyle/checkstyle/issues/6474). + ### IDE @@ -385,7 +403,7 @@ Gradle 7.6 introduces new failure types for the `Failure` interface returned by IDEs can now distinguish between assertion and framework failures using progress event listeners. For test frameworks that expose expected and actual values, `TestAssertionFailure` contains those values. -#### Introduced `TestLauncher` task execution +#### Introduced `TestLauncher` task execution The [`TestLauncher`](javadoc/org/gradle/tooling/TestLauncher.html) interface now allows Tooling API clients to execute any tasks along with the selected tests: @@ -398,7 +416,7 @@ connection.newTestLauncher() .run() ``` -#### Introduced class, method, package, and pattern test selection via `TestLauncher` +#### Introduced class, method, package, and pattern test selection via `TestLauncher` The [TestLauncher](javadoc/org/gradle/tooling/TestLauncher.html) interface now allows Tooling API clients to select test classes, methods, packages and patterns with a new API. From 6ff69c576fc466e1e1eb1d0193a08ad021f4c939 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Fri, 16 Sep 2022 15:30:14 -0400 Subject: [PATCH 02/10] Don't add repository to buildscript for base class tests - The base class tests depend upon CC's eager resolution failing due to a lack of repositories definied. So if we define them in the setup method, those builds succeed, when expected to fail, thus failing tests. - Need to specific repo in every new test then. --- .../CheckstylePluginIntegrationTest.groovy | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index cef3c2a6afee..a1d2706bd44b 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -30,20 +30,26 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { return "check" } + /** + * To ensure the plugins fails (as expected) with configuration cache, do NOT add a repository to the build here, + * the tests in the base class are relying on a failure during eager dependency resolution with CC. + */ def setup() { buildFile << """ apply plugin: 'groovy' - - ${mavenCentralRepository()} """ } + @Issue("https://github.com/gradle/gradle/issues/21301") def "can pass a URL in configProperties"() { given: buildFile """ apply plugin: 'checkstyle' + dependencies { implementation localGroovy() } + ${mavenCentralRepository()} + dependencies { implementation localGroovy() } checkstyle { @@ -69,6 +75,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { buildFile """ apply plugin: 'checkstyle' + ${mavenCentralRepository()} + checkstyle { enableExternalDtdLoad = true } @@ -95,6 +103,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { buildFile """ apply plugin: 'checkstyle' + ${mavenCentralRepository()} + tasks.withType(Checkstyle).configureEach { enableExternalDtdLoad = true } @@ -121,6 +131,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { buildFile """ apply plugin: 'checkstyle' + ${mavenCentralRepository()} + checkstyle { enableExternalDtdLoad = false } @@ -151,6 +163,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { buildFile """ apply plugin: 'checkstyle' + ${mavenCentralRepository()} + checkstyle { enableExternalDtdLoad = false } @@ -172,6 +186,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { def "enable_external_dtd_load feature NOT enabled by default"() { given:buildFile """ apply plugin: 'checkstyle' + + ${mavenCentralRepository()} """ file('src/main/java/org/sample/MyClass.java') << multilineJavaClass() From dda80558d423cb1aad4ff0ed42f3d6c032632fe8 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Tue, 20 Sep 2022 14:45:26 -0400 Subject: [PATCH 03/10] Remove duplicate dependency declaration --- .../quality/checkstyle/CheckstylePluginIntegrationTest.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index a1d2706bd44b..3061669f8d26 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -47,7 +47,6 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { buildFile """ apply plugin: 'checkstyle' - dependencies { implementation localGroovy() } ${mavenCentralRepository()} dependencies { implementation localGroovy() } From 3a85057e89552f92ed953d0b66c37afa5c8fc824 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Tue, 20 Sep 2022 14:55:44 -0400 Subject: [PATCH 04/10] Make property no longer optional and set a convention of false in the task constructor; always set the system property when forking the checkstyle process --- .../groovy/org/gradle/api/plugins/quality/Checkstyle.java | 7 ++----- .../gradle/api/plugins/quality/CheckstyleExtension.java | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java index bf7bfa356735..44c314a39d53 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java @@ -83,7 +83,7 @@ public Checkstyle() { 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); + this.enableExternalDtdLoad = getObjectFactory().property(Boolean.class).convention(false); // Set default JavaLauncher to current JVM in case // CheckstylePlugin that sets Java launcher convention is not applied this.javaLauncher = getCurrentJvmLauncher(); @@ -200,9 +200,7 @@ 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()); - } + spec.getForkOptions().getSystemProperties().put("checkstyle.enableExternalDtdLoad", getEnableExternalDtdLoad().get()); spec.getClasspath().from(getCheckstyleClasspath()); }); workQueue.submit(CheckstyleAction.class, this::setupParameters); @@ -462,7 +460,6 @@ public Property getMaxHeapSize() { * @since 7.6 */ @Incubating - @Optional @Input public Property getEnableExternalDtdLoad() { return enableExternalDtdLoad; diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java index 371827b7c543..b045c27350ac 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java @@ -47,7 +47,7 @@ public class CheckstyleExtension extends CodeQualityExtension { public CheckstyleExtension(Project project) { this.project = project; this.configDirectory = project.getObjects().directoryProperty(); - this.enableExternalDtdLoad = project.getObjects().property(Boolean.class); + this.enableExternalDtdLoad = project.getObjects().property(Boolean.class).convention(false); } /** From 617a5842387052e70dcbb4d4070e0c4ea5f33690 Mon Sep 17 00:00:00 2001 From: Tom Tresansky Date: Tue, 20 Sep 2022 14:56:48 -0400 Subject: [PATCH 05/10] Improve javadoc on property Co-authored-by: Sterling Greene --- .../groovy/org/gradle/api/plugins/quality/Checkstyle.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java index 44c314a39d53..a38aaa60f2ae 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java @@ -451,11 +451,11 @@ public Property getMaxHeapSize() { } /** - * Enable the ability to use custom DTD files in config and load them from some location. - * Disabled by default due to security concerns. + * Enable the use of external DTD files in configuration files. + * Disabled by default because this may be unsafe. * See Checkstyle documentation for more details. * - * @return The property controlling whether to enable the ability to use custom DTD files + * @return property to enable the use of external DTD files * * @since 7.6 */ From f3aece7611d8cf84bdaf9b0c809b28f20fbcd127 Mon Sep 17 00:00:00 2001 From: Tom Tresansky Date: Tue, 20 Sep 2022 14:57:08 -0400 Subject: [PATCH 06/10] Update subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy Co-authored-by: Sterling Greene --- .../quality/checkstyle/CheckstylePluginIntegrationTest.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index 3061669f8d26..3f739473d310 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -183,7 +183,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { @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 """ + given: + buildFile """ apply plugin: 'checkstyle' ${mavenCentralRepository()} From 13ba0d3a6e91cc67f4349159913772f56031a9e4 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Tue, 20 Sep 2022 15:41:09 -0400 Subject: [PATCH 07/10] Improve error message coming out of Checkstyle task failure --- .../CheckstylePluginIntegrationTest.groovy | 6 +-- .../api/plugins/quality/Checkstyle.java | 1 + .../internal/CheckstyleActionParameters.java | 2 + .../quality/internal/CheckstyleInvoker.groovy | 42 ++++++++++--------- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index 3f739473d310..77526712404b 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -40,7 +40,6 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { """ } - @Issue("https://github.com/gradle/gradle/issues/21301") def "can pass a URL in configProperties"() { given: @@ -201,9 +200,8 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { failedDueToXmlDTDProcessingError() } - private failedDueToXmlDTDProcessingError() { - result.assertHasErrorOutput("A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction") - result.assertHasErrorOutput("java.lang.NullPointerException") + private failedDueToXmlDTDProcessingError(String taskName = 'checkstyleMain') { + failure.assertHasCause("An error occurred configuring or running the Checkstyle task: $taskName.") } private String checkStyleCommonXml() { diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java index a38aaa60f2ae..a4be7713b901 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java @@ -224,6 +224,7 @@ private void setupParameters(CheckstyleActionParameters parameters) { if (stylesheetString != null) { parameters.getStylesheetString().set(stylesheetString.asString()); } + parameters.getTaskName().set(getName()); } /** diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java index 3fb984e61cd2..ed8989fc0b15 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java @@ -51,4 +51,6 @@ public interface CheckstyleActionParameters extends WorkParameters { MapProperty getConfigProperties(); Property getStylesheetString(); + + Property getTaskName(); } diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy index 519b21a9278a..99eb99141657 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy @@ -67,32 +67,36 @@ class CheckstyleInvoker implements Action { ant.taskdef(name: 'checkstyle', classname: 'com.puppycrawl.tools.checkstyle.ant.CheckstyleAntTask') } - ant.checkstyle(config: config.asFile, failOnViolation: false, - maxErrors: maxErrors, maxWarnings: maxWarnings, failureProperty: FAILURE_PROPERTY_NAME) { + try { + ant.checkstyle(config: config.asFile, failOnViolation: false, + maxErrors: maxErrors, maxWarnings: maxWarnings, failureProperty: FAILURE_PROPERTY_NAME) { - source.addToAntBuilder(ant, 'fileset', FileCollection.AntType.FileSet) + source.addToAntBuilder(ant, 'fileset', FileCollection.AntType.FileSet) - if (showViolations) { - formatter(type: 'plain', useFile: false) - } + if (showViolations) { + formatter(type: 'plain', useFile: false) + } - if (isXmlRequired || isHtmlRequired) { - formatter(type: 'xml', toFile: xmlOutputLocation) - } + if (isXmlRequired || isHtmlRequired) { + formatter(type: 'xml', toFile: xmlOutputLocation) + } - configProperties.each { key, value -> - property(key: key, value: value.toString()) - } + configProperties.each { key, value -> + property(key: key, value: value.toString()) + } - if (configDir) { - // User provided their own config_loc - def userProvidedConfigLoc = configProperties[CONFIG_LOC_PROPERTY] - if (userProvidedConfigLoc) { - throw new InvalidUserDataException("Cannot add config_loc to checkstyle.configProperties. Please configure the configDirectory on the checkstyle task instead.") + if (configDir) { + // User provided their own config_loc + def userProvidedConfigLoc = configProperties[CONFIG_LOC_PROPERTY] + if (userProvidedConfigLoc) { + throw new InvalidUserDataException("Cannot add config_loc to checkstyle.configProperties. Please configure the configDirectory on the checkstyle task instead.") + } + // Use configDir for config_loc + property(key: CONFIG_LOC_PROPERTY, value: configDir.toString()) } - // Use configDir for config_loc - property(key: CONFIG_LOC_PROPERTY, value: configDir.toString()) } + } catch (Exception e) { + throw new GradleException("An error occurred configuring or running the Checkstyle task: " + parameters.getTaskName().get() + ".", e) } if (isHtmlRequired) { From 3548f39d795e892533a03bebf6a976176cae8f22 Mon Sep 17 00:00:00 2001 From: Thomas Tresansky Date: Tue, 20 Sep 2022 15:46:56 -0400 Subject: [PATCH 08/10] Extract separate class for external DTD options tests --- ...ginExternalDTDOptionIntegrationTest.groovy | 196 ++++++++++++++++++ .../CheckstylePluginIntegrationTest.groovy | 173 ---------------- 2 files changed, 196 insertions(+), 173 deletions(-) create mode 100644 subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy new file mode 100644 index 000000000000..28cf5d1dc0be --- /dev/null +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy @@ -0,0 +1,196 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.api.plugins.quality.checkstyle + +import org.gradle.integtests.fixtures.AbstractIntegrationSpec +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.multilineJavaClass + +@Issue("https://github.com/gradle/gradle/issues/21624") +@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load") +class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrationSpec { + /** + * To ensure the plugins fails (as expected) with configuration cache, do NOT add a repository to the build here, + * the tests in the base class are relying on a failure during eager dependency resolution with CC. + */ + def setup() { + buildFile << """ + apply plugin: 'groovy' + """ + } + + def "can use enable_external_dtd_load feature on extension"() { + given: + buildFile """ + apply plugin: 'checkstyle' + + ${mavenCentralRepository()} + + 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")) + } + + def "can use enable_external_dtd_load feature on task"() { + given: + buildFile """ + apply plugin: 'checkstyle' + + ${mavenCentralRepository()} + + 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")) + } + + def "can use enable_external_dtd_load feature on task to override extension value for a task"() { + given: + buildFile """ + apply plugin: 'checkstyle' + + ${mavenCentralRepository()} + + 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")) + } + + def "if use enable_external_dtd_load feature NOT enabled, error if feature used in rules XML"() { + given: + buildFile """ + apply plugin: 'checkstyle' + + ${mavenCentralRepository()} + + 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() + } + + def "enable_external_dtd_load feature NOT enabled by default"() { + given: + buildFile """ + apply plugin: 'checkstyle' + + ${mavenCentralRepository()} + """ + + 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(String taskName = 'checkstyleMain') { + failure.assertHasCause("An error occurred configuring or running the Checkstyle task: $taskName.") + } + + private String checkStyleCommonXml() { + return """ + + + + """ + } + + private String checkStyleMainXml() { + // Leave the XML processing instruction at the very first position in the file or else the XML is not valid + return """ + + ]> + + + &common; + + + + + + + + + """ + } +} diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy index 77526712404b..fc09efac0153 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy @@ -17,11 +17,8 @@ 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 { @@ -64,174 +61,4 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest { then: executedAndNotSkipped ':checkstyleMain' } - - // region: Enable External DTDs - @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' - - ${mavenCentralRepository()} - - 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' - - ${mavenCentralRepository()} - - 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' - - ${mavenCentralRepository()} - - 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' - - ${mavenCentralRepository()} - - 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 """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - """ - - 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(String taskName = 'checkstyleMain') { - failure.assertHasCause("An error occurred configuring or running the Checkstyle task: $taskName.") - } - - private String checkStyleCommonXml() { - return """ - - - - """ - } - - private String checkStyleMainXml() { - // Leave the XML processing instruction at the very first position in the file or else the XML is not valid - return """ - - ]> - - - &common; - - - - - - - - - """ - } - // endregion: Enable External DTDs } From 91a48ab3734077ff8bc10fb6d29c44dc62387074 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Wed, 21 Sep 2022 01:00:20 -0400 Subject: [PATCH 09/10] Refactor Checkstyle DTD integration tests --- ...ginExternalDTDOptionIntegrationTest.groovy | 86 +++++-------------- 1 file changed, 23 insertions(+), 63 deletions(-) diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy index 28cf5d1dc0be..d9fe7f1623ba 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy @@ -26,75 +26,54 @@ import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiPro @Issue("https://github.com/gradle/gradle/issues/21624") @See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load") class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrationSpec { - /** - * To ensure the plugins fails (as expected) with configuration cache, do NOT add a repository to the build here, - * the tests in the base class are relying on a failure during eager dependency resolution with CC. - */ def setup() { buildFile << """ - apply plugin: 'groovy' + plugins { + id 'java' + id 'checkstyle' + } + + ${mavenCentralRepository()} """ + + file('src/main/java/org/sample/MyClass.java') << multilineJavaClass() + file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml() + file('config/checkstyle/checkstyle.xml') << checkStyleMainXml() } def "can use enable_external_dtd_load feature on extension"() { given: buildFile """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - 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")) + assertFailedWithCheckstyleVerificationErrors() } def "can use enable_external_dtd_load feature on task"() { given: buildFile """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - 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")) + assertFailedWithCheckstyleVerificationErrors() } def "can use enable_external_dtd_load feature on task to override extension value for a task"() { given: buildFile """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - checkstyle { enableExternalDtdLoad = false } @@ -104,63 +83,44 @@ class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrati } """ - 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")) + assertFailedWithCheckstyleVerificationErrors() } def "if use enable_external_dtd_load feature NOT enabled, error if feature used in rules XML"() { given: buildFile """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - 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() + assertFailedWithDtdProcessingError() } def "enable_external_dtd_load feature NOT enabled by default"() { - given: - buildFile """ - apply plugin: 'checkstyle' - - ${mavenCentralRepository()} - """ - - 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() + assertFailedWithDtdProcessingError() + } + + private void assertFailedWithCheckstyleVerificationErrors() { + 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")) } - private failedDueToXmlDTDProcessingError(String taskName = 'checkstyleMain') { + private void assertFailedWithDtdProcessingError(String taskName = 'checkstyleMain') { failure.assertHasCause("An error occurred configuring or running the Checkstyle task: $taskName.") } From 4d01de7c12bfda20123c45015fdd1a7c19b99885 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Wed, 21 Sep 2022 01:38:19 -0400 Subject: [PATCH 10/10] Tweak the error message when Checkstyle fails with a NPE --- ...ginExternalDTDOptionIntegrationTest.groovy | 4 ++- .../api/plugins/quality/Checkstyle.java | 1 - .../internal/CheckstyleActionParameters.java | 2 -- .../CheckstyleInvocationException.java | 27 +++++++++++++++++++ .../quality/internal/CheckstyleInvoker.groovy | 21 ++++++++------- 5 files changed, 41 insertions(+), 14 deletions(-) create mode 100644 subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvocationException.java diff --git a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy index d9fe7f1623ba..8b50374298b2 100644 --- a/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy +++ b/subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy @@ -121,7 +121,9 @@ class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrati } private void assertFailedWithDtdProcessingError(String taskName = 'checkstyleMain') { - failure.assertHasCause("An error occurred configuring or running the Checkstyle task: $taskName.") + failure.assertHasCause("A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction") + failure.assertHasCause("An unexpected error occurred configuring and executing Checkstyle.") + failure.assertHasCause("java.lang.NullPointerException") } private String checkStyleCommonXml() { diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java index a4be7713b901..a38aaa60f2ae 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java @@ -224,7 +224,6 @@ private void setupParameters(CheckstyleActionParameters parameters) { if (stylesheetString != null) { parameters.getStylesheetString().set(stylesheetString.asString()); } - parameters.getTaskName().set(getName()); } /** diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java index ed8989fc0b15..3fb984e61cd2 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleActionParameters.java @@ -51,6 +51,4 @@ public interface CheckstyleActionParameters extends WorkParameters { MapProperty getConfigProperties(); Property getStylesheetString(); - - Property getTaskName(); } diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvocationException.java b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvocationException.java new file mode 100644 index 000000000000..375081daa427 --- /dev/null +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvocationException.java @@ -0,0 +1,27 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.api.plugins.quality.internal; + +import org.gradle.api.GradleException; +import org.gradle.internal.exceptions.Contextual; + +@Contextual +class CheckstyleInvocationException extends GradleException { + CheckstyleInvocationException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy index 99eb99141657..b98bdc34b3f0 100644 --- a/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy +++ b/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvoker.groovy @@ -61,6 +61,14 @@ class CheckstyleInvoker implements Action { xmlOutputLocation = new File(parameters.temporaryDir.asFile.get(), xmlOutputLocation.name) } + if (configDir) { + // User provided their own config_loc + def userProvidedConfigLoc = configProperties[CONFIG_LOC_PROPERTY] + if (userProvidedConfigLoc) { + throw new InvalidUserDataException("Cannot add config_loc to checkstyle.configProperties. Please configure the configDirectory on the checkstyle task instead.") + } + } + try { ant.taskdef(name: 'checkstyle', classname: 'com.puppycrawl.tools.checkstyle.CheckStyleTask') } catch (RuntimeException ignore) { @@ -85,18 +93,11 @@ class CheckstyleInvoker implements Action { property(key: key, value: value.toString()) } - if (configDir) { - // User provided their own config_loc - def userProvidedConfigLoc = configProperties[CONFIG_LOC_PROPERTY] - if (userProvidedConfigLoc) { - throw new InvalidUserDataException("Cannot add config_loc to checkstyle.configProperties. Please configure the configDirectory on the checkstyle task instead.") - } - // Use configDir for config_loc - property(key: CONFIG_LOC_PROPERTY, value: configDir.toString()) - } + // Use configDir for config_loc + property(key: CONFIG_LOC_PROPERTY, value: configDir.toString()) } } catch (Exception e) { - throw new GradleException("An error occurred configuring or running the Checkstyle task: " + parameters.getTaskName().get() + ".", e) + throw new CheckstyleInvocationException("An unexpected error occurred configuring and executing Checkstyle.", e) } if (isHtmlRequired) {