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

Move check for cyclic membership to GroupImpl



    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.7.0, 1.8.0
    • core
    • None


      while investigating potential improvements for adding/removing group members, I noticed that the way we check for cyclic group membership in the UserValidator may limit our options wrt arranging member references in the tree defined by the rep:Group node.

      With the current diff mechanism the only option for spotting cyclic membership in the UserValidator is by looking at modifications/addition of rep:members properties. However, if adding a new member reference would not just be appended to an existing or new property but instead would otherwise change the structure of these properties (e.g. by inserting and splitting), we may end up re-checking member references that have already been verified; just because they look like being added in the diff.

      Consequently, we may consider moving the check to the GroupImpl and limit it to those cases where the member is either passed to the addMember call or is being resolved as it may happen during addMembers.

      • more options to improve content structure of member references to improve write performance
      • only a single resolution of memberId/memberReference to the member itself (in GroupImpl)
      • no additional resolution of member-reference(s) during commit validation
      • improved performance for ImportBehavior.BEST_EFFORT that doesn't require the new member to exist.
      • check for cyclic membership only in the user management implementation, not enforced upon commit
      • ImportBehavior.BEST_EFFORT will not spot cyclic membership

      alex.parvulescu, what do you think?


        1. OAK-6072.patch
          8 kB
          Angela Schreiber
        2. OAK-6072-tests.patch
          25 kB
          Angela Schreiber

        Issue Links



              angela Angela Schreiber
              angela Angela Schreiber
              0 Vote for this issue
              1 Start watching this issue