From dee6d4260c2cca0917d5a921460d16f062ab27d6 Mon Sep 17 00:00:00 2001
From: Bryan Duxbury <bryanduxbury@apache.org>
Date: Tue, 23 Feb 2010 19:06:25 +0000
Subject: [PATCH] THRIFT-713. java: Java compareTo method throws NPE when any
 field isn't set.

This patch fixes a somewhat egregious bug in the generated compareTo for non-union structs and avoids possible NPEs.

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@915499 13f79535-47bb-0310-9956-ffa450edef68
---
 compiler/cpp/src/generate/t_java_generator.cc | 10 +--
 lib/java/build.xml                            |  2 +
 .../org/apache/thrift/test/CompareTest.java   | 65 +++++++++++++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100644 lib/java/test/org/apache/thrift/test/CompareTest.java

diff --git a/compiler/cpp/src/generate/t_java_generator.cc b/compiler/cpp/src/generate/t_java_generator.cc
index 4b9cd479..bb59ab8b 100644
--- a/compiler/cpp/src/generate/t_java_generator.cc
+++ b/compiler/cpp/src/generate/t_java_generator.cc
@@ -1392,14 +1392,16 @@ void t_java_generator::generate_java_struct_compare_to(ofstream& out, t_struct*
   vector<t_field*>::const_iterator m_iter;
   for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
     t_field* field = *m_iter;
-    indent(out) << "lastComparison = Boolean.valueOf(" << generate_isset_check(field) << ").compareTo(" << generate_isset_check(field) << ");" << endl;
+    indent(out) << "lastComparison = Boolean.valueOf(" << generate_isset_check(field) << ").compareTo(typedOther." << generate_isset_check(field) << ");" << endl;
     indent(out) << "if (lastComparison != 0) {" << endl;
     indent(out) << "  return lastComparison;" << endl;
     indent(out) << "}" << endl;
 
-    indent(out) << "lastComparison = TBaseHelper.compareTo(" << field->get_name() << ", typedOther." << field->get_name() << ");" << endl;
-    indent(out) << "if (lastComparison != 0) {" << endl;
-    indent(out) << "  return lastComparison;" << endl;
+    indent(out) << "if (" << generate_isset_check(field) << ") {";
+    indent(out) << "  lastComparison = TBaseHelper.compareTo(" << field->get_name() << ", typedOther." << field->get_name() << ");" << endl;
+    indent(out) << "  if (lastComparison != 0) {" << endl;
+    indent(out) << "    return lastComparison;" << endl;
+    indent(out) << "  }" << endl;
     indent(out) << "}" << endl;
   }
 
diff --git a/lib/java/build.xml b/lib/java/build.xml
index 7b189452..97509639 100644
--- a/lib/java/build.xml
+++ b/lib/java/build.xml
@@ -176,6 +176,8 @@
       classpathref="test.classpath" failonerror="true" />
     <java classname="org.apache.thrift.test.DeepCopyTest"
       classpathref="test.classpath" failonerror="true" />
+    <java classname="org.apache.thrift.test.CompareTest"
+      classpathref="test.classpath" failonerror="true" />
     <java classname="org.apache.thrift.test.MetaDataTest"
       classpathref="test.classpath" failonerror="true" />
     <java classname="org.apache.thrift.test.JavaBeansTest"
diff --git a/lib/java/test/org/apache/thrift/test/CompareTest.java b/lib/java/test/org/apache/thrift/test/CompareTest.java
new file mode 100644
index 00000000..569d9180
--- /dev/null
+++ b/lib/java/test/org/apache/thrift/test/CompareTest.java
@@ -0,0 +1,65 @@
+/*
+ * 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.test;
+
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TSerializer;
+import org.apache.thrift.protocol.TBinaryProtocol;
+import thrift.test.*;
+
+public class CompareTest {
+  public static void main(String[] args) throws Exception {
+    Bonk bonk1 = new Bonk();
+    Bonk bonk2 = new Bonk();
+
+    // Compare empty thrift objects.
+    if (bonk1.compareTo(bonk2) != 0)
+      throw new RuntimeException("empty objects not equal according to compareTo");
+
+    bonk1.setMessage("m");
+
+    // Compare one thrift object with a filled in field and another without it.
+    if (bonk1.compareTo(bonk2) <= 0)
+      throw new RuntimeException("compare " + bonk1 + " to " + bonk2 + " returned " + bonk1.compareTo(bonk2));
+    if (bonk2.compareTo(bonk1) >= 0)
+      throw new RuntimeException("compare " + bonk2 + " to " + bonk1 + " returned " + bonk2.compareTo(bonk1));
+
+    // Compare both have filled-in fields.
+    bonk2.setMessage("z");
+    if (bonk1.compareTo(bonk2) >= 0)
+      throw new RuntimeException("compare " + bonk1 + " to " + bonk2 + " returned " + bonk1.compareTo(bonk2));
+    if (bonk2.compareTo(bonk1) <= 0)
+      throw new RuntimeException("compare " + bonk2 + " to " + bonk1 + " returned " + bonk2.compareTo(bonk1));
+
+    // Compare bonk1 has a field filled in that bonk2 doesn't.
+    bonk1.setType(123);
+    if (bonk1.compareTo(bonk2) <= 0)
+      throw new RuntimeException("compare " + bonk1 + " to " + bonk2 + " returned " + bonk1.compareTo(bonk2));
+    if (bonk2.compareTo(bonk1) >= 0)
+      throw new RuntimeException("compare " + bonk2 + " to " + bonk1 + " returned " + bonk2.compareTo(bonk1));
+
+    // Compare bonk1 and bonk2 equal.
+    bonk2.setType(123);
+    bonk2.setMessage("m");
+    if (bonk1.compareTo(bonk2) != 0)
+      throw new RuntimeException("compare " + bonk1 + " to " + bonk2 + " returned " + bonk1.compareTo(bonk2));
+  }
+}
-- 
2.17.1