commit adc3ec1530a2a9d721634bcda7f000bb38c31ec5
author Pavlin Radoslavov <pavlin@onlab.us> Fri Apr 11 16:08:53 2014 -0700
committer Pavlin Radoslavov <pavlin@onlab.us> Mon Apr 14 17:43:02 2014 -0700
tree 5475f9538aecab9d28ea3a61f9ac74482b73ca2e
parent 1364a9356cc9a1b536578bc9dafae390198502c2

Fix issues found by FindBugs: EQ_OVERRIDING_EQUALS_NOT_SYMMETRIC

http://findbugs.sourceforge.net/bugDescriptions.html#EQ_OVERRIDING_EQUALS_NOT_SYMMETRIC

Note that the problem with implementing equals() in both the
base and derived class is fundamental:
http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals.html
http://www.artima.com/lejava/articles/equality.html

Note: The Floodlight fix in LLDPTLV.java and
LLDPOrganizationalTLV.java is incorrect, because inside LLDPTLV.java
the equals() implementation is using "instanceof" instead of "getClass()":

https://github.com/floodlight/floodlight/blob/master/src/main/java/net/floodlightcontroller/packet/LLDPTLV.java
https://github.com/floodlight/floodlight/blob/master/src/main/java/net/floodlightcontroller/packet/LLDPOrganizationalTLV.java

Change-Id: I0ff28a8ba1190036dbef87c6a0982c79bba115c8

src/main/java/net/onrc/onos/core/packet/LLDPOrganizationalTLV.java

diff --git a/src/main/java/net/onrc/onos/core/packet/LLDPOrganizationalTLV.java b/src/main/java/net/onrc/onos/core/packet/LLDPOrganizationalTLV.java
index beae222..14edd04 100644
--- a/src/main/java/net/onrc/onos/core/packet/LLDPOrganizationalTLV.java
+++ b/src/main/java/net/onrc/onos/core/packet/LLDPOrganizationalTLV.java
@@ -161,11 +161,16 @@
         if (o == this) {
             return true;
         }
-
-        if (!(o instanceof LLDPOrganizationalTLV)) {
+        if (!super.equals(o)) {
             return false;
         }
-
+        //
+        // NOTE: Subclasses are are considered as change of identity, hence
+        // equals() will return false if the class type doesn't match.
+        //
+        if (getClass() != o.getClass()) {
+            return false;
+        }
         LLDPOrganizationalTLV other = (LLDPOrganizationalTLV) o;
         if (this.type != other.type) {
             return false;

src/main/java/net/onrc/onos/core/packet/LLDPTLV.java

diff --git a/src/main/java/net/onrc/onos/core/packet/LLDPTLV.java b/src/main/java/net/onrc/onos/core/packet/LLDPTLV.java
index e23ce11..402a508 100644
--- a/src/main/java/net/onrc/onos/core/packet/LLDPTLV.java
+++ b/src/main/java/net/onrc/onos/core/packet/LLDPTLV.java
@@ -128,7 +128,25 @@
         if (obj == null) {
             return false;
         }
-        if (!(obj instanceof LLDPTLV)) {
+        //
+        // NOTE: Subclasses are are considered as change of identity, hence
+        // equals() will return false if the class type doesn't match.
+        //
+        // The implication is that two instances - base class and derived class
+        // will be different even if they have same bits on the wire.
+        // We use this assumption to address the fundamental
+        // "equivalence relation" issue with class inheritance and "equals()":
+        //    http://www.artima.com/lejava/articles/equality.html
+        //    http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals.html
+        //
+        // Based on existing code, we don't mix the usage of based and derived
+        // class instances.
+        //
+        // Note that the fix below is different from the Floodlight fix.
+        // The Floodlight code uses "if (!(obj instanceof LLDPTLV))", but
+        // that statement breaks the "equivalence relation".
+        //
+        if (getClass() != obj.getClass()) {
             return false;
         }
         LLDPTLV other = (LLDPTLV) obj;