THRIFT-5521: [java gen] use jdk8 option type in java generator code

Client: Java
Patch: Jiayu Liu

This closes #2525

17 files changed, 214 insertions, 28 deletions

diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc b/compiler/cpp/src/thrift/generate/t_java_generator.cc
index efcd7d304..6f4267512 100644
--- a/compiler/cpp/src/thrift/generate/t_java_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_java_generator.cc
@@ -46,6 +46,9 @@ using std::vector;
 
 static const string endl = "\n"; // avoid ostream << std::endl flushes
 
+static const string thrift_option_class = "org.apache.thrift.Option";
+static const string jdk_option_class = "java.util.Optional";
+
 /**
  * Java code generator.
  *
@@ -70,6 +73,7 @@ public:
     reuse_objects_ = false;
     use_option_type_ = false;
     generate_future_iface_ = false;
+    use_jdk8_option_type_ = false;
     undated_generated_annotations_ = false;
     suppress_generated_annotations_ = false;
     rethrow_unhandled_exceptions_ = false;
@@ -101,6 +105,13 @@ public:
         reuse_objects_ = true;
       } else if (iter->first.compare("option_type") == 0) {
         use_option_type_ = true;
+        if (iter->second.compare("jdk8") == 0) {
+          use_jdk8_option_type_ = true;
+        } else if (iter->second.compare("thrift") == 0 || iter->second.compare("") == 0) {
+          use_jdk8_option_type_ = false;
+        } else {
+          throw "option_type must be 'jdk8' or 'thrift'";
+        }
       } else if (iter->first.compare("rethrow_unhandled_exceptions") == 0) {
         rethrow_unhandled_exceptions_ = true;
       } else if (iter->first.compare("generated_annotations") == 0) {
@@ -423,6 +434,7 @@ private:
   bool reuse_objects_;
   bool generate_future_iface_;
   bool use_option_type_;
+  bool use_jdk8_option_type_;
   bool undated_generated_annotations_;
   bool suppress_generated_annotations_;
   bool rethrow_unhandled_exceptions_;
@@ -1287,7 +1299,7 @@ void t_java_generator::generate_tuple_scheme_read_value(ostream& out, t_struct*
   indent_up();
   indent(out) << "switch (setField) {" << endl;
   indent_up();
-  
+
   const vector<t_field*>& members = tstruct->get_members();
   vector<t_field*>::const_iterator m_iter;
 
@@ -2367,18 +2379,36 @@ void t_java_generator::generate_java_bean_boilerplate(ostream& out, t_struct* ts
         if (is_deprecated) {
           indent(out) << "@Deprecated" << endl;
         }
-        indent(out) << "public org.apache.thrift.Option<Integer> get" << cap_name;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "public " << jdk_option_class << "<Integer> get" << cap_name;
+        } else {
+          indent(out) << "public " << thrift_option_class << "<Integer> get" << cap_name;
+        }
+
         out << get_cap_name("size() {") << endl;
 
         indent_up();
         indent(out) << "if (this." << field_name << " == null) {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.none();" << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".empty();" << endl;
+        } else {
+          indent(out) << "return " << thrift_option_class << ".none();" << endl;
+        }
+
         indent_down();
         indent(out) << "} else {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.some(this." << field_name << ".size());"
-                    << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".of(this.";
+        } else {
+          indent(out) << "return " << thrift_option_class << ".some(this.";
+        }
+        out << field_name << ".size());" << endl;
+
         indent_down();
         indent(out) << "}" << endl;
         indent_down();
@@ -2411,19 +2441,38 @@ void t_java_generator::generate_java_bean_boilerplate(ostream& out, t_struct* ts
         if (is_deprecated) {
           indent(out) << "@Deprecated" << endl;
         }
-        indent(out) << "public org.apache.thrift.Option<java.util.Iterator<"
-                    << type_name(element_type, true, false) << ">> get" << cap_name;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "public " << jdk_option_class << "<";
+        } else {
+          indent(out) << "public " << thrift_option_class << "<";
+        }
+        out << "java.util.Iterator<" << type_name(element_type, true, false) << ">> get"
+            << cap_name;
+
         out << get_cap_name("iterator() {") << endl;
 
         indent_up();
         indent(out) << "if (this." << field_name << " == null) {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.none();" << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".empty();" << endl;
+        } else {
+          indent(out) << "return " << thrift_option_class << ".none();" << endl;
+        }
+
         indent_down();
         indent(out) << "} else {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.some(this." << field_name << ".iterator());"
-                    << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".of(this.";
+        } else {
+          indent(out) << "return " << thrift_option_class << ".some(this.";
+        }
+        out << field_name << ".iterator());" << endl;
+
         indent_down();
         indent(out) << "}" << endl;
         indent_down();
@@ -2521,7 +2570,13 @@ void t_java_generator::generate_java_bean_boilerplate(ostream& out, t_struct* ts
         if (is_deprecated) {
           indent(out) << "@Deprecated" << endl;
         }
-        indent(out) << "public org.apache.thrift.Option<" << type_name(type, true) << ">";
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "public " << jdk_option_class << "<" << type_name(type, true) << ">";
+        } else {
+          indent(out) << "public " << thrift_option_class << "<" << type_name(type, true) << ">";
+        }
+
         if (type->is_base_type() && ((t_base_type*)type)->get_base() == t_base_type::TYPE_BOOL) {
           out << " is";
         } else {
@@ -2532,11 +2587,24 @@ void t_java_generator::generate_java_bean_boilerplate(ostream& out, t_struct* ts
 
         indent(out) << "if (this.isSet" << cap_name << "()) {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.some(this." << field_name << ");" << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".of(this.";
+        } else {
+          indent(out) << "return " << thrift_option_class << ".some(this.";
+        }
+        out << field_name << ");" << endl;
+
         indent_down();
         indent(out) << "} else {" << endl;
         indent_up();
-        indent(out) << "return org.apache.thrift.Option.none();" << endl;
+
+        if (use_jdk8_option_type_) {
+          indent(out) << "return " << jdk_option_class << ".empty();" << endl;
+        } else {
+          indent(out) << "return " << thrift_option_class << ".none();" << endl;
+        }
+
         indent_down();
         indent(out) << "}" << endl;
         indent_down();
@@ -5651,7 +5719,10 @@ THRIFT_REGISTER_GENERATOR(
     "    android:         Generated structures are Parcelable.\n"
     "    android_legacy:  Do not use java.io.IOException(throwable) (available for Android 2.3 and "
     "above).\n"
-    "    option_type:     Wrap optional fields in an Option type.\n"
+    "    option_type=[thrift|jdk8]:\n"
+    "                     thrift: wrap optional fields in thrift Option type.\n"
+    "                     jdk8: Wrap optional fields in JDK8+ Option type.\n"
+    "                     If the Option type is not specified, 'thrift' is used.\n"
     "    rethrow_unhandled_exceptions:\n"
     "                     Enable rethrow of unhandled exceptions and let them propagate further."
     " (Default behavior is to catch and log it.)\n"
diff --git a/lib/java/gradle/functionalTests.gradle b/lib/java/gradle/functionalTests.gradle
index f00f74bf2..f1975202b 100644
--- a/lib/java/gradle/functionalTests.gradle
+++ b/lib/java/gradle/functionalTests.gradle
@@ -91,8 +91,8 @@ if (org.gradle.internal.os.OperatingSystem.current().windows) {
 def javaExe = file("${System.getProperty('java.home')}/bin/java${execExt}").canonicalPath
 // The common Uber jar path
 def jarPath = shadowJar.archivePath.canonicalPath
-def trustStore = file('test/.truststore').canonicalPath
-def keyStore = file('test/.keystore').canonicalPath
+def trustStore = file('test/resources/.truststore').canonicalPath
+def keyStore = file('test/resources/.keystore').canonicalPath
 
 task generateRunnerScriptForClient(group: 'Build') {
     description = 'Generate a runner script for cross-check tests with TestClient'
diff --git a/lib/java/gradle/generateTestThrift.gradle b/lib/java/gradle/generateTestThrift.gradle
index d5bc3afaf..c48845e9d 100644
--- a/lib/java/gradle/generateTestThrift.gradle
+++ b/lib/java/gradle/generateTestThrift.gradle
@@ -24,11 +24,12 @@ ext.genSrc = file("$buildDir/gen-java")
 ext.genBeanSrc = file("$buildDir/gen-javabean")
 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")
 
 // Add the generated code directories to the test source set
 sourceSets {
-    test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc, genUnsafeSrc
+    test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc, genUnsafeSrc, genOptionTypeJdk8Src
 }
 
 // ----------------------------------------------------------------------------
@@ -37,7 +38,10 @@ sourceSets {
 // A callable closure to make this easier
 ext.thriftCompile = { Task task, String thriftFileName, String generator = 'java', File outputDir = genSrc ->
     def thriftFile = file("$thriftRoot/test/$thriftFileName")
-    assert thriftFile.exists()
+    if (!thriftFile.exists()) {
+        thriftFile = file("$projectDir/test/resources/$thriftFileName")
+        assert thriftFile.exists(), "can't find $thriftFile"
+    }
 
     task.inputs.file thriftFile
     task.outputs.dir outputDir
@@ -85,6 +89,15 @@ task generateJava(group: 'Build') {
     thriftCompile(it, 'partial/thrift_test_schema.thrift')
 }
 
+task generateOptionalTypeJava(group: 'Build') {
+    description = 'Generate the thrift gen-option-type-jdk8 source'
+    generate.dependsOn it
+
+    ext.outputBuffer = new ByteArrayOutputStream()
+
+    thriftCompile(it, 'JavaOptionTypeJdk8Test.thrift', 'java:option_type=jdk8', genOptionTypeJdk8Src)
+}
+
 task generateBeanJava(group: 'Build') {
     description = 'Generate the thrift gen-javabean source'
     generate.dependsOn it
diff --git a/lib/java/gradle/sourceConfiguration.gradle b/lib/java/gradle/sourceConfiguration.gradle
index ce0db75d7..c510a40d5 100644
--- a/lib/java/gradle/sourceConfiguration.gradle
+++ b/lib/java/gradle/sourceConfiguration.gradle
@@ -37,7 +37,7 @@ sourceSets {
             exclude '**/test/TestTServletServer.java'
         }
         resources {
-            srcDir 'test'
+            srcDir 'test/resources'
             include 'log4j.properties'
         }
     }
