Fixing the lack of ClassLoader consideration in the compareTo methods of enum.Enum and enums.Enum, along with unit tests, as mentioned on Bugzilla entry #32619 by Kathy Van Stone.

git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/lang/trunk@398812 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/commons/lang/enum/Enum.java

@@ -543,18 +543,10 @@ public abstract class Enum implements Comparable, Serializable {
             return iName.equals(((Enum) other).iName);
         } else {
             // This and other are in different class loaders, we must use reflection.
-            try {
+            if (other.getClass().getName().equals(this.getClass().getName()) == false) {
-                Method mth = other.getClass().getMethod("getName", null);
+                return false;
-                String name = (String) mth.invoke(other, null);
-                return iName.equals(name);
-            } catch (NoSuchMethodException e) {
-                // ignore - should never happen
-            } catch (IllegalAccessException e) {
-                // ignore - should never happen
-            } catch (InvocationTargetException e) {
-                // ignore - should never happen
             }
-            return false;
+            return iName.equals( getNameInOtherClassLoader(other) );
         }
     }
     
@@ -573,6 +565,9 @@ public abstract class Enum implements Comparable, Serializable {
      * <p>The default ordering is alphabetic by name, but this
      * can be overridden by subclasses.</p>
      * 
+     * <p>If the parameter is in a different class loader than this instance,
+     * reflection is used to compare the names.</p>
+     *
      * @see java.lang.Comparable#compareTo(Object)
      * @param other  the other object to compare to
      * @return -ve if this is less than the other object, +ve if greater
@@ -584,9 +579,29 @@ public abstract class Enum implements Comparable, Serializable {
         if (other == this) {
             return 0;
         }
+        if (other.getClass() != this.getClass()) {
+            if (other.getClass().getName().equals(this.getClass().getName())) {
+                return iName.compareTo( getNameInOtherClassLoader(other) );
+            }
+        }
         return iName.compareTo(((Enum) other).iName);
     }
 
+    private String getNameInOtherClassLoader(Object other) {
+        try {
+            Method mth = other.getClass().getMethod("getName", null);
+            String name = (String) mth.invoke(other, null);
+            return name;
+        } catch (NoSuchMethodException e) {
+            // ignore - should never happen
+        } catch (IllegalAccessException e) {
+            // ignore - should never happen
+        } catch (InvocationTargetException e) {
+            // ignore - should never happen
+        }
+        throw new IllegalStateException("This should not happen");
+    }
+
     /**
      * <p>Human readable description of this Enum item.</p>
      * 

src/java/org/apache/commons/lang/enums/Enum.java

@@ -543,18 +543,7 @@ public abstract class Enum implements Comparable, Serializable {
             if (other.getClass().getName().equals(this.getClass().getName()) == false) {
                 return false;
             }
-            try {
+            return iName.equals( getNameInOtherClassLoader(other) );
-                Method mth = other.getClass().getMethod("getName", null);
-                String name = (String) mth.invoke(other, null);
-                return iName.equals(name);
-            } catch (NoSuchMethodException e) {
-                // ignore - should never happen
-            } catch (IllegalAccessException e) {
-                // ignore - should never happen
-            } catch (InvocationTargetException e) {
-                // ignore - should never happen
-            }
-            return false;
         }
     }
     
@@ -573,6 +562,9 @@ public abstract class Enum implements Comparable, Serializable {
      * <p>The default ordering is alphabetic by name, but this
      * can be overridden by subclasses.</p>
      * 
+     * <p>If the parameter is in a different class loader than this instance,
+     * reflection is used to compare the names.</p>
+     *
      * @see java.lang.Comparable#compareTo(Object)
      * @param other  the other object to compare to
      * @return -ve if this is less than the other object, +ve if greater
@@ -584,9 +576,29 @@ public abstract class Enum implements Comparable, Serializable {
         if (other == this) {
             return 0;
         }
+        if (other.getClass() != this.getClass()) {
+            if (other.getClass().getName().equals(this.getClass().getName())) {
+                return iName.compareTo( getNameInOtherClassLoader(other) );
+            }
+        }
         return iName.compareTo(((Enum) other).iName);
     }
 
+    private String getNameInOtherClassLoader(Object other) {
+        try {
+            Method mth = other.getClass().getMethod("getName", null);
+            String name = (String) mth.invoke(other, null);
+            return name;
+        } catch (NoSuchMethodException e) {
+            // ignore - should never happen
+        } catch (IllegalAccessException e) {
+            // ignore - should never happen
+        } catch (InvocationTargetException e) {
+            // ignore - should never happen
+        }
+        throw new IllegalStateException("This should not happen");
+    }
+
     /**
      * <p>Human readable description of this Enum item.</p>
      * 

src/test/org/apache/commons/lang/enum/EnumTest.java

@@ -444,12 +444,12 @@ public final class EnumTest extends TestCase {
 
     public void testColorEnumEqualsWithDifferentClassLoaders() throws SecurityException, IllegalArgumentException,
             ClassNotFoundException, NoSuchMethodException, IllegalAccessException, InvocationTargetException {
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.BLUE);
+        this.testWithDifferentClassLoaders(ColorEnum.BLUE);
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.GREEN);
+        this.testWithDifferentClassLoaders(ColorEnum.GREEN);
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.RED);
+        this.testWithDifferentClassLoaders(ColorEnum.RED);
     }
 
-    void testEqualsTrueWithDifferentClassLoaders(ColorEnum colorEnum) throws ClassNotFoundException, SecurityException,
+    void testWithDifferentClassLoaders(ColorEnum colorEnum) throws ClassNotFoundException, SecurityException,
             NoSuchMethodException, IllegalArgumentException, IllegalAccessException, InvocationTargetException {
         // Sanity checks:
         assertTrue(colorEnum.equals(colorEnum));
@@ -457,6 +457,7 @@ public final class EnumTest extends TestCase {
         // set up:
         ClassLoader classLoader = ClassUtilsTest.newSystemClassLoader();
         Object enumObjectFromOtherClassLoader = this.getColorEnum(classLoader, colorEnum.getName());
+
         // the real test, part 1.
         try {
             ColorEnum testCase = (ColorEnum) enumObjectFromOtherClassLoader;
@@ -464,10 +465,12 @@ public final class EnumTest extends TestCase {
         } catch (ClassCastException e) {
             // normal.
         }
+
         // the real test, part 2.
         assertEquals("The two objects should match even though they are from different class loaders", colorEnum,
                 enumObjectFromOtherClassLoader);
-        // the real test, part 3.
+
+        // the real test, part 3 - testing equals(Object)
         int falseCount = 0;
         for (Iterator iter = ColorEnum.iterator(); iter.hasNext();) {
             ColorEnum element = (ColorEnum) iter.next();
@@ -477,6 +480,17 @@ public final class EnumTest extends TestCase {
             }
         }
         assertEquals(ColorEnum.getEnumList().size() - 1, falseCount);
+
+        // the real test, part 4 - testing compareTo(Object) == 0
+        falseCount = 0;
+        for (Iterator iter = ColorEnum.iterator(); iter.hasNext();) {
+            ColorEnum element = (ColorEnum) iter.next();
+            if (!colorEnum.equals(element)) {
+                falseCount++;
+                assertFalse( ((Comparable)enumObjectFromOtherClassLoader).compareTo(element) == 0);
+            }
+        }
+        assertEquals(ColorEnum.getEnumList().size() - 1, falseCount);
     }
 
     Object getColorEnum(ClassLoader classLoader, String color) throws ClassNotFoundException, SecurityException,

src/test/org/apache/commons/lang/enums/EnumTest.java

@@ -442,12 +442,12 @@ public final class EnumTest extends TestCase {
     
     public void testColorEnumEqualsWithDifferentClassLoaders() throws SecurityException, IllegalArgumentException,
             ClassNotFoundException, NoSuchMethodException, IllegalAccessException, InvocationTargetException {
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.BLUE);
+        this.testWithDifferentClassLoaders(ColorEnum.BLUE);
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.GREEN);
+        this.testWithDifferentClassLoaders(ColorEnum.GREEN);
-        this.testEqualsTrueWithDifferentClassLoaders(ColorEnum.RED);
+        this.testWithDifferentClassLoaders(ColorEnum.RED);
     }
 
-    void testEqualsTrueWithDifferentClassLoaders(ColorEnum colorEnum) throws ClassNotFoundException, SecurityException,
+    void testWithDifferentClassLoaders(ColorEnum colorEnum) throws ClassNotFoundException, SecurityException,
             NoSuchMethodException, IllegalArgumentException, IllegalAccessException, InvocationTargetException {
         // Sanity checks:
         assertTrue(colorEnum.equals(colorEnum));
@@ -455,6 +455,7 @@ public final class EnumTest extends TestCase {
         // set up:
         ClassLoader classLoader = ClassUtilsTest.newSystemClassLoader();
         Object enumObjectFromOtherClassLoader = this.getColorEnum(classLoader, colorEnum.getName());
+
         // the real test, part 1.
         try {
             ColorEnum testCase = (ColorEnum) enumObjectFromOtherClassLoader;
@@ -462,10 +463,12 @@ public final class EnumTest extends TestCase {
         } catch (ClassCastException e) {
             // normal.
         }
+
         // the real test, part 2.
         assertEquals("The two objects should match even though they are from different class loaders", colorEnum,
                 enumObjectFromOtherClassLoader);
-        // the real test, part 3.
+
+        // the real test, part 3 - testing equals(Object)
         int falseCount = 0;
         for (Iterator iter = ColorEnum.iterator(); iter.hasNext();) {
             ColorEnum element = (ColorEnum) iter.next();
@@ -475,6 +478,17 @@ public final class EnumTest extends TestCase {
             }
         }
         assertEquals(ColorEnum.getEnumList().size() - 1, falseCount);
+
+        // the real test, part 4 - testing compareTo(Object) == 0
+        falseCount = 0;
+        for (Iterator iter = ColorEnum.iterator(); iter.hasNext();) {
+            ColorEnum element = (ColorEnum) iter.next();
+            if (!colorEnum.equals(element)) {
+                falseCount++;
+                assertFalse( ((Comparable)enumObjectFromOtherClassLoader).compareTo(element) == 0);
+            }
+        }
+        assertEquals(ColorEnum.getEnumList().size() - 1, falseCount);
     }
 
     Object getColorEnum(ClassLoader classLoader, String color) throws ClassNotFoundException, SecurityException,