THRFIT-804. java: CompareTo is broken for unions set to map, set, or list

This patch fixes TUnion's compareTo, and factors out the standard part of the comparison to TBaseHelper.

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@957350 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 2db3ca3..89c1956 100644 (file)
--- a/compiler/cpp/src/generate/t_java_generator.cc
+++ b/compiler/cpp/src/generate/t_java_generator.cc
@@ -1010,12 +1010,7 @@ void t_java_generator::generate_union_comparisons(ofstream& out, t_struct* tstru
   indent(out) << "public int compareTo(" << type_name(tstruct) << " other) {" << endl;
   indent(out) << "  int lastComparison = TBaseHelper.compareTo(getSetField(), other.getSetField());" << endl;
   indent(out) << "  if (lastComparison == 0) {" << endl;
-  indent(out) << "    Object myValue = getFieldValue();" << endl;
-  indent(out) << "    if (myValue instanceof byte[]) {" << endl;
-  indent(out) << "      return TBaseHelper.compareTo((byte[])myValue, (byte[])other.getFieldValue());" << endl;
-  indent(out) << "    } else {" << endl;
-  indent(out) << "      return TBaseHelper.compareTo((Comparable)myValue, (Comparable)other.getFieldValue());" << endl;
-  indent(out) << "    }" << endl;
+  indent(out) << "    return TBaseHelper.compareTo(getFieldValue(), other.getFieldValue());" << endl;
   indent(out) << "  }" << endl;
   indent(out) << "  return lastComparison;" << endl;
   indent(out) << "}" << endl;

diff --git a/lib/java/src/org/apache/thrift/TBaseHelper.java b/lib/java/src/org/apache/thrift/TBaseHelper.java
index fccece8..55e9505 100644 (file)
--- a/lib/java/src/org/apache/thrift/TBaseHelper.java
+++ b/lib/java/src/org/apache/thrift/TBaseHelper.java
@@ -31,6 +31,22 @@ public class TBaseHelper {
 
   private static final Comparator comparator = new NestedStructureComparator();
 
+  public static int compareTo(Object o1, Object o2) {
+    if (o1 instanceof Comparable) {
+      return compareTo((Comparable)o1, (Comparable)o2);
+    } else if (o1 instanceof List) {
+      return compareTo((List)o1, (List)o2);
+    } else if (o1 instanceof Set) {
+      return compareTo((Set)o1, (Set)o2);
+    } else if (o1 instanceof Map) {
+      return compareTo((Map)o1, (Map)o2);
+    } else if (o1 instanceof byte[]) {
+      return compareTo((byte[])o1, (byte[])o2);
+    } else {
+      throw new IllegalArgumentException("Cannot compare objects of type " + o1.getClass());
+    }
+  }
+
   public static int compareTo(boolean a, boolean b) {
     return Boolean.valueOf(a).compareTo(b);
   }

diff --git a/lib/java/test/org/apache/thrift/TestTUnion.java b/lib/java/test/org/apache/thrift/TestTUnion.java
index 2ff80e9..a8b1287 100644 (file)
--- a/lib/java/test/org/apache/thrift/TestTUnion.java
+++ b/lib/java/test/org/apache/thrift/TestTUnion.java
@@ -24,10 +24,16 @@ import org.apache.thrift.transport.TMemoryBuffer;
 
 import thrift.test.ComparableUnion;
 import thrift.test.Empty;
+import thrift.test.RandomStuff;
 import thrift.test.SomeEnum;
 import thrift.test.StructWithAUnion;
 import thrift.test.TestUnion;
 import thrift.test.TestUnionMinusStringField;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
 import junit.framework.TestCase;
 
 public class TestTUnion extends TestCase {
@@ -98,6 +104,22 @@ public class TestTUnion extends TestCase {
 
     assertTrue(cu.compareTo(cu2) < 0);
     assertTrue(cu2.compareTo(cu) > 0);
+    
+    TestUnion union1 = new TestUnion(TestUnion._Fields.STRUCT_LIST, new ArrayList<RandomStuff>());
+    TestUnion union2 = new TestUnion(TestUnion._Fields.STRUCT_LIST, new ArrayList<RandomStuff>());
+    assertTrue(union1.compareTo(union2) == 0);
+
+    TestUnion union3 = new TestUnion(TestUnion._Fields.I32_SET, new HashSet<Integer>());
+    Set<Integer> i32_set = new HashSet<Integer>();
+    i32_set.add(1);
+    TestUnion union4 = new TestUnion(TestUnion._Fields.I32_SET, i32_set);
+    assertTrue(union3.compareTo(union4) < 0);
+
+    Map<Integer, Integer> i32_map = new HashMap<Integer, Integer>();
+    i32_map.put(1,1);
+    TestUnion union5 = new TestUnion(TestUnion._Fields.I32_MAP, i32_map);
+    TestUnion union6 = new TestUnion(TestUnion._Fields.I32_MAP, new HashMap<Integer, Integer>());
+    assertTrue(union5.compareTo(union6) > 0);
   }
 
   public void testEquality() throws Exception {

diff --git a/test/DebugProtoTest.thrift b/test/DebugProtoTest.thrift
index 5e361d2..c67fbcb 100644 (file)
--- a/test/DebugProtoTest.thrift
+++ b/test/DebugProtoTest.thrift
@@ -299,6 +299,8 @@ union TestUnion {
   4: list<RandomStuff> struct_list;
   5: i32 other_i32_field;
   6: SomeEnum enum_field;
+  7: set<i32> i32_set;
+  8: map<i32, i32> i32_map;
 }
 
 union TestUnionMinusStringField {
@@ -307,6 +309,8 @@ union TestUnionMinusStringField {
   4: list<RandomStuff> struct_list;
   5: i32 other_i32_field;
   6: SomeEnum enum_field;
+  7: set<i32> i32_set;
+  8: map<i32, i32> i32_map;
 }
 
 union ComparableUnion {
@@ -339,4 +343,5 @@ struct BreaksRubyCompactProtocol {
   1: string field1;
   2: BigFieldIdStruct field2;
   3: i32 field3;
-}
\ No newline at end of file
+}
+