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
diff --git a/compiler/cpp/src/generate/t_java_generator.cc b/compiler/cpp/src/generate/t_java_generator.cc
index 4b9cd47..bb59ab8 100644
--- a/compiler/cpp/src/generate/t_java_generator.cc
+++ b/compiler/cpp/src/generate/t_java_generator.cc
@@ -1392,14 +1392,16 @@
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 7b18945..9750963 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 0000000..569d918
--- /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));
+ }
+}