diff --git a/lib/java/gradle/unitTests.gradle b/lib/java/gradle/unitTests.gradle
index 61f2fbdeb..2bf1c039a 100644
--- a/lib/java/gradle/unitTests.gradle
+++ b/lib/java/gradle/unitTests.gradle
@@ -74,9 +74,9 @@ test {
     systemProperties = [
         'build.test': "${compileTestJava.destinationDir}",
         'test.port': "${testPort}",
-        'javax.net.ssl.trustStore': "${projectDir}/test/.truststore",
+        'javax.net.ssl.trustStore': "${projectDir}/test/resources/.truststore",
         'javax.net.ssl.trustStorePassword': 'thrift',
-        'javax.net.ssl.keyStore': "${projectDir}/test/.keystore",
+        'javax.net.ssl.keyStore': "${projectDir}/test/resources/.keystore",
         'javax.net.ssl.keyStorePassword': 'thrift'
     ]
 }
diff --git a/lib/java/test/org/apache/thrift/TestOptionType.java b/lib/java/test/org/apache/thrift/TestOptionType.java
index f70285ffa..ddde9d35f 100644
--- a/lib/java/test/org/apache/thrift/TestOptionType.java
+++ b/lib/java/test/org/apache/thrift/TestOptionType.java
@@ -20,11 +20,10 @@
 package org.apache.thrift;
 
 import junit.framework.TestCase;
