THRIFT-4086: Add missing calls to get_true_type when generating validator + metadata code

Client: java

5 files changed, 100 insertions, 2 deletions

diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc b/compiler/cpp/src/thrift/generate/t_java_generator.cc
index 3ce1450b1..7dfd10f52 100644
--- a/compiler/cpp/src/thrift/generate/t_java_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_java_generator.cc
@@ -2192,7 +2192,7 @@ void t_java_generator::generate_java_validator(ostream& out, t_struct* tstruct)
 
   out << indent() << "// check for sub-struct validity" << endl;
   for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
-    t_type* type = (*f_iter)->get_type();
+    t_type* type = get_true_type((*f_iter)->get_type());
     if (type->is_struct() && !((t_struct*)type)->is_union()) {
       out << indent() << "if (" << (*f_iter)->get_name() << " != null) {" << endl;
       out << indent() << "  " << (*f_iter)->get_name() << ".validate();" << endl;
@@ -2965,7 +2965,8 @@ void t_java_generator::generate_field_value_meta_data(std::ostream& out, t_type*
   out << endl;
   indent_up();
   indent_up();
-  if (type->is_struct() || type->is_xception()) {
+  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, "
diff --git a/lib/java/gradle/generateTestThrift.gradle b/lib/java/gradle/generateTestThrift.gradle
index e8515473e..0506aa6cf 100644
--- a/lib/java/gradle/generateTestThrift.gradle
+++ b/lib/java/gradle/generateTestThrift.gradle
@@ -26,6 +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")
 
 // Add the generated code directories to the test source set
 sourceSets {
@@ -143,3 +145,13 @@ 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'
+    generate.dependsOn it
+
+    ext.outputBuffer = new ByteArrayOutputStream()
+
+    thriftCompile(it, 'JavaStructOrderA.thrift', 'java', genStructOrderTestASrc)
+    thriftCompile(it, 'JavaStructOrderB.thrift', 'java', genStructOrderTestBSrc)
+}
diff --git a/lib/java/src/test/java/org/apache/thrift/TestStructOrder.java b/lib/java/src/test/java/org/apache/thrift/TestStructOrder.java
new file mode 100644
index 000000000..dd67d9b5d
--- /dev/null
+++ b/lib/java/src/test/java/org/apache/thrift/TestStructOrder.java
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.thrift;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.*;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+// Tests that declaring structs in different order (esp. when they reference each other) generates
+// identical code
+public class TestStructOrder {
+
+  @Test
+  public void testStructOrder() throws Exception {
+    List<String> filenames = Arrays.asList("Parent.java", "Child.java");
+
+    for (String fn : filenames) {
+      String fnA = "struct-order-test/a/" + fn;
+      String fnB = "struct-order-test/b/" + fn;
+
+      try (InputStream isA = TestStructOrder.class.getClassLoader().getResourceAsStream(fnA);
+          InputStream isB = TestStructOrder.class.getClassLoader().getResourceAsStream(fnB)) {
+
+        assertNotNull(isA, "Resource not found: " + fnA);
+        assertNotNull(isB, "Resource not found: " + fnB);
+
+        int hashA = Arrays.hashCode(readAllBytes(isA));
+        assertEquals(
+            hashA,
+            Arrays.hashCode(readAllBytes(isB)),
+            String.format("Generated Java files %s and %s differ", fnA, fnB));
+      }
+    }
+  }
+
+  // TODO Use InputStream.readAllBytes post-Java8
+  private byte[] readAllBytes(InputStream is) throws IOException {
+    ByteArrayOutputStream os = new ByteArrayOutputStream();
+    byte[] buff = new byte[1024];
+    int bytesRead;
+    while ((bytesRead = is.read(buff, 0, buff.length)) != -1) {
+      os.write(buff, 0, bytesRead);
+    }
+    return os.toByteArray();
+  }
+}
diff --git a/lib/java/src/test/resources/JavaStructOrderA.thrift b/lib/java/src/test/resources/JavaStructOrderA.thrift
new file mode 100644
index 000000000..bac520900
--- /dev/null
+++ b/lib/java/src/test/resources/JavaStructOrderA.thrift
@@ -0,0 +1,9 @@
+// Define Parent, then Child. No forward declarations.
+struct Parent {
+    1: required string Name
+}
+
+struct Child {
+    1: required string Name
+    2: required Parent Parent
+}
diff --git a/lib/java/src/test/resources/JavaStructOrderB.thrift b/lib/java/src/test/resources/JavaStructOrderB.thrift
new file mode 100644
index 000000000..aa2afeab2
--- /dev/null
+++ b/lib/java/src/test/resources/JavaStructOrderB.thrift
@@ -0,0 +1,10 @@
+// Define Child, then Parent. Parent is a forward declaration and was problematic for our Java compiler before
+// 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
+}
+
+struct Parent {
+    1: required string Name
+}