KAFKA-18955: Fix infinite loop and standardize options in MetadataSchemaCheckerTool (apache#19165)

ahuang98 · web-flow · commit 0ce87e723649 · 2025-03-17T14:00:27.000-07:00
- fix infinite loop in FieldSpecPairIterator.java 
- fix bug in Unifier.java that was resulting in verify-evolution and
verify-evolution-git to break in certain scenarios and probably generate
false positives
- verify-evolution-git command takes an absolute path like the other
commands
- verify-evolution arguments are more clear (path and parent_path)

Reviewers: Reviewers: Josep Prat &lt;josep.prat@aiven.io&gt;, mannoopj &lt;139923522+mannoopj@users.noreply.github.com&gt;, Colin P. McCabe &lt;cmccabe@apache.org&gt;
diff --git a/generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java b/generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java
@@ -121,18 +121,34 @@ static MessageSpec readMessageSpecFromString(String contents) {
     }
 
     /**
-     * Read a MessageSpec file from remote git repo.
+     * Read the file from the specified git reference.
      *
-     * @param filePath The file to read from remote git repo.
-     * @param ref The specific git reference to be used for testing.
+     * @param filePath The fully qualified file path. The git directory will be derived from this.
+     * @param gitRef The specific git reference to be used for comparison.
      * @return The file contents.
      */
-    static String getDataFromGit(String filePath, Path gitPath, String ref) throws IOException {
-        Git git = Git.open(new File(gitPath + "/.git"));
+    static String readFileFromGitRef(String filePath, String gitRef) throws IOException {
+        Path fileAbsolutePath = Paths.get(filePath).toAbsolutePath();
+
+        // traverse up parent directories until .git directory is found
+        Path projectRoot = fileAbsolutePath.getParent();
+        if (projectRoot == null) {
+            throw new RuntimeException("The file path provided does not have a parent directory");
+        }
+        while (!Files.exists(projectRoot.resolve(".git"))) {
+            projectRoot = projectRoot.getParent();
+            if (projectRoot == null) {
+                throw new RuntimeException("Invalid path, need to be within a Git repository");
+            }
+        }
+
+        String pathFromProjectRoot = projectRoot.relativize(fileAbsolutePath).toString();
+
+        Git git = Git.open(Paths.get(projectRoot.toString(), ".git").toFile());
         Repository repository = git.getRepository();
-        Ref head = repository.getRefDatabase().findRef(ref);
+        Ref head = repository.getRefDatabase().findRef(gitRef);
         if (head == null) {
-            throw new IllegalStateException("Cannot find " + ref + " in the repository.");
+            throw new IllegalStateException("Cannot find " + gitRef + " in the repository.");
         }
 
         try (RevWalk revWalk = new RevWalk(repository)) {
@@ -141,9 +157,9 @@ static String getDataFromGit(String filePath, Path gitPath, String ref) throws I
             try (TreeWalk treeWalk = new TreeWalk(repository)) {
                 treeWalk.addTree(tree);
                 treeWalk.setRecursive(true);
-                treeWalk.setFilter(PathFilter.create(String.valueOf(Paths.get(filePath.substring(1)))));
+                treeWalk.setFilter(PathFilter.create(String.valueOf(Paths.get(pathFromProjectRoot))));
                 if (!treeWalk.next()) {
-                    throw new IllegalStateException("Did not find expected file " + filePath.substring(1));
+                    throw new IllegalStateException("Did not find expected file " + pathFromProjectRoot);
                 }
                 ObjectId objectId = treeWalk.getObjectId(0);
                 ObjectLoader loader = repository.open(objectId);
diff --git a/generator/src/main/java/org/apache/kafka/message/checker/FieldSpecPairIterator.java b/generator/src/main/java/org/apache/kafka/message/checker/FieldSpecPairIterator.java
@@ -72,6 +72,7 @@ public boolean hasNext() {
                                     "message2, but should not be, based on its versions.");
                         }
                     }
+                    field1 = iterator1.hasNext() ? iterator1.next() : null;
                     break;
                 }
                 case MESSAGE2_ONLY:
diff --git a/generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java b/generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java
@@ -25,11 +25,8 @@
 import net.sourceforge.argparse4j.internal.HelpScreenException;
 
 import java.io.PrintStream;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.Paths;
 
-import static org.apache.kafka.message.checker.CheckerUtils.getDataFromGit;
+import static org.apache.kafka.message.checker.CheckerUtils.readFileFromGitRef;
 
 public class MetadataSchemaCheckerTool {
     public static void main(String[] args) throws Exception {
@@ -54,18 +51,18 @@ public static void run(
             required(true).
             help("The path to a schema JSON file.");
         Subparser evolutionVerifierParser = subparsers.addParser("verify-evolution").
-            help("Verify that an evolution of a JSON file is valid.");
-        evolutionVerifierParser.addArgument("--path1", "-1").
+            help("Verify that a schema JSON file is a valid evolution of a parent schema.");
+        evolutionVerifierParser.addArgument("--path", "-1").
             required(true).
-            help("The initial schema JSON path.");
-        evolutionVerifierParser.addArgument("--path2", "-2").
+            help("The path to a schema JSON file.");
+        evolutionVerifierParser.addArgument("--parent_path", "-2").
             required(true).
-            help("The final schema JSON path.");
+            help("The path to the parent schema JSON file.");
         Subparser evolutionGitVerifierParser = subparsers.addParser("verify-evolution-git").
-            help(" Verify that an evolution of a JSON file is valid using git.");
-        evolutionGitVerifierParser.addArgument("--file", "-3").
+            help("Verify that a schema JSON file is a valid evolution of your local git master branch.");
+        evolutionGitVerifierParser.addArgument("--path", "-3").
             required(true).
-            help("The edited JSON file");
+            help("The path to your edited JSON file");
         evolutionGitVerifierParser.addArgument("--ref", "-4")
             .required(false)
             .setDefault("refs/heads/trunk")
@@ -85,31 +82,24 @@ public static void run(
                 break;
             }
             case "verify-evolution": {
-                String path1 = namespace.getString("path1");
-                String path2 = namespace.getString("path2");
+                String child = namespace.getString("path");
+                String parent = namespace.getString("parent_path");
                 EvolutionVerifier verifier = new EvolutionVerifier(
-                    CheckerUtils.readMessageSpecFromFile(path1),
-                    CheckerUtils.readMessageSpecFromFile(path2));
+                    CheckerUtils.readMessageSpecFromFile(parent),
+                    CheckerUtils.readMessageSpecFromFile(child));
                 verifier.verify();
-                writer.println("Successfully verified evolution of path1: " + path1 +
-                        ", and path2: " + path2);
+                writer.println("Successfully verified evolution of path: " + child +
+                        " from parent: " + parent);
                 break;
             }
             case "verify-evolution-git": {
-                String filePath = "/metadata/src/main/resources/common/metadata/" + namespace.getString("file");
-                Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
-                while (!Files.exists(rootKafkaDirectory.resolve(".git"))) {
-                    rootKafkaDirectory = rootKafkaDirectory.getParent();
-                    if (rootKafkaDirectory == null) {
-                        throw new RuntimeException("Invalid directory, need to be within a Git repository");
-                    }
-                }
-                String gitContent = getDataFromGit(filePath, rootKafkaDirectory, namespace.getString("ref"));
+                String path = namespace.getString("path");
+                String gitContent = readFileFromGitRef(path, namespace.getString("ref"));
                 EvolutionVerifier verifier = new EvolutionVerifier(
-                        CheckerUtils.readMessageSpecFromFile(rootKafkaDirectory + filePath),
-                        CheckerUtils.readMessageSpecFromString(gitContent));
+                    CheckerUtils.readMessageSpecFromFile(path),
+                    CheckerUtils.readMessageSpecFromString(gitContent));
                 verifier.verify();
-                writer.println("Successfully verified evolution of file: " + namespace.getString("file"));
+                writer.println("Successfully verified evolution of file: " + namespace.getString("path"));
                 break;
             }
             default:
diff --git a/generator/src/main/java/org/apache/kafka/message/checker/Unifier.java b/generator/src/main/java/org/apache/kafka/message/checker/Unifier.java
@@ -51,7 +51,7 @@ class Unifier {
         this.structRegistry1.register(topLevelMessage1);
         this.topLevelMessage2 = topLevelMessage2;
         this.structRegistry2 = new StructRegistry();
-        this.structRegistry1.register(topLevelMessage2);
+        this.structRegistry2.register(topLevelMessage2);
     }
 
     static FieldSpec structSpecToFieldSpec(StructSpec structSpec) {
diff --git a/generator/src/test/java/org/apache/kafka/message/checker/MetadataSchemaCheckerToolTest.java b/generator/src/test/java/org/apache/kafka/message/checker/MetadataSchemaCheckerToolTest.java
@@ -21,6 +21,9 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 
 import static org.apache.kafka.message.checker.CheckerTestUtils.messageSpecStringToTempFile;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -29,13 +32,21 @@ public class MetadataSchemaCheckerToolTest {
     @Test
     public void testVerifyEvolutionGit() throws Exception {
         try (ByteArrayOutputStream stream = new ByteArrayOutputStream()) {
+            Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
+            while (!Files.exists(rootKafkaDirectory.resolve(".git"))) {
+                rootKafkaDirectory = rootKafkaDirectory.getParent();
+                if (rootKafkaDirectory == null) {
+                    throw new RuntimeException("Invalid directory, need to be within a Git repository");
+                }
+            }
+            Path schemaPath = rootKafkaDirectory.resolve("metadata/src/main/resources/common/metadata/AbortTransactionRecord.json");
             MetadataSchemaCheckerTool.run(
                 // In the CI environment because the CI fetch command only creates HEAD and refs/remotes/pull/... references.
                 // Since there may not be other branches like refs/heads/trunk in CI, HEAD serves as the baseline reference.
-                new String[]{"verify-evolution-git", "--file", "AbortTransactionRecord.json", "--ref", "HEAD"},
+                new String[]{"verify-evolution-git", "--path", schemaPath.toString(), "--ref", "HEAD"},
                 new PrintStream(stream)
             );
-            assertEquals("Successfully verified evolution of file: AbortTransactionRecord.json",
+            assertEquals("Successfully verified evolution of file: " + schemaPath,
                 stream.toString().trim());
         }
     }
@@ -60,8 +71,8 @@ public void testSuccessfulVerifyEvolution() throws Exception {
                 "'validVersions': '0-2', 'flexibleVersions': '0+', " +
                 "'fields': [{'name': 'BrokerId', 'type': 'int32', 'versions': '0+'}]}");
             MetadataSchemaCheckerTool.run(new String[] {"verify-evolution",
-                "--path1", path, "--path2", path}, new PrintStream(stream));
-            assertEquals("Successfully verified evolution of path1: " + path + ", and path2: " + path,
+                "--path", path, "--parent_path", path}, new PrintStream(stream));
+            assertEquals("Successfully verified evolution of path: " + path + " from parent: " + path,
                 stream.toString().trim());
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -72,6 +72,7 @@ public boolean hasNext() {`
`72`	`72`	`"message2, but should not be, based on its versions.");`
`73`	`73`	`}`
`74`	`74`	`}`
	`75`	`+ field1 = iterator1.hasNext() ? iterator1.next() : null;`
`75`	`76`	`break;`
`76`	`77`	`}`
`77`	`78`	`case MESSAGE2_ONLY:`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ class Unifier {`
`51`	`51`	`this.structRegistry1.register(topLevelMessage1);`
`52`	`52`	`this.topLevelMessage2 = topLevelMessage2;`
`53`	`53`	`this.structRegistry2 = new StructRegistry();`
`54`		`- this.structRegistry1.register(topLevelMessage2);`
	`54`	`+ this.structRegistry2.register(topLevelMessage2);`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`static FieldSpec structSpecToFieldSpec(StructSpec structSpec) {`