-import org.apache.thrift.Option;
 
 // Tests and documents behavior for the "Option<T>" type
 public class TestOptionType extends TestCase {
-    public void testSome() throws Exception {
+    public void testSome() {
         String name = "Chuck Norris";
         Option<String> option = Option.fromNullable(name);
 
diff --git a/lib/java/test/org/apache/thrift/TestOptionals.java b/lib/java/test/org/apache/thrift/TestOptionals.java
index f3d4cfcf6..b34e06dd3 100644
--- a/lib/java/test/org/apache/thrift/TestOptionals.java
+++ b/lib/java/test/org/apache/thrift/TestOptionals.java
@@ -26,7 +26,7 @@ import thrift.test.Opt30;
 import thrift.test.Opt64;
 import thrift.test.Opt80;
 
-// Exercises the isSet methods using structs from from ManyOptionals.thrift
+// Exercises the isSet methods using structs from ManyOptionals.thrift
 public class TestOptionals extends TestCase {
   public void testEncodingUtils() throws Exception {
     assertEquals((short)0x8, EncodingUtils.setBit((short)0, 3, true));
diff --git a/lib/java/test/org/apache/thrift/TestOptionalsWithJdk8.java b/lib/java/test/org/apache/thrift/TestOptionalsWithJdk8.java
new file mode 100644
index 000000000..93dacedda
--- /dev/null
+++ b/lib/java/test/org/apache/thrift/TestOptionalsWithJdk8.java
@@ -0,0 +1,63 @@
+/*
+ * 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 junit.framework.TestCase;
+import thrift.test.optiontypejdk8.Person;
+
+// Tests and documents behavior for the JDK8 "Option<T>" type
+public class TestOptionalsWithJdk8 extends TestCase {
+
+    public void testConstruction() {
+        Person person = new Person(1L, "name");
+        assertFalse(person.getAge().isPresent());
+        assertFalse(person.isSetAge());
+        assertFalse(person.getPhone().isPresent());
+        assertFalse(person.isSetPhone());
+        assertEquals(1L, person.getId());
+        assertTrue(person.isSetId());
+        assertEquals("name", person.getName());
+        assertTrue(person.isSetName());
+
+        assertFalse(person.getAddresses().isPresent());
+        assertEquals(Integer.valueOf(0), person.getAddressesSize().orElse(0));
+        assertFalse(person.getPets().isPresent());
+        assertEquals(Integer.valueOf(0), person.getPetsSize().orElse(0));
+    }
+
+    public void testEmpty() {
+        Person person = new Person();
+        person.setPhone("phone");
+        assertFalse(person.getAge().isPresent());
+        assertFalse(person.isSetAge());
+        assertTrue(person.getPhone().isPresent());
+        assertEquals("phone", person.getPhone().get());
+        assertTrue(person.isSetPhone());
+        assertEquals(0L, person.getId());
+        assertFalse(person.isSetId());
+        assertNull(person.getName());
+        assertFalse(person.isSetName());
+
+        assertFalse(person.getAddresses().isPresent());
+        assertEquals(Integer.valueOf(0), person.getAddressesSize().orElse(0));
+        assertFalse(person.getPets().isPresent());
+        assertEquals(Integer.valueOf(0), person.getPetsSize().orElse(0));
+    }
+}
diff --git a/lib/java/test/.keystore b/lib/java/test/resources/.keystore
index 4dd66ac07..4dd66ac07 100644
--- a/lib/java/test/.keystore
+++ b/lib/java/test/resources/.keystore
diff --git a/lib/java/test/.truststore b/lib/java/test/resources/.truststore
index 26fbd195f..26fbd195f 100644
--- a/lib/java/test/.truststore
+++ b/lib/java/test/resources/.truststore
diff --git a/test/JavaBeansTest.thrift b/lib/java/test/resources/JavaBeansTest.thrift
index b6c3ea861..b6c3ea861 100644
--- a/test/JavaBeansTest.thrift
+++ b/lib/java/test/resources/JavaBeansTest.thrift
diff --git a/test/JavaBinaryDefault.thrift b/lib/java/test/resources/JavaBinaryDefault.thrift
index 5517802c7..5517802c7 100644
--- a/test/JavaBinaryDefault.thrift
+++ b/lib/java/test/resources/JavaBinaryDefault.thrift
diff --git a/test/JavaDeepCopyTest.thrift b/lib/java/test/resources/JavaDeepCopyTest.thrift
index fc974aefd..fc974aefd 100644
--- a/test/JavaDeepCopyTest.thrift
+++ b/lib/java/test/resources/JavaDeepCopyTest.thrift
diff --git a/lib/java/test/resources/JavaOptionTypeJdk8Test.thrift b/lib/java/test/resources/JavaOptionTypeJdk8Test.thrift
new file mode 100644
index 000000000..193e079d0
--- /dev/null
+++ b/lib/java/test/resources/JavaOptionTypeJdk8Test.thrift
@@ -0,0 +1,44 @@
+/*
+ * 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.
+ *
+ * Contains some contributions under the Thrift Software License.
+ * Please see doc/old-thrift-license.txt in the Thrift distribution for
+ * details.
+ */
+
+namespace java thrift.test.optiontypejdk8
+
+struct Person {
+  1: required i64 id;
+  2: required string name;
+  3: optional i64 age;
+  4: optional string phone;
+  5: optional list<string> addresses;
+  6: optional map<string, Pet> pets;
+}
+
+enum PetType {
+  Cat = 1
+  Dog = 2
+  Bunny = 3
+}
+
+struct Pet {
+  1: required string name;
+  2: optional PetType type;
+}
diff --git a/test/JavaTypes.thrift b/lib/java/test/resources/JavaTypes.thrift
index 555334082..555334082 100644
--- a/test/JavaTypes.thrift
+++ b/lib/java/test/resources/JavaTypes.thrift
diff --git a/lib/java/test/log4j.properties b/lib/java/test/resources/log4j.properties
index ab9bebafe..ab9bebafe 100644
--- a/lib/java/test/log4j.properties
+++ b/lib/java/test/resources/log4j.properties
diff --git a/test/Makefile.am b/test/Makefile.am
index 58482f2d2..d428086bd 100755
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -142,10 +142,6 @@ EXTRA_DIST = \
 	Include.thrift \
 	Identifiers.thrift \
 	Int64Test.thrift \
-	JavaBeansTest.thrift \
-	JavaBinaryDefault.thrift \
-	JavaDeepCopyTest.thrift \
-	JavaTypes.thrift \
 	JsDeepConstructorTest.thrift \
 	ManyOptionals.thrift \
 	ManyTypedefs.thrift \