Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-3165

Redundant test for duplicate membership in Group.addMember

    XMLWordPrintableJSON

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.2.11, 1.3.4, 1.4
    • core
    • None

    Description

      while looking at the MembershipWriter.addMember again I spotted the following code:

      int bestCount = membershipSizeThreshold;
              PropertyState bestProperty = null;
              Tree bestTree = null;
              while (trees.hasNext()) {
                  Tree t = trees.next();
                  PropertyState refs = t.getProperty(UserConstants.REP_MEMBERS);
                  if (refs != null) {
                      int numRefs = 0;
                      for (String ref : refs.getValue(Type.WEAKREFERENCES)) {
                          if (ref.equals(memberContentId)) {
                              return false;
                          }
                          numRefs++;
                      }
                      if (numRefs < bestCount) {
                          bestCount = numRefs;
                          bestProperty = refs;
                          bestTree = t;
                      }
                  }
              }
      

      the fact that loop doesn't stop once the first bestProperty has been found and there is an explicit test for the the references already containing the contentID of the member to be added, indicates to me that the additional test for isDeclaredMember(newMember) in GroupImpl is redundant and can be omitted.

      proposed improvement and some additional test attached as patch.
      tripod, i would appreciate if you could confirm that my reading of your code is correct.

      Attachments

        1. OAK-3165.patch
          3 kB
          Angela Schreiber

        Issue Links

          Activity

            People

              angela Angela Schreiber
              angela Angela Schreiber
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: