mirror of
https://github.com/apache/kafka.git
synced 2026-10-28 08:24:58 +08:00
## Summary Follow-up nits from @chia7712's review of #21337. None affects checker behaviour; grouped as a single small PR. JIRA: [KAFKA-20779](https://issues.apache.org/jira/browse/KAFKA-20779) ## Changes | # | File | Change | |---|------|--------| | 1 | `core/…/PublicApiViolation.java` | `equals` uses `Objects.equals(memberName, …)` instead of the ternary null-check. | | 2 | `core/…/CascadeValidator.java` | `asSuppression` omits `#member` when the finding has no `memberName`, so class-scope `INVALID_SUPERTYPE` suppressions no longer render as `…Owner#null`. Also uses ASCII hyphens in the description separator. | | 3 | `maven-plugin/…/KafkaInternalApiCheckerMojo.java` | Doc comments + `@param` tags on the class default ctor + six setters; `:maven-plugin:javadoc` is now warning-free. | | 4 | `api-checker/build.gradle` | Adds `testLogging { events "passed", "skipped", "failed" }` for consistency with the main Kafka build. | | 5 | `api-checker/README.md` | Documents `:maven-plugin:test`; layout tree shows the test folder. | Hooking `:api-checker:*:check` into the root `check` was dropped from this PR per review — Kafka's CI relies on `kafka.test.xml.output.dir` + `copyTestXml` to collect outputs from composite-included builds, so that wiring needs a proper follow-up rather than being folded in here. ## Test plan - [x] `./gradlew :api-checker:core:check :api-checker:gradle-plugins:check :api-checker:maven-plugin:check` — clean; all api-checker verification tasks pass. - [x] `./gradlew :api-checker:maven-plugin:javadoc` — zero warnings. Reviewers: Chia-Ping Tsai <chia7712@gmail.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
1 parent
971c6b4131
commit
de8482a4e2
5 files changed
+67
-11
No files matched your search
+13
-6
@@ -25,11 +25,16 @@ below cover building, testing, and publishing the plugins themselves.
|
||||
## Test
|
||||
|
||||
```bash
|
||||
./gradlew :api-checker:core:test :api-checker:gradle-plugins:test
|
||||
./gradlew :api-checker:core:test :api-checker:gradle-plugins:test :api-checker:maven-plugin:test
|
||||
```
|
||||
|
||||
`:gradle-plugins:test` includes a Gradle TestKit end-to-end test that applies the
|
||||
`org.apache.kafka.internal-api-checker` plugin to a synthetic consumer project.
|
||||
- `:core:test` — unit tests for the scanner, validators, and reporter, plus the shared
|
||||
`testFixtures` (`AsmClassFactory`, `TempJarBuilder`) they and the plugin tests use.
|
||||
- `:gradle-plugins:test` — includes a Gradle TestKit end-to-end test that applies the
|
||||
`org.apache.kafka.internal-api-checker` plugin to a synthetic consumer project.
|
||||
- `:maven-plugin:test` — hosts `PluginXmlParityTest`, which locks the generated
|
||||
`plugin.xml` Mojo descriptor against the fields on `KafkaInternalApiCheckerMojo` so
|
||||
adding a parameter without exposing it (or vice versa) fails locally.
|
||||
|
||||
## Publish
|
||||
|
||||
@@ -84,7 +89,9 @@ api-checker/
|
||||
│ └── src/{main,test}/java/.../gradle/ # Plugin/Task/Extension × 2
|
||||
└── maven-plugin/
|
||||
├── build.gradle # Maven deps; templates plugin.xml at processResources
|
||||
└── src/main/
|
||||
├── java/.../maven/KafkaInternalApiCheckerMojo.java
|
||||
└── resources/META-INF/maven/plugin.xml
|
||||
└── src/
|
||||
├── main/
|
||||
│ ├── java/.../maven/KafkaInternalApiCheckerMojo.java
|
||||
│ └── resources/META-INF/maven/plugin.xml
|
||||
└── test/java/.../maven/PluginXmlParityTest.java # locks plugin.xml ↔ Mojo fields
|
||||
```
|
||||
@@ -80,6 +80,9 @@ subprojects {
|
||||
|
||||
test {
|
||||
useJUnitPlatform()
|
||||
testLogging {
|
||||
events "passed", "skipped", "failed"
|
||||
}
|
||||
}
|
||||
|
||||
checkstyle {
|
||||
|
||||
@@ -444,10 +444,16 @@ final class CascadeValidator {
|
||||
private static PublicApiViolation asSuppression(PublicApiViolation original, String reason) {
|
||||
boolean noReason = reason.isEmpty();
|
||||
String prettyReason = noReason ? PublicApiViolation.NO_REASON_MARKER : reason;
|
||||
// Class-level cascade findings (e.g. INVALID_SUPERTYPE) carry a null memberName; only
|
||||
// append "#member" when we actually have one, so class-scope suppressions render as
|
||||
// "Suppressed INVALID_SUPERTYPE in org.apache.kafka…Owner" rather than "…Owner#null".
|
||||
String location = original.getMemberName() != null
|
||||
? original.getClassName() + "#" + original.getMemberName()
|
||||
: original.getClassName();
|
||||
String description = "Suppressed " + original.getViolationType() + " in "
|
||||
+ original.getClassName() + "#" + original.getMemberName()
|
||||
+ " — " + original.getDescription()
|
||||
+ " — reason: " + prettyReason;
|
||||
+ location
|
||||
+ " - " + original.getDescription()
|
||||
+ " - reason: " + prettyReason;
|
||||
return new PublicApiViolation(original.getClassName(), "SUPPRESSED",
|
||||
description, original.getMemberName(), noReason);
|
||||
}
|
||||
|
||||
@@ -16,6 +16,8 @@
|
||||
*/
|
||||
package org.apache.kafka.apicheck;
|
||||
|
||||
import java.util.Objects;
|
||||
|
||||
/**
|
||||
* Represents a violation of the public API rules.
|
||||
*/
|
||||
@@ -86,7 +88,7 @@ public class PublicApiViolation {
|
||||
if (!className.equals(that.className)) return false;
|
||||
if (!violationType.equals(that.violationType)) return false;
|
||||
if (!description.equals(that.description)) return false;
|
||||
return memberName != null ? memberName.equals(that.memberName) : that.memberName == null;
|
||||
return Objects.equals(memberName, that.memberName);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+39
-1
@@ -49,6 +49,10 @@ import java.util.List;
|
||||
threadSafe = true)
|
||||
public class KafkaInternalApiCheckerMojo extends AbstractMojo {
|
||||
|
||||
/** Public no-arg constructor invoked by Maven when instantiating the Mojo. */
|
||||
public KafkaInternalApiCheckerMojo() {
|
||||
}
|
||||
|
||||
/**
|
||||
* The Maven project.
|
||||
*/
|
||||
@@ -199,27 +203,61 @@ public class KafkaInternalApiCheckerMojo extends AbstractMojo {
|
||||
return kafkaJars;
|
||||
}
|
||||
|
||||
// Getters and setters for testing
|
||||
// Setters used by tests to construct the Mojo without going through the Maven container.
|
||||
|
||||
/**
|
||||
* Sets the Maven project the Mojo runs against (test-only injection).
|
||||
*
|
||||
* @param project the Maven project to scan
|
||||
*/
|
||||
public void setProject(MavenProject project) {
|
||||
this.project = project;
|
||||
}
|
||||
|
||||
/**
|
||||
* Enables or disables the checker for one invocation.
|
||||
*
|
||||
* @param enabled {@code true} to run the checker, {@code false} to skip
|
||||
*/
|
||||
public void setEnabled(boolean enabled) {
|
||||
this.enabled = enabled;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sets whether reported violations fail the Maven build.
|
||||
*
|
||||
* @param failOnViolation {@code true} to fail the build on any violation
|
||||
*/
|
||||
public void setFailOnViolation(boolean failOnViolation) {
|
||||
this.failOnViolation = failOnViolation;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sets whether the checker fails when no {@code org.apache.kafka:*} dependency is present.
|
||||
*
|
||||
* @param failOnNoKafkaDependency {@code true} to fail (rather than warn) when the project has
|
||||
* no Kafka artifact on its classpath
|
||||
*/
|
||||
public void setFailOnNoKafkaDependency(boolean failOnNoKafkaDependency) {
|
||||
this.failOnNoKafkaDependency = failOnNoKafkaDependency;
|
||||
}
|
||||
|
||||
/**
|
||||
* Overrides the classes/jars scanned. Each entry may be a class directory, an individual
|
||||
* {@code .class} file, or a {@code .jar} archive.
|
||||
*
|
||||
* @param classesDirectories roots to scan; if {@code null} or empty, defaults to the project's
|
||||
* main compiled output
|
||||
*/
|
||||
public void setClassesDirectories(List<File> classesDirectories) {
|
||||
this.classesDirectories = classesDirectories;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sets the file path where the text report is written.
|
||||
*
|
||||
* @param reportFile destination file for the human-readable report
|
||||
*/
|
||||
public void setReportFile(File reportFile) {
|
||||
this.reportFile = reportFile;
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user