Skip to content

Commit 0ce87e7

Browse files
ahuang98ahuang98
authored
KAFKA-18955: Fix infinite loop and standardize options in MetadataSchemaCheckerTool (#19165)
- 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 <[email protected]>, mannoopj <[email protected]>, Colin P. McCabe <[email protected]>
1 parent e9ffe0b commit 0ce87e7

File tree

5 files changed

+62
-44
lines changed

5 files changed

+62
-44
lines changed

‎generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -121,18 +121,34 @@ static MessageSpec readMessageSpecFromString(String contents) {
121121
}
122122

123123
/**
124-
* Read a MessageSpec file from remote git repo.
124+
* Read the file from the specified git reference.
125125
*
126-
* @param filePath The file to read from remote git repo.
127-
* @param ref The specific git reference to be used for testing.
126+
* @param filePath The fully qualified file path. The git directory will be derived from this.
127+
* @param gitRef The specific git reference to be used for comparison.
128128
* @return The file contents.
129129
*/
130-
static String getDataFromGit(String filePath, Path gitPath, String ref) throws IOException {
131-
Git git = Git.open(new File(gitPath + "/.git"));
130+
static String readFileFromGitRef(String filePath, String gitRef) throws IOException {
131+
Path fileAbsolutePath = Paths.get(filePath).toAbsolutePath();
132+
133+
// traverse up parent directories until .git directory is found
134+
Path projectRoot = fileAbsolutePath.getParent();
135+
if (projectRoot == null) {
136+
throw new RuntimeException("The file path provided does not have a parent directory");
137+
}
138+
while (!Files.exists(projectRoot.resolve(".git"))) {
139+
projectRoot = projectRoot.getParent();
140+
if (projectRoot == null) {
141+
throw new RuntimeException("Invalid path, need to be within a Git repository");
142+
}
143+
}
144+
145+
String pathFromProjectRoot = projectRoot.relativize(fileAbsolutePath).toString();
146+
147+
Git git = Git.open(Paths.get(projectRoot.toString(), ".git").toFile());
132148
Repository repository = git.getRepository();
133-
Ref head = repository.getRefDatabase().findRef(ref);
149+
Ref head = repository.getRefDatabase().findRef(gitRef);
134150
if (head == null) {
135-
throw new IllegalStateException("Cannot find " + ref + " in the repository.");
151+
throw new IllegalStateException("Cannot find " + gitRef + " in the repository.");
136152
}
137153

138154
try (RevWalk revWalk = new RevWalk(repository)) {
@@ -141,9 +157,9 @@ static String getDataFromGit(String filePath, Path gitPath, String ref) throws I
141157
try (TreeWalk treeWalk = new TreeWalk(repository)) {
142158
treeWalk.addTree(tree);
143159
treeWalk.setRecursive(true);
144-
treeWalk.setFilter(PathFilter.create(String.valueOf(Paths.get(filePath.substring(1)))));
160+
treeWalk.setFilter(PathFilter.create(String.valueOf(Paths.get(pathFromProjectRoot))));
145161
if (!treeWalk.next()) {
146-
throw new IllegalStateException("Did not find expected file " + filePath.substring(1));
162+
throw new IllegalStateException("Did not find expected file " + pathFromProjectRoot);
147163
}
148164
ObjectId objectId = treeWalk.getObjectId(0);
149165
ObjectLoader loader = repository.open(objectId);

‎generator/src/main/java/org/apache/kafka/message/checker/FieldSpecPairIterator.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ public boolean hasNext() {
7272
"message2, but should not be, based on its versions.");
7373
}
7474
}
75+
field1 = iterator1.hasNext() ? iterator1.next() : null;
7576
break;
7677
}
7778
case MESSAGE2_ONLY:

‎generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java‎

Lines changed: 20 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,8 @@
2525
import net.sourceforge.argparse4j.internal.HelpScreenException;
2626

2727
import java.io.PrintStream;
28-
import java.nio.file.Files;
29-
import java.nio.file.Path;
30-
import java.nio.file.Paths;
3128

32-
import static org.apache.kafka.message.checker.CheckerUtils.getDataFromGit;
29+
import static org.apache.kafka.message.checker.CheckerUtils.readFileFromGitRef;
3330

3431
public class MetadataSchemaCheckerTool {
3532
public static void main(String[] args) throws Exception {
@@ -54,18 +51,18 @@ public static void run(
5451
required(true).
5552
help("The path to a schema JSON file.");
5653
Subparser evolutionVerifierParser = subparsers.addParser("verify-evolution").
57-
help("Verify that an evolution of a JSON file is valid.");
58-
evolutionVerifierParser.addArgument("--path1", "-1").
54+
help("Verify that a schema JSON file is a valid evolution of a parent schema.");
55+
evolutionVerifierParser.addArgument("--path", "-1").
5956
required(true).
60-
help("The initial schema JSON path.");
61-
evolutionVerifierParser.addArgument("--path2", "-2").
57+
help("The path to a schema JSON file.");
58+
evolutionVerifierParser.addArgument("--parent_path", "-2").
6259
required(true).
63-
help("The final schema JSON path.");
60+
help("The path to the parent schema JSON file.");
6461
Subparser evolutionGitVerifierParser = subparsers.addParser("verify-evolution-git").
65-
help(" Verify that an evolution of a JSON file is valid using git.");
66-
evolutionGitVerifierParser.addArgument("--file", "-3").
62+
help("Verify that a schema JSON file is a valid evolution of your local git master branch.");
63+
evolutionGitVerifierParser.addArgument("--path", "-3").
6764
required(true).
68-
help("The edited JSON file");
65+
help("The path to your edited JSON file");
6966
evolutionGitVerifierParser.addArgument("--ref", "-4")
7067
.required(false)
7168
.setDefault("refs/heads/trunk")
@@ -85,31 +82,24 @@ public static void run(
8582
break;
8683
}
8784
case "verify-evolution": {
88-
String path1 = namespace.getString("path1");
89-
String path2 = namespace.getString("path2");
85+
String child = namespace.getString("path");
86+
String parent = namespace.getString("parent_path");
9087
EvolutionVerifier verifier = new EvolutionVerifier(
91-
CheckerUtils.readMessageSpecFromFile(path1),
92-
CheckerUtils.readMessageSpecFromFile(path2));
88+
CheckerUtils.readMessageSpecFromFile(parent),
89+
CheckerUtils.readMessageSpecFromFile(child));
9390
verifier.verify();
94-
writer.println("Successfully verified evolution of path1: " + path1 +
95-
", and path2: " + path2);
91+
writer.println("Successfully verified evolution of path: " + child +
92+
" from parent: " + parent);
9693
break;
9794
}
9895
case "verify-evolution-git": {
99-
String filePath = "/metadata/src/main/resources/common/metadata/" + namespace.getString("file");
100-
Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
101-
while (!Files.exists(rootKafkaDirectory.resolve(".git"))) {
102-
rootKafkaDirectory = rootKafkaDirectory.getParent();
103-
if (rootKafkaDirectory == null) {
104-
throw new RuntimeException("Invalid directory, need to be within a Git repository");
105-
}
106-
}
107-
String gitContent = getDataFromGit(filePath, rootKafkaDirectory, namespace.getString("ref"));
96+
String path = namespace.getString("path");
97+
String gitContent = readFileFromGitRef(path, namespace.getString("ref"));
10898
EvolutionVerifier verifier = new EvolutionVerifier(
109-
CheckerUtils.readMessageSpecFromFile(rootKafkaDirectory + filePath),
110-
CheckerUtils.readMessageSpecFromString(gitContent));
99+
CheckerUtils.readMessageSpecFromFile(path),
100+
CheckerUtils.readMessageSpecFromString(gitContent));
111101
verifier.verify();
112-
writer.println("Successfully verified evolution of file: " + namespace.getString("file"));
102+
writer.println("Successfully verified evolution of file: " + namespace.getString("path"));
113103
break;
114104
}
115105
default:

‎generator/src/main/java/org/apache/kafka/message/checker/Unifier.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ class Unifier {
5151
this.structRegistry1.register(topLevelMessage1);
5252
this.topLevelMessage2 = topLevelMessage2;
5353
this.structRegistry2 = new StructRegistry();
54-
this.structRegistry1.register(topLevelMessage2);
54+
this.structRegistry2.register(topLevelMessage2);
5555
}
5656

5757
static FieldSpec structSpecToFieldSpec(StructSpec structSpec) {

‎generator/src/test/java/org/apache/kafka/message/checker/MetadataSchemaCheckerToolTest.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121

2222
import java.io.ByteArrayOutputStream;
2323
import java.io.PrintStream;
24+
import java.nio.file.Files;
25+
import java.nio.file.Path;
26+
import java.nio.file.Paths;
2427

2528
import static org.apache.kafka.message.checker.CheckerTestUtils.messageSpecStringToTempFile;
2629
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -29,13 +32,21 @@ public class MetadataSchemaCheckerToolTest {
2932
@Test
3033
public void testVerifyEvolutionGit() throws Exception {
3134
try (ByteArrayOutputStream stream = new ByteArrayOutputStream()) {
35+
Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
36+
while (!Files.exists(rootKafkaDirectory.resolve(".git"))) {
37+
rootKafkaDirectory = rootKafkaDirectory.getParent();
38+
if (rootKafkaDirectory == null) {
39+
throw new RuntimeException("Invalid directory, need to be within a Git repository");
40+
}
41+
}
42+
Path schemaPath = rootKafkaDirectory.resolve("metadata/src/main/resources/common/metadata/AbortTransactionRecord.json");
3243
MetadataSchemaCheckerTool.run(
3344
// In the CI environment because the CI fetch command only creates HEAD and refs/remotes/pull/... references.
3445
// Since there may not be other branches like refs/heads/trunk in CI, HEAD serves as the baseline reference.
35-
new String[]{"verify-evolution-git", "--file", "AbortTransactionRecord.json", "--ref", "HEAD"},
46+
new String[]{"verify-evolution-git", "--path", schemaPath.toString(), "--ref", "HEAD"},
3647
new PrintStream(stream)
3748
);
38-
assertEquals("Successfully verified evolution of file: AbortTransactionRecord.json",
49+
assertEquals("Successfully verified evolution of file: " + schemaPath,
3950
stream.toString().trim());
4051
}
4152
}
@@ -60,8 +71,8 @@ public void testSuccessfulVerifyEvolution() throws Exception {
6071
"'validVersions': '0-2', 'flexibleVersions': '0+', " +
6172
"'fields': [{'name': 'BrokerId', 'type': 'int32', 'versions': '0+'}]}");
6273
MetadataSchemaCheckerTool.run(new String[] {"verify-evolution",
63-
"--path1", path, "--path2", path}, new PrintStream(stream));
64-
assertEquals("Successfully verified evolution of path1: " + path + ", and path2: " + path,
74+
"--path", path, "--parent_path", path}, new PrintStream(stream));
75+
assertEquals("Successfully verified evolution of path: " + path + " from parent: " + path,
6576
stream.toString().trim());
6677
}
6778
}

0 commit comments

Comments
 (0)