Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.0.5
    • Fix Version/s: 3.0.6, 3.1.0.RC3
    • Component/s: ACLs
    • Labels:
      None

      Description

      Changing the the parent ACL of an AclImpl when it has a parent already causes a NullpointException in AclImpl.equals(.)

      The unit test (paste into AclImplTests.java) proves it.

      @Test
      public void testChangeParent() throws Exception

      { AclImpl parentAcl = new AclImpl(objectIdentity, 1L, mockAuthzStrategy, mockAuditLogger); AclImpl childAcl = new AclImpl(objectIdentity, 2L, mockAuthzStrategy, mockAuditLogger); AclImpl changeParentAcl = new AclImpl(objectIdentity, 3L, mockAuthzStrategy, mockAuditLogger); // This works childAcl.setParent(parentAcl); childAcl.setParent(null); // setting to null first avoids NPE childAcl.setParent(changeParentAcl); // This causes an NPE childAcl.setParent(parentAcl); }

        Activity

        Hide
        Simon van der Sluis added a comment -

        A fix in AclImpl.equals(.) as shown below fixes it.

        public boolean equals(Object obj) {
        if (obj instanceof AclImpl) {
        AclImpl rhs = (AclImpl) obj;
        if (this.aces.equals(rhs.aces)) {
        if ((this.parentAcl == null && rhs.parentAcl == null) || (this.parentAcl !=null && this.parentAcl.equals(rhs.parentAcl))) {
        if ((this.objectIdentity == null && rhs.objectIdentity == null) || (this.objectIdentity.equals(rhs.objectIdentity))) {
        if ((this.id == null && rhs.id == null) || (this.id.equals(rhs.id))) {
        if ((this.owner == null && rhs.owner == null) || this.owner.equals(rhs.owner)) {
        if (this.entriesInheriting == rhs.entriesInheriting) {
        if ((this.loadedSids == null && rhs.loadedSids == null))

        { return true; }

        if (this.loadedSids.size() == rhs.loadedSids.size()) {
        for (int i = 0; i < this.loadedSids.size(); i++) {
        if (!this.loadedSids.get.equals(rhs.loadedSids.get))

        { return false; }

        }
        return true;
        }
        }
        }
        }
        }
        }
        }
        }
        return false;
        }

        Show
        Simon van der Sluis added a comment - A fix in AclImpl.equals(.) as shown below fixes it. public boolean equals(Object obj) { if (obj instanceof AclImpl) { AclImpl rhs = (AclImpl) obj; if (this.aces.equals(rhs.aces)) { if ((this.parentAcl == null && rhs.parentAcl == null) || ( this.parentAcl !=null && this.parentAcl.equals(rhs.parentAcl))) { if ((this.objectIdentity == null && rhs.objectIdentity == null) || (this.objectIdentity.equals(rhs.objectIdentity))) { if ((this.id == null && rhs.id == null) || (this.id.equals(rhs.id))) { if ((this.owner == null && rhs.owner == null) || this.owner.equals(rhs.owner)) { if (this.entriesInheriting == rhs.entriesInheriting) { if ((this.loadedSids == null && rhs.loadedSids == null)) { return true; } if (this.loadedSids.size() == rhs.loadedSids.size()) { for (int i = 0; i < this.loadedSids.size(); i++) { if (!this.loadedSids.get .equals(rhs.loadedSids.get )) { return false; } } return true; } } } } } } } } return false; }
        Hide
        Simon van der Sluis added a comment -

        I had hoped that wiki markup would work in the above comment, instead the *'s show where the code was modified.

        Show
        Simon van der Sluis added a comment - I had hoped that wiki markup would work in the above comment, instead the *'s show where the code was modified.
        Hide
        Simon van der Sluis added a comment -

        It's a pretty ugly piece of code for an equals method, would it be better to use an equals builder from apache-commons-lang?

        Show
        Simon van der Sluis added a comment - It's a pretty ugly piece of code for an equals method, would it be better to use an equals builder from apache-commons-lang?
        Hide
        Simon van der Sluis added a comment -

        And since equals(.) is implemented, hashCode() probably should be too.

        Show
        Simon van der Sluis added a comment - And since equals(.) is implemented, hashCode() probably should be too.
        Hide
        Simon van der Sluis added a comment -

        If it's not clear from the junit, setting the parent to null before changing it to a new value is a simple workaround for this bug.

        Show
        Simon van der Sluis added a comment - If it's not clear from the junit, setting the parent to null before changing it to a new value is a simple workaround for this bug.
        Hide
        Luke Taylor added a comment -

        Thanks. I've fixed that plus some other possible NPE occurrences in the same method.

        Show
        Luke Taylor added a comment - Thanks. I've fixed that plus some other possible NPE occurrences in the same method.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Simon van der Sluis
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: