Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: base
    • Labels:
      None

      Description

      FlexibleMapAccessor.java:44, SE_NO_SERIALVERSIONID
      SnVI: org.apache.ofbiz.base.util.collections.FlexibleMapAccessor is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • FlexibleServletAccessor.java:47, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.FlexibleServletAccessor is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • FlexibleServletAccessor.java:181, EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS
        Eq: org.apache.ofbiz.base.util.collections.FlexibleServletAccessor.equals(Object) checks for operand being a String

      This equals method is checking to see if the argument is some incompatible type (i.e., a class that is neither a supertype nor subtype of the class that defines the equals method). For example, the Foo class might have an equals method that looks like:

      public boolean equals(Object o) {
      if (o instanceof Foo)
      return name.equals(((Foo)o).name);
      else if (o instanceof String)
      return name.equals(o);
      else return false;
      This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive. Without those properties, very unexpected behavoirs are possible.

      • FlexibleServletAccessor.java:208, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.FlexibleServletAccessor$AttributeAccessor is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • GenericMap.java:68, HE_EQUALS_USE_HASHCODE
        HE: org.apache.ofbiz.base.util.collections.GenericMap defines equals and uses Object.hashCode()

      This class overrides equals(Object), but does not override hashCode(), and inherits the implementation of hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM). Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes.

      If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is:

      public int hashCode() {
      assert false : "hashCode not designed";
      return 42; // any arbitrary constant will do
      }

      • GenericMapValues.java:45, EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS
        Eq: org.apache.ofbiz.base.util.collections.GenericMapValues.equals(Object) checks for operand being a java.util.List

      This equals method is checking to see if the argument is some incompatible type (i.e., a class that is neither a supertype nor subtype of the class that defines the equals method). For example, the Foo class might have an equals method that looks like:

      public boolean equals(Object o) {
      if (o instanceof Foo)
      return name.equals(((Foo)o).name);
      else if (o instanceof String)
      return name.equals(o);
      else return false;
      This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive. Without those properties, very unexpected behavoirs are possible.

      • GenericMapValues.java:45, EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS
        Eq: org.apache.ofbiz.base.util.collections.GenericMapValues.equals(Object) checks for operand being a java.util.Set

      This equals method is checking to see if the argument is some incompatible type (i.e., a class that is neither a supertype nor subtype of the class that defines the equals method). For example, the Foo class might have an equals method that looks like:

      public boolean equals(Object o) {
      if (o instanceof Foo)
      return name.equals(((Foo)o).name);
      else if (o instanceof String)
      return name.equals(o);
      else return false;
      This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive. Without those properties, very unexpected behavoirs are possible.

      • LRUMap.java:33, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.LRUMap is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • LifoSet.java:35, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.LifoSet is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • MapComparator.java:32, SE_COMPARATOR_SHOULD_BE_SERIALIZABLE
        Se: org.apache.ofbiz.base.util.collections.MapComparator implements Comparator but not Serializable

      This class implements the Comparator interface. You should consider whether or not it should also implement the Serializable interface. If a comparator is used to construct an ordered collection such as a TreeMap, then the TreeMap will be serializable only if the comparator is also serializable. As most comparators have little or no state, making them serializable is generally easy and good defensive programming.

      • MapComparator.java:51, NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT
        NP: org.apache.ofbiz.base.util.collections.MapComparator.equals(Object) does not check for null argument

      This implementation of equals(Object) violates the contract defined by java.lang.Object.equals() because it does not check for null being passed as the argument. All equals() methods should return false if passed a null value.

      • MapComparator.java:51, HE_EQUALS_USE_HASHCODE
        HE: org.apache.ofbiz.base.util.collections.MapComparator defines equals and uses Object.hashCode()

      This class overrides equals(Object), but does not override hashCode(), and inherits the implementation of hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM). Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes.

      If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is:

      public int hashCode() {
      assert false : "hashCode not designed";
      return 42; // any arbitrary constant will do
      }

      • MapContext.java:-1, CI_CONFUSED_INHERITANCE
        CI: Class org.apache.ofbiz.base.util.collections.MapContext$ListSet is final but declares protected field org.apache.ofbiz.base.util.collections.MapContext$ListSet.listImpl

      This class is declared to be final, but declares fields to be protected. Since the class is final, it can not be derived from, and the use of protected is confusing. The access modifier for the field should be changed to private or public to represent the true use for the field.

      • MapContext.java:345, RI_REDUNDANT_INTERFACES
        RI: Class org.apache.ofbiz.base.util.collections.MapContext$ListSet implements same interface as superclass

      This class declares that it implements an interface that is also implemented by a superclass. This is redundant because once a superclass implements an interface, all subclasses by default also implement this interface. It may point out that the inheritance hierarchy has changed since this class was created, and consideration should be given to the ownership of the interface's implementation.

      • ResourceBundleMapWrapper.java:-1, SE_BAD_FIELD
        Se: Class org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper defines non-transient non-serializable instance field initialResourceBundle

      This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods. Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

      • ResourceBundleMapWrapper.java:-1, SE_BAD_FIELD
        Se: Class org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper defines non-transient non-serializable instance field rbmwStack

      This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods. Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

      • ResourceBundleMapWrapper.java:-1, SE_BAD_FIELD
        Se: Class org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper$InternalRbmWrapper defines non-transient non-serializable instance field resourceBundle

      This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods. Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

      • ResourceBundleMapWrapper.java:36, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • ResourceBundleMapWrapper.java:153, SE_NO_SERIALVERSIONID
        SnVI: org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper$InternalRbmWrapper is Serializable; consider declaring a serialVersionUID

      This class implements the Serializable interface, but does not define a serialVersionUID field. A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String). Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes. To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

      • ResourceBundleMapWrapper.java:223, RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
        RCN: Redundant nullcheck of java.util.ResourceBundle.getObject(String), which is known to be non-null in org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper$InternalRbmWrapper.containsKey(Object)

      This method contains a redundant check of a known non-null value against the constant null.

        Activity

        Hide
        Dennis Balkir Dennis Balkir added a comment -
        • fixed Diamond Operators

        class FlexibleMapAccessor:

        • Line 44: didn’t do anything, serialVersionUID is not necessary for this class

        class FlexibleServletAccessor:

        • deleted unnecessary else-blocks
        • Line 47: didn’t do anything, serialVersionUID is not necessary for this class
        • deleted unnecessary casting in equals and added a nullcheck, to prevent a casting exception; findbugs bug still there, equals method isn’t really used for what it’s supposed to be used
        • Line 210: didn’t do anything, serialVersionUID is not necessary for this class

        class GenericMap:

        • didn’t find circular dependency
        • Line 68: didn’t implement hashCode, not necessary

        class GenericMapValues:

        • Line 45: didn’t change anything, hopefully it was intended like this

        class LRUMap:

        • didn’t do anything, serialVersionUID is not necessary for this class

        class LifoSet:

        • didn’t do anything, serialVersionUID is not necessary for this class

        class MapComparator:

        • class doesn’t implement Serializable, has not to be changed
        • added nullcheck in equals which returns false, if a null object is given
        • implemented hashCode(), because equals is implemented
        • deleted unnecessary else-block

        class MapContext:

        • Line 341: deleted the statement, that class ListSet implements Set, because its superclass already does it, therefore ListSet automatically does it too
        • changed listImpl from protected to private since the class ListSet is final
        • deleted unnecessary else-blocks

        class ResourceBundleMapWrapper:

        • Line 37: didn’t do anything, serialVersionUID is not necessary for this class
        • Line 39 & 40: couldn’t change the implementation
        • Line 153: couldn’t change the implementation
        • Line 221: deleted unnecessary nullcheck; if arg0 is null, the thrown exception will be caught, if not it still returns true
        • deleted unnecessary else-blocks
        Show
        Dennis Balkir Dennis Balkir added a comment - fixed Diamond Operators class FlexibleMapAccessor: Line 44: didn’t do anything, serialVersionUID is not necessary for this class class FlexibleServletAccessor: deleted unnecessary else-blocks Line 47: didn’t do anything, serialVersionUID is not necessary for this class deleted unnecessary casting in equals and added a nullcheck, to prevent a casting exception; findbugs bug still there, equals method isn’t really used for what it’s supposed to be used Line 210: didn’t do anything, serialVersionUID is not necessary for this class class GenericMap: didn’t find circular dependency Line 68: didn’t implement hashCode , not necessary class GenericMapValues: Line 45: didn’t change anything, hopefully it was intended like this class LRUMap: didn’t do anything, serialVersionUID is not necessary for this class class LifoSet: didn’t do anything, serialVersionUID is not necessary for this class class MapComparator: class doesn’t implement Serializable, has not to be changed added nullcheck in equals which returns false, if a null object is given implemented hashCode() , because equals is implemented deleted unnecessary else-block class MapContext: Line 341: deleted the statement, that class ListSet implements Set , because its superclass already does it, therefore ListSet automatically does it too changed listImpl from protected to private since the class ListSet is final deleted unnecessary else-blocks class ResourceBundleMapWrapper: Line 37: didn’t do anything, serialVersionUID is not necessary for this class Line 39 & 40: couldn’t change the implementation Line 153: couldn’t change the implementation Line 221: deleted unnecessary nullcheck; if arg0 is null, the thrown exception will be caught, if not it still returns true deleted unnecessary else-blocks
        Hide
        mbrohl Michael Brohl added a comment -

        Hi Dennis,

        after applying this patch, the tests are failing with the newest trunk revision. Can you please check?

        Thanks and regards,
        Michael

        Show
        mbrohl Michael Brohl added a comment - Hi Dennis, after applying this patch, the tests are failing with the newest trunk revision. Can you please check? Thanks and regards, Michael
        Hide
        mbrohl Michael Brohl added a comment -

        Thanks Dennis,

        your patch is in trunk r1811685.

        Show
        mbrohl Michael Brohl added a comment - Thanks Dennis, your patch is in trunk r1811685.

          People

          • Assignee:
            mbrohl Michael Brohl
            Reporter:
            Dennis Balkir Dennis Balkir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development