jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tobias Bocanegra (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OAK-3165) Redundant test for duplicate membership in Group.addMember
Date Wed, 29 Jul 2015 21:10:05 GMT

    [ https://issues.apache.org/jira/browse/OAK-3165?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14646764#comment-14646764
] 

Tobias Bocanegra commented on OAK-3165:
---------------------------------------

yes. you are correct. the {{MembershipWriter}} already checks if the user is part of the members.

(we could also remove the check and just search for the first property that has fewer values
as the defined max size. and/or we could remember where the last member was removed from.
however, keeping the check like this is more robust, I think)

> Redundant test for duplicate membership in Group.addMember
> ----------------------------------------------------------
>
>                 Key: OAK-3165
>                 URL: https://issues.apache.org/jira/browse/OAK-3165
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: core
>            Reporter: angela
>            Assignee: angela
>             Fix For: 1.3.4
>
>         Attachments: OAK-3165.patch
>
>
> while looking at the {{MembershipWriter.addMember}} again I spotted the following code:
> {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;
>                 }
>             }
>         }
> {code}
> 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message