Author: bryanduxbury
Date: Tue Feb 23 19:06:25 2010
New Revision: 915499
URL: http://svn.apache.org/viewvc?rev=915499&view=rev
Log:
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.
Added:
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/CompareTest.java
Modified:
incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
incubator/thrift/trunk/lib/java/build.xml
Modified: incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc?rev=915499&r1=915498&r2=915499&view=diff
==============================================================================
--- incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
(original)
+++ incubator/thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc Tue
Feb 23 19:06:25 2010
@@ -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;
}
Modified: incubator/thrift/trunk/lib/java/build.xml
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/lib/java/build.xml?rev=915499&r1=915498&r2=915499&view=diff
==============================================================================
--- incubator/thrift/trunk/lib/java/build.xml (original)
+++ incubator/thrift/trunk/lib/java/build.xml Tue Feb 23 19:06:25 2010
@@ -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"
Added:
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/CompareTest.java
URL:
http://svn.apache.org/viewvc/incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/CompareTest.java?rev=915499&view=auto
==============================================================================
---
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/CompareTest.java
(added)
+++
incubator/thrift/trunk/lib/java/test/org/apache/thrift/test/CompareTest.java
Tue Feb 23 19:06:25 2010
@@ -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));
+ }
+}
