From d89ef3fd5f84d4d7cd6a96efe56faa8651203a39 Mon Sep 17 00:00:00 2001 From: Brice Dutheil Date: Mon, 19 Oct 2026 15:35:43 +0200 Subject: [PATCH 1/5] refactor: Move spotbugs and spotless to convention plugins In order, to anticipate the static analysis skill, let's migrate the spotless and forbidden api script plugins to convention plugins. --- build.gradle.kts | 1 - buildSrc/build.gradle.kts | 9 +++ ...java.conventions.forbidden-apis.gradle.kts | 18 ++++++ ...trace-java.conventions.spotbugs.gradle.kts | 63 ++++++++++++++++++ gradle.lockfile | 29 --------- gradle/forbiddenapis.gradle | 18 ------ gradle/java_no_deps.gradle | 4 +- gradle/libs.versions.toml | 2 + gradle/spotbugs.gradle | 64 ------------------- 9 files changed, 94 insertions(+), 114 deletions(-) create mode 100644 buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts create mode 100644 buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts delete mode 100644 gradle.lockfile delete mode 100644 gradle/forbiddenapis.gradle delete mode 100644 gradle/spotbugs.gradle diff --git a/build.gradle.kts b/build.gradle.kts index 30606877d37..34964add571 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -15,7 +15,6 @@ plugins { alias(libs.plugins.shadow) apply false alias(libs.plugins.spotless) - id("com.github.spotbugs") version "6.5.11" id("io.github.gradle-nexus.publish-plugin") version "2.0.0" id("me.champeau.gradle.japicmp") version "0.4.3" id("me.champeau.jmh") version "0.7.3" apply false diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index a5444bfa1eb..6d58f4a4630 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -1,3 +1,5 @@ +import org.gradle.api.attributes.java.TargetJvmVersion + plugins { `java-gradle-plugin` `kotlin-dsl` @@ -94,6 +96,13 @@ repositories { dependencies { implementation(gradleApi()) implementation(libs.forbiddenapis) + implementation(libs.spotbugs) { + // buildSrc targets Java 8 for muzzle workers, but the SpotBugs plugin requires Java 11. + // This is fine because the spotbugs plugin runs in the Gradle Daemon JVM (Java 25). + attributes { + attribute(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, 11) + } + } implementation("net.bytebuddy", "byte-buddy-gradle-plugin", libs.versions.byte.buddy.get()) diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts new file mode 100644 index 00000000000..f90d1cd2040 --- /dev/null +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts @@ -0,0 +1,18 @@ +plugins { + java + id("de.thetaphi.forbiddenapis") +} + +forbiddenApis { + signaturesFiles = files(rootDir.resolve("gradle/forbiddenApiFilters/main.txt")) + ignoreSignaturesOfMissingClasses = true + ignoreFailures = false +} + +tasks.matching { it.name.startsWith("forbiddenApis") && it.name.endsWith("Test") }.configureEach { + enabled = false +} + +dependencies { + compileOnly(versionCatalogs.named("libs").findLibrary("forbiddenapis").get()) +} diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts new file mode 100644 index 00000000000..59ef392f9c0 --- /dev/null +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts @@ -0,0 +1,63 @@ +import com.github.spotbugs.snom.SpotBugsTask + +plugins { + java + id("com.github.spotbugs") +} + +spotbugs { + ignoreFailures = true + excludeFilter = rootProject.layout.projectDirectory.file("gradle/spotbugFilters/exclude.xml") +} + +tasks.withType().configureEach { + launcher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.current() + } + + if (project.path.startsWith(":dd-smoke-tests") || + !(name.endsWith("Main") || name.endsWith("Main_java11"))) { + enabled = false + return@configureEach + } + + showProgress = !providers.environmentVariable("CI").isPresent + ignoreFailures = false + omitVisitors = listOf( + "ConstructorThrow", + "DefaultEncodingDetector", + "DoInsideDoPrivileged", + "DontUseEnum", + "DroppedException", + "FindDeadLocalStores", + "FindHEmismatch", + "FindNullDeref", + "FindReturnRef", + "FindRunInvocations", + "FindUselessControlFlow", + "InitializationChain", + "LazyInit", + "LoadOfKnownNullValue", + "LostLoggerDueToWeakReference", + "MethodReturnCheck", + "MutableStaticFields", + "Naming", + "RuntimeExceptionCapture", + "SerializableIdiom", + "UnreadFields", + ) + val reportFile = layout.buildDirectory.file("reports/spotbugs/$name.html") + reports { + create("html") { + required = true + outputLocation = reportFile + setStylesheet("fancy-hist.xsl") + } + } +} + +val libs = versionCatalogs.named("libs") +dependencies { + compileOnly(libs.findLibrary("spotbugs-annotations").get()) + testImplementation(libs.findLibrary("jsr305").get()) +} diff --git a/gradle.lockfile b/gradle.lockfile deleted file mode 100644 index 62fccbf9c11..00000000000 --- a/gradle.lockfile +++ /dev/null @@ -1,29 +0,0 @@ -# This is a Gradle generated file for dependency locking. -# Manual edits can break the build and are not advised. -# This file is expected to be part of source control. -# To regenerate this file, run: ./gradlew :dependencies --write-locks -com.github.spotbugs:spotbugs-annotations:4.10.4=spotbugs -com.github.spotbugs:spotbugs:4.10.4=spotbugs -com.github.stephenc.jcip:jcip-annotations:1.0-1=spotbugs -com.google.code.findbugs:jsr305:3.0.2=spotbugs -com.google.code.gson:gson:2.14.0=spotbugs -com.google.errorprone:error_prone_annotations:2.48.0=spotbugs -commons-io:commons-io:2.21.0=spotbugs -jaxen:jaxen:2.0.6=spotbugs -net.sf.saxon:Saxon-HE:12.10=spotbugs -org.apache.bcel:bcel:6.12.0=spotbugs -org.apache.commons:commons-lang3:3.20.0=spotbugs -org.apache.commons:commons-text:1.15.0=spotbugs -org.apache.logging.log4j:log4j-api:2.26.1=spotbugs -org.apache.logging.log4j:log4j-core:2.26.1=spotbugs -org.dom4j:dom4j:2.2.0=spotbugs -org.ow2.asm:asm-analysis:9.10.1=spotbugs -org.ow2.asm:asm-commons:9.10.1=spotbugs -org.ow2.asm:asm-tree:9.10.1=spotbugs -org.ow2.asm:asm-util:9.10.1=spotbugs -org.ow2.asm:asm:9.10.1=spotbugs -org.slf4j:slf4j-api:2.0.17=spotbugsSlf4j -org.slf4j:slf4j-api:2.0.18=spotbugs -org.slf4j:slf4j-simple:2.0.17=spotbugsSlf4j -org.xmlresolver:xmlresolver:5.3.3=spotbugs -empty=spotbugsPlugins diff --git a/gradle/forbiddenapis.gradle b/gradle/forbiddenapis.gradle deleted file mode 100644 index 788d5b1b979..00000000000 --- a/gradle/forbiddenapis.gradle +++ /dev/null @@ -1,18 +0,0 @@ -apply plugin: "de.thetaphi.forbiddenapis" - -def mainFilterFile = files("$rootDir/gradle/forbiddenApiFilters/main.txt") - -forbiddenApis { - signaturesFiles = mainFilterFile - ignoreSignaturesOfMissingClasses = true - ignoreFailures = false -} - -// disable forbiddenApis for all test tasks -tasks.matching { it.name.startsWith('forbiddenApis') && it.name.endsWith('Test') }.configureEach { - it.enabled = false -} - -dependencies { - compileOnly libs.forbiddenapis -} diff --git a/gradle/java_no_deps.gradle b/gradle/java_no_deps.gradle index b723dd5e73c..f558fc04fb4 100644 --- a/gradle/java_no_deps.gradle +++ b/gradle/java_no_deps.gradle @@ -10,11 +10,11 @@ import java.nio.file.LinkOption apply plugin: 'dd-trace-java.configure-tests' apply plugin: 'java-library' apply plugin: 'dd-trace-java.test-jvm-constraints' +apply plugin: 'dd-trace-java.conventions.forbidden-apis' +apply plugin: 'dd-trace-java.conventions.spotbugs' apply from: "$rootDir/gradle/codenarc.gradle" -apply from: "$rootDir/gradle/forbiddenapis.gradle" apply from: "$rootDir/gradle/spotless.gradle" -apply from: "$rootDir/gradle/spotbugs.gradle" apply from: "$rootDir/gradle/repositories.gradle" apply from: "$rootDir/gradle/test-suites.gradle" diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 5ca41e88055..3391e29ab8c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -7,6 +7,7 @@ forbiddenapis = "3.11" gradle-tooling-api = "8.14.5" jsr305 = "3.0.2" shadow = "9.4.2" +spotbugs-gradle-plugin = "6.5.11" spotbugs_annotations = "4.10.4" # Source code formatters @@ -94,6 +95,7 @@ forbiddenapis = { module = "de.thetaphi:forbiddenapis", version.ref = "forbidden gradle-tooling-api = { module = "org.gradle:gradle-tooling-api", version.ref = "gradle-tooling-api" } jsr305 = { module = "com.google.code.findbugs:jsr305", version.ref = "jsr305" } spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs_annotations" } +spotbugs = { module = "com.github.spotbugs.snom:spotbugs-gradle-plugin", version.ref = "spotbugs-gradle-plugin" } # DataDog libs and forks datadog-okhttp = { module = "com.datadoghq.okhttp3:okhttp", version.ref = "datadog-okhttp" } diff --git a/gradle/spotbugs.gradle b/gradle/spotbugs.gradle deleted file mode 100644 index a69304a7728..00000000000 --- a/gradle/spotbugs.gradle +++ /dev/null @@ -1,64 +0,0 @@ -apply plugin: 'com.github.spotbugs' - -spotbugs { - ignoreFailures = true - excludeFilter = file("$rootDir/gradle/spotbugFilters/exclude.xml") -} - -// Configure spotbugs for Main tasks and disable it for all other -// Can't use the class here due to being different class -tasks.matching { it.name.startsWith('spotbugs') }.configureEach { - // Needs Java 11+ to run spotbugs - it.launcher = javaToolchains.launcherFor { - it.languageVersion = JavaLanguageVersion.current() - } - - def name = it.name - if (project.path.startsWith(":dd-smoke-tests") - || !(name.endsWith("Main") - || name.endsWith("Main_java11")) - ) { - it.enabled = false - return - } - - it.showProgress = !providers.environmentVariable("CI").isPresent() - it.ignoreFailures = false - // detector documentation is in the following link: - // https://spotbugs-in-kengo-toda.readthedocs.io/en/lqc-list-detectors/detectors.html - it.omitVisitors = [ - 'ConstructorThrow', - 'DefaultEncodingDetector', - 'DoInsideDoPrivileged', - 'DontUseEnum', - 'DroppedException', - 'FindDeadLocalStores', - 'FindHEmismatch', - 'FindNullDeref', - 'FindReturnRef', - 'FindRunInvocations', - 'FindUselessControlFlow', - 'InitializationChain', - 'LazyInit', - 'LoadOfKnownNullValue', - 'LostLoggerDueToWeakReference', - 'MethodReturnCheck', - 'MutableStaticFields', - 'Naming', - 'RuntimeExceptionCapture', - 'SerializableIdiom', - 'UnreadFields', - ] - it.reports { - html { - required = true - destination(file("$buildDir/reports/spotbugs/${name}.html")) - stylesheet = 'fancy-hist.xsl' - } - } -} - -dependencies { - compileOnly(libs.spotbugs.annotations) - testImplementation(libs.jsr305) -} From 15b256e4612bd8473d4ef0d601775869ea26a386 Mon Sep 17 00:00:00 2001 From: Brice Dutheil Date: Mon, 19 Oct 2026 16:41:11 +0200 Subject: [PATCH 2/5] refactor: Separate task level behavior from plugin conventin default --- ...trace-java.conventions.spotbugs.gradle.kts | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts index 59ef392f9c0..9a7efb20eb9 100644 --- a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts @@ -6,23 +6,9 @@ plugins { } spotbugs { - ignoreFailures = true + ignoreFailures = false excludeFilter = rootProject.layout.projectDirectory.file("gradle/spotbugFilters/exclude.xml") -} - -tasks.withType().configureEach { - launcher = javaToolchains.launcherFor { - languageVersion = JavaLanguageVersion.current() - } - - if (project.path.startsWith(":dd-smoke-tests") || - !(name.endsWith("Main") || name.endsWith("Main_java11"))) { - enabled = false - return@configureEach - } - showProgress = !providers.environmentVariable("CI").isPresent - ignoreFailures = false omitVisitors = listOf( "ConstructorThrow", "DefaultEncodingDetector", @@ -46,6 +32,19 @@ tasks.withType().configureEach { "SerializableIdiom", "UnreadFields", ) +} + +tasks.withType().configureEach { + if (project.path.startsWith(":dd-smoke-tests") || + !(name.endsWith("Main") || name.endsWith("Main_java11"))) { + enabled = false + return@configureEach + } + + launcher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.current() + } + val reportFile = layout.buildDirectory.file("reports/spotbugs/$name.html") reports { create("html") { From 7c8a57551040d57af1969bc4cf630983917801ef Mon Sep 17 00:00:00 2001 From: Brice Dutheil Date: Tue, 20 Oct 2026 14:17:39 +0200 Subject: [PATCH 3/5] refactor: Improve configurability of spotbugs task per sourceset --- buildSrc/build.gradle.kts | 5 +- ...trace-java.conventions.spotbugs.gradle.kts | 65 ++++--- ...ace-java.module.instrumentation.gradle.kts | 7 +- .../ForbiddenApisConventionPluginTest.kt | 155 ++++++++++++++++ .../spotbugs/SpotBugsConventionsPluginTest.kt | 169 ++++++++++++++++++ gradle/libs.versions.toml | 2 +- 6 files changed, 366 insertions(+), 37 deletions(-) create mode 100644 buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt create mode 100644 buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index 6d58f4a4630..8a4d0cf3ddc 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -96,9 +96,8 @@ repositories { dependencies { implementation(gradleApi()) implementation(libs.forbiddenapis) - implementation(libs.spotbugs) { - // buildSrc targets Java 8 for muzzle workers, but the SpotBugs plugin requires Java 11. - // This is fine because the spotbugs plugin runs in the Gradle Daemon JVM (Java 25). + implementation(libs.spotbugs.gradle.plugin) { + // Select the plugin's Java 11 variant while buildSrc still has to targets Java 8 for muzzle workers. attributes { attribute(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, 11) } diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts index 9a7efb20eb9..bd3064f402c 100644 --- a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts @@ -7,38 +7,47 @@ plugins { spotbugs { ignoreFailures = false - excludeFilter = rootProject.layout.projectDirectory.file("gradle/spotbugFilters/exclude.xml") + // Exclude known types and packages + excludeFilter = rootDir.resolve("gradle/spotbugFilters/exclude.xml") showProgress = !providers.environmentVariable("CI").isPresent - omitVisitors = listOf( - "ConstructorThrow", - "DefaultEncodingDetector", - "DoInsideDoPrivileged", - "DontUseEnum", - "DroppedException", - "FindDeadLocalStores", - "FindHEmismatch", - "FindNullDeref", - "FindReturnRef", - "FindRunInvocations", - "FindUselessControlFlow", - "InitializationChain", - "LazyInit", - "LoadOfKnownNullValue", - "LostLoggerDueToWeakReference", - "MethodReturnCheck", - "MutableStaticFields", - "Naming", - "RuntimeExceptionCapture", - "SerializableIdiom", - "UnreadFields", - ) } +val productionTaskName = Regex("spotbugsMain(?:_java\\d+)?") + tasks.withType().configureEach { - if (project.path.startsWith(":dd-smoke-tests") || - !(name.endsWith("Main") || name.endsWith("Main_java11"))) { - enabled = false - return@configureEach + when { + project.path.startsWith(":dd-smoke-tests") -> { + enabled = false + } + productionTaskName.matches(name) -> { + omitVisitors = listOf( + "ConstructorThrow", + "DefaultEncodingDetector", + "DoInsideDoPrivileged", + "DontUseEnum", + "DroppedException", + "FindDeadLocalStores", + "FindHEmismatch", + "FindNullDeref", + "FindReturnRef", + "FindRunInvocations", + "FindUselessControlFlow", + "InitializationChain", + "LazyInit", + "LoadOfKnownNullValue", + "LostLoggerDueToWeakReference", + "MethodReturnCheck", + "MutableStaticFields", + "Naming", + "RuntimeExceptionCapture", + "SerializableIdiom", + "UnreadFields", + ) + } + else -> { + // static-analysis-rules skill: enable selected test tasks here with their own detector configuration. + enabled = false + } } launcher = javaToolchains.launcherFor { diff --git a/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts index f5d797dfbe6..ad4f117ce52 100644 --- a/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts +++ b/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts @@ -2,7 +2,7 @@ import datadog.gradle.plugin.instrument.BuildTimeInstrumentationExtension plugins { id("dd-trace-java.conventions.java") - id("de.thetaphi.forbiddenapis") + id("dd-trace-java.conventions.forbidden-apis") } // These plugins are produced by buildSrc and are only available at runtime. @@ -26,10 +26,7 @@ configurations.named("muzzleBootstrap") { tasks.withType().configureEach { enabled = false } forbiddenApis { - signaturesFiles = files( - "$rootDir/gradle/forbiddenApiFilters/main.txt", - "$rootDir/gradle/forbiddenApiFilters/instrumentation.txt" - ) + signaturesFiles += files(rootDir.resolve("gradle/forbiddenApiFilters/instrumentation.txt")) } val libs = versionCatalogs.named("libs") diff --git a/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt b/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt new file mode 100644 index 00000000000..fef6bd17de4 --- /dev/null +++ b/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt @@ -0,0 +1,155 @@ +package datadog.gradle.plugin.quality + +import datadog.gradle.plugin.GradleFixture +import org.assertj.core.api.Assertions.assertThat +import org.gradle.testkit.runner.TaskOutcome +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import java.io.File + +class ForbiddenApisConventionPluginTest : GradleFixture() { + @BeforeEach + fun setup() { + writeSettings("""rootProject.name = "forbidden-apis-convention-test"""") + writeFile("gradle/libs.versions.toml", File("../gradle/libs.versions.toml").readText()) + writeFile( + "gradle/forbiddenApiFilters/main.txt", + """ + java.lang.System#exit(int) + missing.OptionalDependency + """ + ) + } + + @Test + fun `preserves defaults and name-based exclusions for late source sets and tasks`() { + addSubproject( + "library", + """ + import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis + + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + + sourceSets.create("latestDepTest") + sourceSets.create("main_java11") + sourceSets.create("testFixtures") + sourceSets.create("jmh") + tasks.register("forbiddenApisSyntheticTest") + tasks.register("unrelatedTest") + + tasks.register("verifyDefaults") { + doLast { + val mainFilter = rootProject.file("gradle/forbiddenApiFilters/main.txt") + tasks.withType().forEach { + check(it.signaturesFiles.files == setOf(mainFilter)) + check(it.ignoreSignaturesOfMissingClasses) + check(!it.ignoreFailures) + } + listOf("forbiddenApisTest", "forbiddenApisLatestDepTest", "forbiddenApisSyntheticTest").forEach { + check(!tasks.getByName(it).enabled) + } + listOf("forbiddenApisMain", "forbiddenApisMain_java11", "forbiddenApisTestFixtures", "forbiddenApisJmh", "unrelatedTest").forEach { + check(tasks.getByName(it).enabled) + } + val aggregate = tasks.getByName("forbiddenApis") + val checkTask = tasks.getByName("check") + check(aggregate in checkTask.taskDependencies.getDependencies(checkTask)) + check(tasks.getByName("forbiddenApisMain_java11") in aggregate.taskDependencies.getDependencies(aggregate)) + val dependency = configurations.getByName("compileOnly").dependencies.single { + it.group == "de.thetaphi" && it.name == "forbiddenapis" + } + check(dependency.version == libs.versions.forbiddenapis.get()) + check(configurations.getByName("runtimeClasspath").allDependencies.none { it.name == "forbiddenapis" }) + } + } + """ + ) + + val result = run(":library:verifyDefaults") + + assertThat(result.task(":library:verifyDefaults")?.outcome) + .describedAs(result.output).isEqualTo(TaskOutcome.SUCCESS) + } + + @Test + fun `preserves extension and typed task overrides`() { + writeFile("gradle/forbiddenApiFilters/instrumentation.txt", "java.lang.Runtime#exit(int)") + writeRootProject( + """ + import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis + + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + + forbiddenApis { + signaturesFiles += files("gradle/forbiddenApiFilters/instrumentation.txt") + ignoreFailures = true + } + tasks.named("forbiddenApisMain") { + failOnMissingClasses = false + } + tasks.named("forbiddenApisTest") { + enabled = true + } + + tasks.register("verifyOverrides") { + doLast { + val main = tasks.named("forbiddenApisMain").get() + check(main.signaturesFiles.files == setOf(file("gradle/forbiddenApiFilters/main.txt"), file("gradle/forbiddenApiFilters/instrumentation.txt"))) + check(main.ignoreFailures) + check(!main.failOnMissingClasses) + check(tasks.getByName("forbiddenApisTest").enabled) + } + } + """ + ) + + val result = run("verifyOverrides") + + assertThat(result.task(":verifyOverrides")?.outcome) + .describedAs(result.output).isEqualTo(TaskOutcome.SUCCESS) + } + + @Test + fun `fails check on a forbidden call and allows compile-only suppression`() { + writeRootProject( + """ + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + repositories { + mavenCentral() + } + """ + ) + writeJavaSource("Example", "public class Example { public void exit() { System.exit(0); } }") + + val failed = run("check", expectFailure = true) + + assertThat(failed.task(":forbiddenApisMain")?.outcome) + .describedAs(failed.output).isEqualTo(TaskOutcome.FAILED) + assertThat(failed.output).contains("java.lang.System#exit(int)") + + writeJavaSource( + "Example", + """ + import de.thetaphi.forbiddenapis.SuppressForbidden; + + public class Example { + @SuppressForbidden + public void exit() { System.exit(0); } + } + """ + ) + + val suppressed = run("check") + + assertThat(suppressed.task(":forbiddenApisMain")?.outcome) + .describedAs(suppressed.output).isEqualTo(TaskOutcome.SUCCESS) + assertThat(suppressed.task(":forbiddenApisTest")?.outcome).isEqualTo(TaskOutcome.SKIPPED) + assertThat(suppressed.task(":check")?.outcome).isEqualTo(TaskOutcome.SUCCESS) + } +} diff --git a/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt b/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt new file mode 100644 index 00000000000..559fd9a9465 --- /dev/null +++ b/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt @@ -0,0 +1,169 @@ +package datadog.gradle.plugin.spotbugs + +import datadog.gradle.plugin.GradleFixture +import org.assertj.core.api.Assertions.assertThat +import org.gradle.testkit.runner.TaskOutcome.FAILED +import org.gradle.testkit.runner.TaskOutcome.SKIPPED +import org.gradle.testkit.runner.TaskOutcome.SUCCESS +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource + +class SpotBugsConventionsPluginTest : GradleFixture() { + @BeforeEach + fun setup() { + writeSettings("""rootProject.name = "spotbugs-convention"""") + writeFile( + "gradle/libs.versions.toml", + """ + [libraries] + spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version = "4.10.4" } + jsr305 = { module = "com.google.code.findbugs:jsr305", version = "3.0.2" } + """ + ) + writeFile("gradle/spotbugFilters/exclude.xml", "") + writeRootProject( + """ + import com.github.spotbugs.snom.SpotBugsTask + + plugins { id("dd-trace-java.conventions.spotbugs") } + allprojects { repositories { mavenCentral() } } + tasks.withType().configureEach { options.release = 8 } + """ + ) + } + + @ParameterizedTest + @ValueSource(strings = ["main", "main_java6", "main_java11", "main_java17", "main_java21", "main_java25"]) + fun `production violations fail check and appear in the HTML report`(sourceSet: String) { + writeRootProject("""sourceSets.maybeCreate("$sourceSet")""", append = true) + writeViolation(sourceSet) + + val result = run("check", expectFailure = true) + val taskName = "spotbugs" + sourceSet.replaceFirstChar(Char::uppercaseChar) + + assertThat(result.task(":$taskName")?.outcome).describedAs(result.output).isEqualTo(FAILED) + assertThat(buildFile("reports/spotbugs/$taskName.html")) + .exists().content().contains("ES_COMPARING_PARAMETER_STRING_WITH_EQ") + } + + @Test + fun `test benchmark and smoke sources are not analysed`() { + val sourceSets = listOf("test", "integrationTest", "test_java17", "jmh", "otherMain") + sourceSets.forEach { + writeRootProject("""sourceSets.maybeCreate("$it")""", append = true) + writeViolation(it) + } + addSubproject( + "dd-smoke-tests:application", + """ + plugins { id("dd-trace-java.conventions.spotbugs") } + sourceSets.create("main_java17") + """ + ) + writeViolation("main", ":dd-smoke-tests:application") + writeViolation("main_java17", ":dd-smoke-tests:application") + val taskPaths = sourceSets.map { ":spotbugs" + it.replaceFirstChar(Char::uppercaseChar) } + + listOf( + ":dd-smoke-tests:application:spotbugsMain", + ":dd-smoke-tests:application:spotbugsMain_java17" + ) + + val result = run(*taskPaths.toTypedArray()) + + taskPaths.forEach { + assertThat(result.task(it)?.outcome).describedAs("$it\n${result.output}").isEqualTo(SKIPPED) + } + } + + @Test + fun `module analysis exclusions apply to existing and later production tasks`() { + writeRootProject( + """ + tasks.named("spotbugsMain").get() + spotbugs { onlyAnalyze = listOf("none") } + sourceSets.create("main_java11") + """, + append = true + ) + writeViolation("main") + writeViolation("main_java11") + + val result = run("spotbugsMain", "spotbugsMain_java11") + + listOf("spotbugsMain", "spotbugsMain_java11").forEach { + assertThat(result.task(":$it")?.outcome).describedAs(result.output).isEqualTo(SUCCESS) + assertThat(buildFile("reports/spotbugs/$it.html")) + .exists().content().doesNotContain("ES_COMPARING_PARAMETER_STRING_WITH_EQ") + } + } + + @Test + fun `test rules can change independently of production rules`() { + writeRootProject( + """ + tasks.named("spotbugsTest") { + enabled = true + visitors = listOf("FindNullDeref") + } + """, + append = true + ) + writeViolation("main") + writeJavaSource( + "example.NullDereference", + """ + package example; + public class NullDereference { + public int length() { + String value = null; + return value.length(); + } + } + """, + sourceSet = "test" + ) + + val nullCheck = run("spotbugsMain", "spotbugsTest", "--continue", expectFailure = true) + + assertThat(nullCheck.task(":spotbugsMain")?.outcome).describedAs(nullCheck.output).isEqualTo(FAILED) + assertThat(nullCheck.task(":spotbugsTest")?.outcome).describedAs(nullCheck.output).isEqualTo(FAILED) + assertThat(buildFile("reports/spotbugs/spotbugsTest.html")) + .exists().content().contains("NP_ALWAYS_NULL") + + writeRootProject( + """ + tasks.named("spotbugsTest") { + visitors = listOf("FindOpenStream") + } + """, + append = true + ) + + val resourceCheck = run("spotbugsMain", "spotbugsTest", "--continue", expectFailure = true) + + assertThat(resourceCheck.task(":spotbugsMain")?.outcome) + .describedAs(resourceCheck.output).isEqualTo(FAILED) + assertThat(resourceCheck.task(":spotbugsTest")?.outcome) + .describedAs(resourceCheck.output).isEqualTo(SUCCESS) + assertThat(buildFile("reports/spotbugs/spotbugsTest.html")) + .exists().content().doesNotContain("NP_ALWAYS_NULL") + } + + private fun writeViolation(sourceSet: String, projectPath: String? = null) { + writeJavaSource( + "example.StringComparison", + """ + package example; + public class StringComparison { + public boolean same(String left, String right) { + return left == right; + } + } + """, + sourceSet, + projectPath + ) + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 3391e29ab8c..2905edf2053 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -95,7 +95,7 @@ forbiddenapis = { module = "de.thetaphi:forbiddenapis", version.ref = "forbidden gradle-tooling-api = { module = "org.gradle:gradle-tooling-api", version.ref = "gradle-tooling-api" } jsr305 = { module = "com.google.code.findbugs:jsr305", version.ref = "jsr305" } spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs_annotations" } -spotbugs = { module = "com.github.spotbugs.snom:spotbugs-gradle-plugin", version.ref = "spotbugs-gradle-plugin" } +spotbugs-gradle-plugin = { module = "com.github.spotbugs.snom:spotbugs-gradle-plugin", version.ref = "spotbugs-gradle-plugin" } # DataDog libs and forks datadog-okhttp = { module = "com.datadoghq.okhttp3:okhttp", version.ref = "datadog-okhttp" } From 491fc4302344fdcc9b6194cd1ac70ff0804b8fc0 Mon Sep 17 00:00:00 2001 From: Brice Dutheil Date: Tue, 20 Oct 2026 16:43:58 +0200 Subject: [PATCH 4/5] fix: correct JDK tunneling socket reads Enabling SpotBugs on main_java17 exposed two rules: - AT_STALE_THREAD_WRITE_OF_PRIMITIVE: the send and receive buffer sizes are shared without synchronization. Make them volatile so readers observe updates to these independent settings. - NCR_NOT_PROPERLY_CHECKED_READ: read() assumes every result except -1 filled its byte. A zero-byte read on timeout fabricated a NUL byte. Wait for data, EOF or an exception. Throw SocketTimeoutException when the read timeout expires. Retry selector wakeups within the original timeout and stop interrupted reads so the retry loop cannot spin. Inspecting the delegated bulk read also exposed byte loss: it could consume more bytes from the channel than requested and discard the rest. Limit the channel read to the requested length and handle empty reads. Suppressing the rules would retain real visibility and stream-contract bugs. Add coverage for byte preservation, EOF, empty reads, timeout, selector wakeup and interruption while preserving asynchronous close. --- .../common/socket/TunnelingJdkSocket.java | 75 ++++++++++++------- .../common/socket/TunnelingJdkSocketTest.java | 56 +++++++++++--- 2 files changed, 93 insertions(+), 38 deletions(-) diff --git a/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java b/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java index a0487a4e944..355ffcebffb 100644 --- a/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java +++ b/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java @@ -3,12 +3,14 @@ import datadog.trace.api.internal.VisibleForTesting; import java.io.IOException; import java.io.InputStream; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.Socket; import java.net.SocketAddress; import java.net.SocketException; +import java.net.SocketTimeoutException; import java.net.StandardProtocolFamily; import java.net.StandardSocketOptions; import java.net.UnixDomainSocketAddress; @@ -21,6 +23,7 @@ import java.nio.file.Path; import java.util.Iterator; import java.util.Set; +import java.util.concurrent.TimeUnit; /** * Subtype UNIX socket for a higher-fidelity impersonation of TCP sockets. This is named "tunneling" @@ -43,8 +46,8 @@ final class TunnelingJdkSocket extends Socket { static final int DEFAULT_BUFFER_SIZE = 8192; // Indicate that the buffer size is not set by initializing to -1 - private int sendBufferSize = -1; - private int receiveBufferSize = -1; + private volatile int sendBufferSize = -1; + private volatile int receiveBufferSize = -1; TunnelingJdkSocket(final Path path) throws IOException, UnsupportedOperationException { this.unixSocketAddress = UnixDomainSocketAddress.of(path); @@ -236,46 +239,66 @@ public InputStream getInputStream() throws IOException { @Override public int read() throws IOException { byte[] nextByte = new byte[1]; - return (read(nextByte, 0, 1) == -1) ? -1 : (nextByte[0] & 0xFF); + return (read(nextByte, 0, 1) == 1) ? (nextByte[0] & 0xFF) : -1; } @Override public int read(byte[] b, int off, int len) throws IOException { + if (off < 0 || len < 0 || off > b.length - len) { + throw new IndexOutOfBoundsException(); + } + if (len == 0) { + return 0; + } if (isInputShutdown()) { return -1; } buffer.clear(); + buffer.limit(Math.min(buffer.capacity(), len)); + int readTimeout = timeout; + long start = System.nanoTime(); + long remaining = readTimeout; try { - int readyChannels = readSelector.select(timeout); - if (readyChannels == 0) { - if (isClosed() || !readSelector.isOpen()) { + while (true) { + int readyChannels = readSelector.select(remaining); + if (readyChannels == 0 && (isClosed() || !readSelector.isOpen())) { throw new SocketException("Socket is closed"); } - return 0; - } - - Set selectedKeys = readSelector.selectedKeys(); - // Multiple input streams share this selector, so serialize iteration and removal from - // its non-thread-safe selected-key set. - synchronized (selectedKeys) { - Iterator keyIterator = selectedKeys.iterator(); - while (keyIterator.hasNext()) { - SelectionKey key = keyIterator.next(); - keyIterator.remove(); - if (key.isReadable()) { - int r = unixSocketChannel.read(buffer); - if (r == -1) { - return -1; + if (readyChannels > 0) { + Set selectedKeys = readSelector.selectedKeys(); + // Multiple input streams share this selector, so serialize iteration and removal + // from its non-thread-safe selected-key set. + synchronized (selectedKeys) { + Iterator keyIterator = selectedKeys.iterator(); + while (keyIterator.hasNext()) { + SelectionKey key = keyIterator.next(); + keyIterator.remove(); + if (key.isReadable()) { + int r = unixSocketChannel.read(buffer); + if (r == -1) { + return -1; + } + if (r > 0) { + buffer.flip(); + buffer.get(b, off, r); + return r; + } + } } - buffer.flip(); - len = Math.min(r, len); - buffer.get(b, off, len); - return len; + } + } + if (Thread.currentThread().isInterrupted()) { + throw new InterruptedIOException("Read interrupted"); + } + // Selection can wake without data; retry within the original timeout. + if (readTimeout > 0) { + remaining = readTimeout - TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + if (remaining <= 0) { + throw new SocketTimeoutException("Read timed out"); } } } - return 0; } catch (ClosedSelectorException | CancelledKeyException e) { SocketException socketException = new SocketException("Socket is closed"); socketException.initCause(e); diff --git a/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java b/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java index ef6741e35bb..ac0757b4e64 100644 --- a/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java +++ b/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java @@ -7,16 +7,18 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.condition.JRE.JAVA_16; import java.io.IOException; import java.io.InputStream; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.InetSocketAddress; import java.net.SocketException; +import java.net.SocketTimeoutException; import java.net.StandardProtocolFamily; import java.net.UnixDomainSocketAddress; +import java.nio.ByteBuffer; import java.nio.channels.CancelledKeyException; import java.nio.channels.ClosedSelectorException; import java.nio.channels.SelectableChannel; @@ -168,39 +170,69 @@ public void testTimeout() throws Exception { clientSocket.setSoTimeout(testTimeout); assertEquals(testTimeout, clientSocket.getSoTimeout()); + clientSocket.selector.wakeup(); long startTime = System.currentTimeMillis(); - int readResult = inputStream.read(); + assertThrows(SocketTimeoutException.class, inputStream::read); long endTime = System.currentTimeMillis(); long readDuration = endTime - startTime; int timeVariance = 100; assertTrue(readDuration >= testTimeout && readDuration <= testTimeout + timeVariance); - assertEquals(0, readResult); int newTimeout = testTimeout / 2; clientSocket.setSoTimeout(newTimeout); assertEquals(newTimeout, clientSocket.getSoTimeout()); - assertTimeoutPreemptively(Duration.ofMillis(testTimeout), () -> inputStream.read()); + assertTimeoutPreemptively( + Duration.ofMillis(testTimeout), + () -> assertThrows(SocketTimeoutException.class, () -> inputStream.read(new byte[1]))); + + int invalidTimeout = -1; + assertThrows(IllegalArgumentException.class, () -> clientSocket.setSoTimeout(invalidTimeout)); // The socket should block indefinitely when timeout is set to 0, per // https://docs.oracle.com/en/java/javase/16/docs/api//java.base/java/net/Socket.html#setSoTimeout(int). int infiniteTimeout = 0; clientSocket.setSoTimeout(infiniteTimeout); assertEquals(infiniteTimeout, clientSocket.getSoTimeout()); + ExecutorService reader = Executors.newSingleThreadExecutor(); try { - assertTimeoutPreemptively(Duration.ofMillis(testTimeout), () -> inputStream.read()); - fail("Read should block indefinitely with infinite timeout"); - } catch (AssertionError e) { - // Expected + clientSocket.selector.wakeup(); + Future read = reader.submit(() -> inputStream.read()); + assertThrows(TimeoutException.class, () -> read.get(testTimeout, TimeUnit.MILLISECONDS)); + reader.shutdownNow(); + assertInstanceOf( + InterruptedIOException.class, + assertThrows(ExecutionException.class, () -> read.get(5, TimeUnit.SECONDS)).getCause()); + } finally { + reader.shutdownNow(); + clientSocket.close(); + assertTrue(reader.awaitTermination(5, TimeUnit.SECONDS)); } - int invalidTimeout = -1; - assertThrows(IllegalArgumentException.class, () -> clientSocket.setSoTimeout(invalidTimeout)); - - clientSocket.close(); assertThrows(SocketException.class, () -> clientSocket.setSoTimeout(testTimeout)); assertThrows(SocketException.class, clientSocket::getSoTimeout); } + @Test + public void testSingleByteReadsPreserveRemainingData() throws Exception { + try (TunnelingJdkSocket clientSocket = createClient()) { + server.serverThread.join(TimeUnit.SECONDS.toMillis(5)); + SocketChannel peer = server.acceptedChannel.get(); + assertNotNull(peer); + ByteBuffer bytes = ByteBuffer.wrap(new byte[] {0, (byte) 255}); + while (bytes.hasRemaining()) { + peer.write(bytes); + } + peer.shutdownOutput(); + + clientSocket.setSoTimeout(1000); + InputStream inputStream = clientSocket.getInputStream(); + assertEquals(0, inputStream.read(new byte[0])); + assertEquals(0, inputStream.read()); + assertEquals(255, inputStream.read()); + assertEquals(-1, inputStream.read()); + } + } + @Test public void testBufferSizes() throws Exception { TunnelingJdkSocket clientSocket = createClient(); From b34800f0a5af387eb5caeac33d0ffc61a900d085 Mon Sep 17 00:00:00 2001 From: Brice Dutheil Date: Tue, 20 Oct 2026 16:44:20 +0200 Subject: [PATCH 5/5] fix: restrict access to the Karate test events handler Enabling SpotBugs on main_java21 exposed PA_PUBLIC_PRIMITIVE_ATTRIBUTE on TestEventsHandlerHolder.TEST_EVENTS_HANDLER. Only KarateTracingListener and ExecutionContext read this field. Both are helpers in the same package and are injected with the holder into the same class loader. Inlined advice does not access the field, and tests use the public start() and stop() methods. Make the field package-private. Public access is unnecessary, so reduce its visibility without a SuppressFBWarnings annotation or an additional accessor. Keep volatile publication unchanged. --- .../trace/instrumentation/karate2/TestEventsHandlerHolder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java b/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java index d2c4c495cab..c37323ec15d 100644 --- a/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java +++ b/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java @@ -8,7 +8,7 @@ public abstract class TestEventsHandlerHolder { - public static volatile TestEventsHandler TEST_EVENTS_HANDLER; + static volatile TestEventsHandler TEST_EVENTS_HANDLER; static { start();