Add GroupManager#isMember(member, group, recurse)

Development
1

Hi devs,

While deprecating com.xpn.xwiki.api.User#isUserInGroup(String) (XWIKI-22786) I noticed that
org.xwiki.user.group.GroupManager — the recommended replacement — has no way to answer the single most common question about groups: is this user a member of this group?

Today the only way is to materialize a collection and search it:

// Java
boolean member = this.groupManager.getMembers(groupReference, false).contains(userReference);

## Velocity
#set ($member = $services.user.group.getMembers($groupReference, false).contains($userReference))

For comparison, the deprecated method it replaces was simply
$xwiki.getUser().isUserInGroup('XWiki.SomeGroup'). Deprecating a one-liner in favour of the above
is a hard sell, and it pushes every caller into re-implementing the same idiom.

Proposal

Add to GroupManager:

/**
 * Indicate if the passed member (user or group) is a member of the passed group.
 *
 * @param member the group member (user or group)
 * @param group the group to check
 * @param recurse false to only check direct membership, true to also take into account groups of
 *            groups
 * @return {@code true} if the passed member is a member of the passed group
 * @throws GroupException when failing to get the group members
 * @since 18.7.0RC1
 */
default boolean isMember(DocumentReference member, DocumentReference group, boolean recurse)
    throws GroupException
{
    return getMembers(group, recurse).contains(member);
}

plus the matching passthrough on GroupScriptService (role hint user.group), so scripts get
$services.user.group.isMember($userReference, $groupReference, false).

Why a default method

Adding an abstract method to GroupManager would be a binary break for any out-of-repo implementor and Revapi would (correctly) fail the build. A default method preserves binary compatibility for existing implementors, which is what our backward-compatibility policy asks for
(https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HBackwardCompatibility).

The default body delegates to the existing getMembers, so it never throws UnsupportedOperationException — also per policy. The point of putting it on the interface rather than in a helper class is that DefaultGroupManager can then override it with a cheaper check: the default implementation materializes the whole member collection, which is wasteful for very large groups (think XWikiAllGroup). Whether we do that override immediately or later is an
implementation detail, not an API question.

Open question for the list

Parameter order. isMember(member, group, recurse) reads naturally (“is member a member of group”), but both parameters are DocumentReference, so getting them backwards compiles fine and silently returns the wrong answer. The alternative, isMember(group, member, recurse), is consistent with the existing getMembers(group, recurse) / getGroups(member, …) convention but reads backwards. I lean towards (member, group, recurse) for readability — opinions welcome.

WDYT?

Thanks,
Vincent

2

Wouldn’t it be better to use getGroups(DocumentReference member, Object wikiTarget, boolean recurse) for the fallback, assuming that most users should be in a few groups, but groups could contain thousands of members? I know that from a data storage point of view, getMembers could be cheaper, though, so without caching it might be cheaper (but we have caching, so repeated calls would be cheaper).

Apart from that, +1, sounds good. Would be nice if we had a proper user/group reference so we don’t continue introducing APIs with the “legacy” DocumentReference in the role of a UserReference but I fear that won’t happen anytime soon.

3

+1 for the proposed signature.

Yes, getGroups would probably make more sense as fallback.

4

Yes, that GroupManager predates the introduction of the newer User API. We need to progress on the new Group API.

Thx

5

Note that UserReference is not really relevant here since we don’t pass a user reference, we pass a member (which can be a user or a group, and we don’t have any API for that).

6

Yes, that’s correct and that’s what the link I gave was about: User and Group APIs

When I said “User API” I really meant “User and Group API”, they go together. It’s a pity I didn’t implement them together in the “new” api.