THRIFT-735: Reject unset unions in required-field validation

compiler/cpp/src/thrift/generate/t_java_generator.cc

@@ -2227,16 +2227,23 @@ void t_java_generator::generate_java_validator(ostream& out, t_struct* tstruct)
   out << indent() << "// check for required fields" << '\n';
   for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
     if ((*f_iter)->get_req() == t_field::T_REQUIRED) {
+      // A union that was read with no recognized field is non-null but carries no value.
+      t_type* req_type = get_true_type((*f_iter)->get_type());
+      bool is_union = req_type->is_struct() && ((t_struct*)req_type)->is_union();
+      string union_unset
+          = is_union ? " || !" + make_valid_java_identifier((*f_iter)->get_name()) + ".isSet()"
+                     : "";
       if (bean_style_) {
-        out << indent() << "if (!" << generate_isset_check(*f_iter) << ") {" << '\n'
+        out << indent() << "if (!" << generate_isset_check(*f_iter) << union_unset << ") {" << '\n'
             << indent()
             << "  throw new org.apache.thrift.protocol.TProtocolException(\"Required field '"
             << (*f_iter)->get_name() << "' is unset! Struct:\" + toString());" << '\n'
             << indent() << "}" << '\n'
             << '\n';
       } else {
         if (type_can_be_null((*f_iter)->get_type())) {
-          indent(out) << "if (" << (*f_iter)->get_name() << " == null) {" << '\n';
+          indent(out) << "if (" << (*f_iter)->get_name() << " == null" << union_unset << ") {"
+                      << '\n';
           indent(out)
               << "  throw new org.apache.thrift.protocol.TProtocolException(\"Required field '"
               << (*f_iter)->get_name() << "' was not present! Struct: \" + toString());" << '\n';

lib/java/gradle/generateTestThrift.gradle

@@ -91,6 +91,7 @@ task generateJava(group: 'Build') {
     thriftCompile(it, 'JavaAnnotationTest.thrift')
     thriftCompile(it, 'partial/thrift_test_schema.thrift')
     thriftCompile(it, 'FuzzTest.thrift')
+    thriftCompile(it, 'JavaRequiredUnion.thrift')
 }
 
 task generateOptionalTypeJava(group: 'Build') {

lib/java/src/test/java/org/apache/thrift/TestRequiredUnionValidation.java

@@ -0,0 +1,105 @@
+/*
+ * 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.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import org.apache.thrift.protocol.TProtocolException;
+import org.junit.jupiter.api.Test;
+import thrift.test.BeansRequiredUnionHolder;
+import thrift.test.BeansUnion;
+import thrift.test.requnion.RequiredUnionChoice;
+import thrift.test.requnion.RequiredUnionChoiceV2;
+import thrift.test.requnion.RequiredUnionHolder;
+import thrift.test.requnion.RequiredUnionHolderV2;
+
+/** THRIFT-735: a required union field must hold a union that has a field set. */
+public class TestRequiredUnionValidation {
+
+  private static RequiredUnionHolder valid() {
+    RequiredUnionHolder holder = new RequiredUnionHolder();
+    holder.setChoice(RequiredUnionChoice.number(1));
+    holder.setAliased(RequiredUnionChoice.text("x"));
+    return holder;
+  }
+
+  @Test
+  public void testSetUnionsValidate() {
+    assertDoesNotThrow(() -> valid().validate());
+  }
+
+  @Test
+  public void testMissingUnionStillRejected() {
+    RequiredUnionHolder holder = valid();
+    holder.setChoice(null);
+    assertThrows(TProtocolException.class, holder::validate);
+  }
+
+  @Test
+  public void testUnsetRequiredUnionRejected() {
+    RequiredUnionHolder holder = valid();
+    holder.setChoice(new RequiredUnionChoice());
+    assertThrows(TProtocolException.class, holder::validate);
+  }
+
+  @Test
+  public void testUnsetRequiredTypedefUnionRejected() {
+    RequiredUnionHolder holder = valid();
+    holder.setAliased(new RequiredUnionChoice());
+    assertThrows(TProtocolException.class, holder::validate);
+  }
+
+  @Test
+  public void testUnsetOptionalUnionAccepted() {
+    RequiredUnionHolder holder = valid();
+    holder.setMaybe(new RequiredUnionChoice());
+    assertDoesNotThrow(holder::validate);
+  }
+
+  @Test
+  public void testUnsetRequiredUnionRejectedBeans() {
+    BeansRequiredUnionHolder holder = new BeansRequiredUnionHolder();
+    holder.set_choice(BeansUnion.number(1));
+    assertDoesNotThrow(holder::validate);
+
+    holder.set_choice(new BeansUnion());
+    assertThrows(TProtocolException.class, holder::validate);
+
+    holder.set_choice(null);
+    assertThrows(TProtocolException.class, holder::validate);
+  }
+
+  /**
+   * A reader that does not know the union member the writer set skips it and leaves the union
+   * unset, so read() rejects the struct through its validate() call.
+   */
+  @Test
+  public void testReadRejectsRequiredUnionWithUnknownMember() throws TException {
+    RequiredUnionHolderV2 written = new RequiredUnionHolderV2();
+    written.setChoice(RequiredUnionChoiceV2.added(5L));
+    written.setAliased(RequiredUnionChoiceV2.text("x"));
+    byte[] bytes = new TSerializer().serialize(written);
+
+    TDeserializer deserializer = new TDeserializer();
+    assertDoesNotThrow(() -> deserializer.deserialize(new RequiredUnionHolderV2(), bytes));
+    assertThrows(
+        TProtocolException.class, () -> deserializer.deserialize(new RequiredUnionHolder(), bytes));
+  }
+}

lib/java/src/test/resources/JavaBeansTest.thrift

@@ -33,6 +33,14 @@ struct OneOfEachBeans {
   11: list<i64> i64_list
 }
 
+union BeansUnion {
+  1: i32 number,
+  2: string text
+}
+
+struct BeansRequiredUnionHolder {
+  1: required BeansUnion choice
+}
 
 service Service {
   i64 mymethod(i64 blah);

lib/java/src/test/resources/JavaRequiredUnion.thrift

@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+namespace java thrift.test.requnion
+
+union RequiredUnionChoice {
+  1: i32 number,
+  2: string text
+}
+
+typedef RequiredUnionChoice RequiredUnionAlias
+
+struct RequiredUnionHolder {
+  1: required RequiredUnionChoice choice,
+  2: required RequiredUnionAlias aliased,
+  3: optional RequiredUnionChoice maybe
+}
+
+// The same shapes as seen by a newer writer whose union has a member that
+// RequiredUnionChoice does not know.
+union RequiredUnionChoiceV2 {
+  1: i32 number,
+  2: string text,
+  3: i64 added
+}
+
+struct RequiredUnionHolderV2 {
+  1: required RequiredUnionChoiceV2 choice,
+  2: required RequiredUnionChoiceV2 aliased
+}