Skip to content

Commit 0790e02

Browse files
Add checks on thrift files based on ID (#846)
1 parent 59e524b commit 0790e02

1 file changed

Lines changed: 127 additions & 0 deletions

File tree

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
package com.databricks.jdbc.dbclient.impl.thrift;
2+
3+
import static org.junit.jupiter.api.Assertions.fail;
4+
5+
import com.databricks.jdbc.log.JdbcLogger;
6+
import com.databricks.jdbc.log.JdbcLoggerFactory;
7+
import java.io.IOException;
8+
import java.nio.file.Files;
9+
import java.nio.file.Path;
10+
import java.nio.file.Paths;
11+
import java.util.ArrayList;
12+
import java.util.List;
13+
import java.util.stream.Stream;
14+
import org.apache.thrift.TFieldIdEnum;
15+
import org.junit.jupiter.api.Test;
16+
17+
/**
18+
* Validates that all Thrift-generated classes comply with field ID constraints.
19+
*
20+
* <p>Field IDs in Thrift must stay below 3329 to avoid conflicts with reserved ranges and ensure
21+
* compatibility with various Thrift implementations and protocols.
22+
*/
23+
public class ThriftGeneratedFieldIdTest {
24+
private static final JdbcLogger LOGGER =
25+
JdbcLoggerFactory.getLogger(ThriftGeneratedFieldIdTest.class);
26+
27+
private static final int MAX_ALLOWED_FIELD_ID = 3329;
28+
private static final String GENERATED_PACKAGE =
29+
"com.databricks.jdbc.model.client.thrift.generated";
30+
private static final String GENERATED_DIR =
31+
"src/main/java/com/databricks/jdbc/model/client/thrift/generated";
32+
33+
@Test
34+
public void testAllThriftFieldIdsAreWithinAllowedRange() throws IOException {
35+
List<String> violations = new ArrayList<>();
36+
37+
Path generatedDirPath = Paths.get(GENERATED_DIR);
38+
39+
if (!Files.exists(generatedDirPath)) {
40+
fail("Generated directory does not exist: " + GENERATED_DIR);
41+
}
42+
43+
// Find all Java files that might contain Thrift-generated classes
44+
try (Stream<Path> javaFiles =
45+
Files.walk(generatedDirPath).filter(path -> path.toString().endsWith(".java"))) {
46+
javaFiles.forEach(javaFile -> checkFileForFieldIdViolations(javaFile, violations));
47+
}
48+
49+
if (!violations.isEmpty()) {
50+
StringBuilder errorMessage = new StringBuilder();
51+
errorMessage
52+
.append("Found Thrift field IDs that exceed the maximum allowed value of ")
53+
.append(MAX_ALLOWED_FIELD_ID)
54+
.append(".\nThis can cause compatibility issues and conflicts with reserved ID ranges.\n")
55+
.append("Violations found:\n");
56+
57+
for (String violation : violations) {
58+
errorMessage.append(" - ").append(violation).append("\n");
59+
}
60+
61+
fail(errorMessage.toString());
62+
}
63+
}
64+
65+
/**
66+
* Examines a Java file to determine if it contains a Thrift-generated class and validates that
67+
* all field IDs are within the allowed range.
68+
*/
69+
private void checkFileForFieldIdViolations(Path javaFile, List<String> violations) {
70+
String fileName = javaFile.getFileName().toString();
71+
72+
if (!fileName.endsWith(".java")) {
73+
return;
74+
}
75+
76+
String className = fileName.substring(0, fileName.length() - 5);
77+
String fullClassName = GENERATED_PACKAGE + "." + className;
78+
79+
try {
80+
Class<?> clazz = Class.forName(fullClassName);
81+
82+
// Look for the _Fields enum that Thrift generates for struct classes
83+
// This enum contains metadata about each field including its ID
84+
for (Class<?> innerClass : clazz.getDeclaredClasses()) {
85+
if (isThriftFieldsEnum(innerClass)) {
86+
validateAllFieldIdsInClass(innerClass, className, violations);
87+
break;
88+
}
89+
}
90+
} catch (ClassNotFoundException e) {
91+
// File exists but class can't be loaded - might not be compiled
92+
// or might not be a valid Thrift-generated class
93+
LOGGER.error(e, "Unable to find class " + fullClassName);
94+
}
95+
}
96+
97+
/** Checks if the given class is a Thrift-generated _Fields enum that contains field metadata. */
98+
private boolean isThriftFieldsEnum(Class<?> innerClass) {
99+
return "_Fields".equals(innerClass.getSimpleName())
100+
&& innerClass.isEnum()
101+
&& TFieldIdEnum.class.isAssignableFrom(innerClass);
102+
}
103+
104+
/**
105+
* Validates that all field IDs in a Thrift class are within the allowed range. Reports any fields
106+
* that have IDs >= MAX_ALLOWED_FIELD_ID.
107+
*/
108+
private void validateAllFieldIdsInClass(
109+
Class<?> fieldsEnum, String className, List<String> violations) {
110+
Object[] fieldDefinitions = fieldsEnum.getEnumConstants();
111+
112+
if (fieldDefinitions != null) {
113+
for (Object fieldDefinition : fieldDefinitions) {
114+
TFieldIdEnum thriftField = (TFieldIdEnum) fieldDefinition;
115+
short fieldId = thriftField.getThriftFieldId();
116+
117+
if (fieldId >= MAX_ALLOWED_FIELD_ID) {
118+
String fieldName = fieldDefinition.toString();
119+
violations.add(
120+
String.format(
121+
"%s._Fields.%s has field ID %d (exceeds maximum of %d)",
122+
className, fieldName, fieldId, MAX_ALLOWED_FIELD_ID - 1));
123+
}
124+
}
125+
}
126+
}
127+
}

0 commit comments

Comments
 (0)