glassfish
  1. glassfish
  2. GLASSFISH-20588

Principal Classes have serious problems

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: future release
    • Component/s: security
    • Labels:
      None

      Description

      Ref: Joshua Bloch, Effective Java, 2nd edition, item #8
      Mr. Bloch does an excellent job of describing precisely the problem in these classes:
      (all in common-util)
      common/common-util/src/main/java/org/glassfish/security/common/Role.java
      common/common-util/src/main/java/org/glassfish/security/common/Group.java
      common/common-util/src/main/java/org/glassfish/security/common/Principal.java
      3 problems:
      (1) These problems are the only low level FindBugs issues in all of common-util.
      (2) The equals/hash methods are a mess. Look at the FB results for common-utils. Especially look at the way the base class equals() method chedcks to see if the other object is a Group. Note how it doesn't check if it is a Role (why?). This is quite brittle. The base class ought to know nothing about subclasses!
      (3) Even though all three classes implement the Principal interface, the outside calling code does NOT use it. Instead, it uses PrincipalImpl. Why bother with an interface if it isn't used ? Should either use them as a class hierarchy OR as references to interfaces.
      I attempted to fix this using Bloch's recommendation which is to use composition instead of inheritance. I.e. I made all three implement the interface directly. Then Role and Group stopped being subclasses. This failed because outside code is looking for PrincipalImpl objects - not Principal objects.
      PrincipalImpl pi = new PrincipalImpl("foo");
      Group g = new Group("foo");
      Role r = new Role("foo");
      pi.equals(r) --> true
      r.equals(pi) --> false

        Activity

        There are no comments yet on this issue.

          People

          • Assignee:
            Craig Perez
            Reporter:
            Byron Nevins
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: