From 4f63573f5a49fb564e7b65b9573769963511dbea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Klemen=20Ko=C5=A1ir?= Date: Thu, 27 Apr 2023 15:13:18 +0900 Subject: THRIFT-4086: Use true type when generating field meta data (#2765) Client: java --- .../cpp/src/thrift/generate/t_java_generator.cc | 30 +++++----- compiler/cpp/src/thrift/generate/t_oop_generator.h | 2 +- lib/java/gradle/generateTestThrift.gradle | 12 ++-- .../org/apache/thrift/TestDefinitionOrder.java | 66 ++++++++++++++++++++++ .../java/org/apache/thrift/TestStructOrder.java | 66 ---------------------- .../src/test/resources/JavaDefinitionOrderA.thrift | 50 ++++++++++++++++ .../src/test/resources/JavaDefinitionOrderB.thrift | 50 ++++++++++++++++ .../src/test/resources/JavaStructOrderA.thrift | 28 --------- .../src/test/resources/JavaStructOrderB.thrift | 29 ---------- 9 files changed, 188 insertions(+), 145 deletions(-) create mode 100644 lib/java/src/test/java/org/apache/thrift/TestDefinitionOrder.java delete mode 100644 lib/java/src/test/java/org/apache/thrift/TestStructOrder.java create mode 100644 lib/java/src/test/resources/JavaDefinitionOrderA.thrift create mode 100644 lib/java/src/test/resources/JavaDefinitionOrderB.thrift delete mode 100644 lib/java/src/test/resources/JavaStructOrderA.thrift delete mode 100644 lib/java/src/test/resources/JavaStructOrderB.thrift diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc b/compiler/cpp/src/thrift/generate/t_java_generator.cc index ee98600f2..4259cf8d7 100644 --- 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 index 2df1be413..884196203 100644 --- a/compiler/cpp/src/thrift/generate/t_oop_generator.h +++ b/compiler/cpp/src/thrift/generate/t_oop_generator.h @@ -70,7 +70,7 @@ public: } 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 index 301812fcc..10bb16bba 100644 --- 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/lib/java/src/test/java/org/apache/thrift/TestDefinitionOrder.java b/lib/java/src/test/java/org/apache/thrift/TestDefinitionOrder.java new file mode 100644 index 000000000..51021b791 --- /dev/null +++ b/lib/java/src/test/java/org/apache/thrift/TestDefinitionOrder.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 fields in different order (esp. when they reference each other) generates +// identical code +public class TestDefinitionOrder { + + @Test + public void testDefinitionOrder() throws Exception { + List filenames = Arrays.asList("Parent.java", "Child.java", "MyEnum.java"); + + for (String fn : filenames) { + String fnA = "definition-order-test/a/" + fn; + String fnB = "definition-order-test/b/" + fn; + + 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); + + 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/java/org/apache/thrift/TestStructOrder.java b/lib/java/src/test/java/org/apache/thrift/TestStructOrder.java deleted file mode 100644 index dd67d9b5d..000000000 --- a/lib/java/src/test/java/org/apache/thrift/TestStructOrder.java +++ /dev/null @@ -1,66 +0,0 @@ -/* - * 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 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/JavaDefinitionOrderA.thrift b/lib/java/src/test/resources/JavaDefinitionOrderA.thrift new file mode 100644 index 000000000..9b4296443 --- /dev/null +++ b/lib/java/src/test/resources/JavaDefinitionOrderA.thrift @@ -0,0 +1,50 @@ +/* + * 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. + */ + +// Define Parent, then Child. No forward declarations. +struct Parent { + 1: required string Name +} + +typedef Parent MyParent +typedef list Parents + +enum MyEnum { + FOO = 1 + BAR = 2 +} + +typedef i8 Age +typedef MyEnum MyEnumV2 +typedef set MyEnums +typedef map MyMapping +typedef binary MyBinary + +struct Child { + 1: required string Name + 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/lib/java/src/test/resources/JavaDefinitionOrderB.thrift b/lib/java/src/test/resources/JavaDefinitionOrderB.thrift new file mode 100644 index 000000000..0cccd9705 --- /dev/null +++ b/lib/java/src/test/resources/JavaDefinitionOrderB.thrift @@ -0,0 +1,50 @@ +/* + * 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. + */ + +// 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 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 Parents +typedef MyEnum MyEnumV2 +typedef set MyEnums +typedef map MyMapping +typedef binary MyBinary + +struct Parent { + 1: required string Name +} + +enum MyEnum { + FOO = 1 + BAR = 2 +} diff --git a/lib/java/src/test/resources/JavaStructOrderA.thrift b/lib/java/src/test/resources/JavaStructOrderA.thrift deleted file mode 100644 index f00a7106d..000000000 --- a/lib/java/src/test/resources/JavaStructOrderA.thrift +++ /dev/null @@ -1,28 +0,0 @@ -/* - * 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. - */ - -// 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 deleted file mode 100644 index 4682668b3..000000000 --- a/lib/java/src/test/resources/JavaStructOrderB.thrift +++ /dev/null @@ -1,29 +0,0 @@ -/* - * 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. - */ - -// 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 -} -- cgit v1.2.1