THRIFT-4086: Use true type when generating field meta data

Client: java
apache · Apr 19, 2023 · 10c210d · 10c210d
1 parent 1e3d90d
commit 10c210d
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 32 deletions.
diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc b/compiler/cpp/src/thrift/generate/t_java_generator.cc
@@ -3023,46 +3023,46 @@ void t_java_generator::generate_metadata_for_field_annotations(std::ostream& out
 }
 
 void t_java_generator::generate_field_value_meta_data(std::ostream& out, t_type* type) {
+  t_type* ttype = get_true_type(type);
   out << endl;
   indent_up();
   indent_up();
-  t_type* ttype = get_true_type(type);
   if (ttype->is_struct() || ttype->is_xception()) {
     indent(out) << "new "
                    "org.apache.thrift.meta_data.StructMetaData(org.apache.thrift.protocol.TType."
                    "STRUCT, "
-                << type_name(type) << ".class";
-  } else if (type->is_container()) {
-    if (type->is_list()) {
+                << type_name(ttype) << ".class";
+  } else if (ttype->is_container()) {
+    if (ttype->is_list()) {
       indent(out)
           << "new org.apache.thrift.meta_data.ListMetaData(org.apache.thrift.protocol.TType.LIST, ";
-      t_type* elem_type = ((t_list*)type)->get_elem_type();
+      t_type* elem_type = ((t_list*)ttype)->get_elem_type();
       generate_field_value_meta_data(out, elem_type);
-    } else if (type->is_set()) {
+    } else if (ttype->is_set()) {
       indent(out)
           << "new org.apache.thrift.meta_data.SetMetaData(org.apache.thrift.protocol.TType.SET, ";
-      t_type* elem_type = ((t_set*)type)->get_elem_type();
+      t_type* elem_type = ((t_set*)ttype)->get_elem_type();
       generate_field_value_meta_data(out, elem_type);
     } else { // map
       indent(out)
           << "new org.apache.thrift.meta_data.MapMetaData(org.apache.thrift.protocol.TType.MAP, ";
-      t_type* key_type = ((t_map*)type)->get_key_type();
-      t_type* val_type = ((t_map*)type)->get_val_type();
+      t_type* key_type = ((t_map*)ttype)->get_key_type();
+      t_type* val_type = ((t_map*)ttype)->get_val_type();
       generate_field_value_meta_data(out, key_type);
       out << ", ";
       generate_field_value_meta_data(out, val_type);
     }
-  } else if (type->is_enum()) {
+  } else if (ttype->is_enum()) {
     indent(out)
         << "new org.apache.thrift.meta_data.EnumMetaData(org.apache.thrift.protocol.TType.ENUM, "
-        << type_name(type) << ".class";
+        << type_name(ttype) << ".class";
   } else {
     indent(out) << "new org.apache.thrift.meta_data.FieldValueMetaData("
-                << get_java_type_string(type);
-    if (type->is_typedef()) {
-      indent(out) << ", \"" << ((t_typedef*)type)->get_symbolic() << "\"";
-    } else if (type->is_binary()) {
+                << get_java_type_string(ttype);
+    if (ttype->is_binary()) {
       indent(out) << ", true";
+    } else if (type->is_typedef()) {
+      indent(out) << ", \"" << ((t_typedef*)type)->get_symbolic() << "\"";
     }
   }
   out << ")";

diff --git a/compiler/cpp/src/thrift/generate/t_oop_generator.h b/compiler/cpp/src/thrift/generate/t_oop_generator.h
@@ -70,7 +70,7 @@ class t_oop_generator : public t_generator {
   }
 
   virtual void generate_java_doc(std::ostream& out, t_field* field) {
-    if (field->get_type()->is_enum()) {
+    if (get_true_type(field->get_type())->is_enum()) {
       std::string combined_message = field->get_doc() + "\n@see "
                                      + get_enum_class_name(field->get_type());
       generate_java_docstring_comment(out, combined_message);

diff --git a/lib/java/gradle/generateTestThrift.gradle b/lib/java/gradle/generateTestThrift.gradle
@@ -26,8 +26,8 @@ ext.genReuseSrc = file("$buildDir/gen-javareuse")
 ext.genFullCamelSrc = file("$buildDir/gen-fullcamel")
 ext.genOptionTypeJdk8Src = file("$buildDir/gen-option-type-jdk8")
 ext.genUnsafeSrc = file("$buildDir/gen-unsafe")
-ext.genStructOrderTestASrc = file("$buildDir/resources/test/struct-order-test/a")
-ext.genStructOrderTestBSrc = file("$buildDir/resources/test/struct-order-test/b")
+ext.genDefinitionOrderTestASrc = file("$buildDir/resources/test/definition-order-test/a")
+ext.genDefinitionOrderTestBSrc = file("$buildDir/resources/test/definition-order-test/b")
 
 // Add the generated code directories to the test source set
 sourceSets {
@@ -146,12 +146,12 @@ task generateWithAnnotationMetadata(group: 'Build') {
     thriftCompile(it, 'JavaAnnotationTest.thrift', 'java:annotations_as_metadata', genSrc)
 }
 
-task generateJavaStructOrderTestJava(group: 'Build') {
-    description = 'Generate structs defined in different order and add to build dir so we can compare them'
+task generateJavaDefinitionOrderTestJava(group: 'Build') {
+    description = 'Generate fields defined in different order and add to build dir so we can compare them'
     generate.dependsOn it
 
     ext.outputBuffer = new ByteArrayOutputStream()
 
-    thriftCompile(it, 'JavaStructOrderA.thrift', 'java', genStructOrderTestASrc)
-    thriftCompile(it, 'JavaStructOrderB.thrift', 'java', genStructOrderTestBSrc)
+    thriftCompile(it, 'JavaDefinitionOrderA.thrift', 'java', genDefinitionOrderTestASrc)
+    thriftCompile(it, 'JavaDefinitionOrderB.thrift', 'java', genDefinitionOrderTestBSrc)
 }
diff --git a/...va/org/apache/thrift/TestStructOrder.java → ...rg/apache/thrift/TestDefinitionOrder.java b/...va/org/apache/thrift/TestStructOrder.java → ...rg/apache/thrift/TestDefinitionOrder.java
@@ -26,20 +26,20 @@
 import java.util.List;
 import org.junit.jupiter.api.Test;
 
-// Tests that declaring structs in different order (esp. when they reference each other) generates
+// Tests that declaring fields in different order (esp. when they reference each other) generates
 // identical code
-public class TestStructOrder {
+public class TestDefinitionOrder {
 
   @Test
-  public void testStructOrder() throws Exception {
-    List<String> filenames = Arrays.asList("Parent.java", "Child.java");
+  public void testDefinitionOrder() throws Exception {
+    List<String> filenames = Arrays.asList("Parent.java", "Child.java", "MyEnum.java");
 
     for (String fn : filenames) {
-      String fnA = "struct-order-test/a/" + fn;
-      String fnB = "struct-order-test/b/" + fn;
+      String fnA = "definition-order-test/a/" + fn;
+      String fnB = "definition-order-test/b/" + fn;
 
-      try (InputStream isA = TestStructOrder.class.getClassLoader().getResourceAsStream(fnA);
-          InputStream isB = TestStructOrder.class.getClassLoader().getResourceAsStream(fnB)) {
+      try (InputStream isA = TestDefinitionOrder.class.getClassLoader().getResourceAsStream(fnA);
+          InputStream isB = TestDefinitionOrder.class.getClassLoader().getResourceAsStream(fnB)) {
 
         assertNotNull(isA, "Resource not found: " + fnA);
         assertNotNull(isB, "Resource not found: " + fnB);

diff --git a/...rc/test/resources/JavaStructOrderA.thrift → ...est/resources/JavaDefinitionOrderA.thrift b/...rc/test/resources/JavaStructOrderA.thrift → ...est/resources/JavaDefinitionOrderA.thrift
@@ -22,7 +22,29 @@ struct Parent {
     1: required string Name
 }
 
+typedef Parent MyParent
+typedef list<Parent> Parents
+
+enum MyEnum {
+    FOO = 1
+    BAR = 2
+}
+
+typedef i8 Age
+typedef MyEnum MyEnumV2
+typedef set<MyEnum> MyEnums
+typedef map<MyEnumV2, Parent> MyMapping
+typedef binary MyBinary
+
 struct Child {
     1: required string Name
-    2: required Parent Parent
+    2: required Age Age
+    3: required Parent Parent1
+    4: required MyParent Parent2
+    5: required Parents GrandParents
+    6: required MyEnum MyEnum
+    7: required MyEnumV2 MyEnumV2
+    8: required MyEnums MyEnums
+    9: required MyMapping MyMapping
+    10: required MyBinary MyBinary
 }
diff --git a/...rc/test/resources/JavaStructOrderB.thrift → ...est/resources/JavaDefinitionOrderB.thrift b/...rc/test/resources/JavaStructOrderB.thrift → ...est/resources/JavaDefinitionOrderB.thrift
@@ -21,9 +21,30 @@
 // fixing THRIFT-4086: Java compiler generates different meta data depending on order of structures in file
 struct Child {
     1: required string Name
-    2: required Parent Parent
+    2: required Age Age
+    3: required Parent Parent1
+    4: required MyParent Parent2
+    5: required Parents GrandParents
+    6: required MyEnum MyEnum
+    7: required MyEnumV2 MyEnumV2
+    8: required MyEnums MyEnums
+    9: required MyMapping MyMapping
+    10: required MyBinary MyBinary
 }
 
+typedef i8 Age
+typedef Parent MyParent
+typedef list<Parent> Parents
+typedef MyEnum MyEnumV2
+typedef set<MyEnum> MyEnums
+typedef map<MyEnumV2, Parent> MyMapping
+typedef binary MyBinary
+
 struct Parent {
     1: required string Name
 }
+
+enum MyEnum {
+    FOO = 1
+    BAR = 2
+}