opendev/gerrit
Code Issues Proposed changes

Merge "Fix nested group expansion"

Browse Source
This commit is contained in:
Shawn Pearce
2013-01-18 21:35:24 +00:00
committed by Gerrit Code Review
parent 0bad0857ba 8f07bbbd90
commit 244a3f522d
6 changed files with 158 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java
View File

@@ -165,7 +165,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
public GroupDetail run(ReviewDb db) throws OrmException, Failure, public GroupDetail run(ReviewDb db) throws OrmException, Failure,
NoSuchGroupException { NoSuchGroupException {
final GroupControl control = groupControlFactory.validateFor(groupId); final GroupControl control = groupControlFactory.validateFor(groupId);
if (groupCache.get(groupId).getType() != AccountGroup.Type.INTERNAL) { AccountGroup group = groupCache.get(groupId);
if (group.getType() != AccountGroup.Type.INTERNAL) {
throw new Failure(new NameAlreadyUsedException()); throw new Failure(new NameAlreadyUsedException());
} }
@@ -186,7 +187,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
Collections.singleton(new AccountGroupIncludeByUuidAudit(m, Collections.singleton(new AccountGroupIncludeByUuidAudit(m,
getAccountId()))); getAccountId())));
db.accountGroupIncludesByUuid().insert(Collections.singleton(m)); db.accountGroupIncludesByUuid().insert(Collections.singleton(m));
groupIncludeCache.evictInclude(incGroupUUID); groupIncludeCache.evictMemberIn(incGroupUUID);
groupIncludeCache.evictMembersOf(group.getGroupUUID());
} }
return groupDetailFactory.create(groupId).call(); return groupDetailFactory.create(groupId).call();
@@ -201,7 +203,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
public VoidResult run(final ReviewDb db) throws OrmException, public VoidResult run(final ReviewDb db) throws OrmException,
NoSuchGroupException, Failure { NoSuchGroupException, Failure {
final GroupControl control = groupControlFactory.validateFor(groupId); final GroupControl control = groupControlFactory.validateFor(groupId);
if (groupCache.get(groupId).getType() != AccountGroup.Type.INTERNAL) { AccountGroup group = groupCache.get(groupId);
if (group.getType() != AccountGroup.Type.INTERNAL) {
throw new Failure(new NameAlreadyUsedException()); throw new Failure(new NameAlreadyUsedException());
} }
@@ -241,7 +244,10 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
} }
} }
for (AccountGroup.UUID uuid : groupsToEvict) { for (AccountGroup.UUID uuid : groupsToEvict) {
groupIncludeCache.evictInclude(uuid); groupIncludeCache.evictMemberIn(uuid);
}
if (!groupsToEvict.isEmpty()) {
groupIncludeCache.evictMembersOf(group.getGroupUUID());
} }
return VoidResult.INSTANCE; return VoidResult.INSTANCE;
} }

11
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
View File

@@ -16,12 +16,17 @@ package com.google.gerrit.server.account;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.Collection; import java.util.Set;
/** Tracks group inclusions in memory for efficient access. */ /** Tracks group inclusions in memory for efficient access. */
public interface GroupIncludeCache { public interface GroupIncludeCache {
public Collection<AccountGroup.UUID> getByInclude(AccountGroup.UUID groupId); /** @return groups directly a member of the passed group. */
public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID group);
public void evictInclude(AccountGroup.UUID groupId); /** @return any groups the passed group belongs to. */
public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId);
public void evictMembersOf(AccountGroup.UUID groupId);
public void evictMemberIn(AccountGroup.UUID groupId);
} }

75
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
View File

@@ -32,7 +32,6 @@ import com.google.inject.name.Named;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
import java.util.Collection;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Set; import java.util.Set;
@@ -44,6 +43,7 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
private static final Logger log = LoggerFactory private static final Logger log = LoggerFactory
.getLogger(GroupIncludeCacheImpl.class); .getLogger(GroupIncludeCacheImpl.class);
private static final String BYINCLUDE_NAME = "groups_byinclude"; private static final String BYINCLUDE_NAME = "groups_byinclude";
private static final String MEMBERS_NAME = "groups_members";
public static Module module() { public static Module module() {
return new CacheModule() { return new CacheModule() {
@@ -52,7 +52,12 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
cache(BYINCLUDE_NAME, cache(BYINCLUDE_NAME,
AccountGroup.UUID.class, AccountGroup.UUID.class,
new TypeLiteral<Set<AccountGroup.UUID>>() {}) new TypeLiteral<Set<AccountGroup.UUID>>() {})
.loader(ByIncludeLoader.class); .loader(MemberInLoader.class);
cache(MEMBERS_NAME,
AccountGroup.UUID.class,
new TypeLiteral<Set<AccountGroup.UUID>>() {})
.loader(MembersOfLoader.class);
bind(GroupIncludeCacheImpl.class); bind(GroupIncludeCacheImpl.class);
bind(GroupIncludeCache.class).to(GroupIncludeCacheImpl.class); bind(GroupIncludeCache.class).to(GroupIncludeCacheImpl.class);
@@ -60,35 +65,83 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
}; };
} }
private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> byInclude; private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf;
private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn;
@Inject @Inject
GroupIncludeCacheImpl( GroupIncludeCacheImpl(
@Named(BYINCLUDE_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> byInclude) { @Named(MEMBERS_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf,
this.byInclude = byInclude; @Named(BYINCLUDE_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn) {
this.membersOf = membersOf;
this.memberIn = memberIn;
} }
public Collection<AccountGroup.UUID> getByInclude(AccountGroup.UUID groupId) { public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID groupId) {
try { try {
return byInclude.get(groupId); return membersOf.get(groupId);
} catch (ExecutionException e) {
log.warn("Cannot load members of group", e);
return Collections.emptySet();
}
}
public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId) {
try {
return memberIn.get(groupId);
} catch (ExecutionException e) { } catch (ExecutionException e) {
log.warn("Cannot load included groups", e); log.warn("Cannot load included groups", e);
return Collections.emptySet(); return Collections.emptySet();
} }
} }
public void evictInclude(AccountGroup.UUID groupId) { public void evictMembersOf(AccountGroup.UUID groupId) {
if (groupId != null) { if (groupId != null) {
byInclude.invalidate(groupId); membersOf.invalidate(groupId);
} }
} }
static class ByIncludeLoader extends public void evictMemberIn(AccountGroup.UUID groupId) {
if (groupId != null) {
memberIn.invalidate(groupId);
}
}
static class MembersOfLoader extends
CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> { CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema; private final SchemaFactory<ReviewDb> schema;
@Inject @Inject
ByIncludeLoader(final SchemaFactory<ReviewDb> sf) { MembersOfLoader(final SchemaFactory<ReviewDb> sf) {
schema = sf;
}
@Override
public Set<AccountGroup.UUID> load(AccountGroup.UUID key) throws Exception {
final ReviewDb db = schema.open();
try {
List<AccountGroup> group = db.accountGroups().byUUID(key).toList();
if (group.size() != 1) {
return Collections.emptySet();
}
Set<AccountGroup.UUID> ids = Sets.newHashSet();
for (AccountGroupIncludeByUuid agi : db.accountGroupIncludesByUuid()
.byGroup(group.get(0).getId())) {
ids.add(agi.getIncludeUUID());
}
return ImmutableSet.copyOf(ids);
} finally {
db.close();
}
}
}
static class MemberInLoader extends
CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
@Inject
MemberInLoader(final SchemaFactory<ReviewDb> sf) {
schema = sf; schema = sf;
} }

108
gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
View File

@@ -14,36 +14,49 @@
package com.google.gerrit.server.account; package com.google.gerrit.server.account;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import java.util.Collections; import java.util.List;
import java.util.Queue; import java.util.Map;
import java.util.Set; import java.util.Set;
/** /**
* Creates a GroupMembership checker for the internal group system, which * Group membership checker for the internal group system.
* starts with the seed groups and includes all child groups. * <p>
* Groups the user is directly a member of are pulled from the in-memory
* AccountCache by way of the IdentifiedUser. Transitive group memberhips are
* resolved on demand starting from the requested group and looking for a path
* to a group the user is a member of. Other group backends are supported by
* recursively invoking the universal GroupMembership.
*/ */
public class IncludingGroupMembership implements GroupMembership { public class IncludingGroupMembership implements GroupMembership {
public interface Factory { public interface Factory {
IncludingGroupMembership create(Iterable<AccountGroup.UUID> groupIds); IncludingGroupMembership create(IdentifiedUser user);
} }
private final GroupIncludeCache groupIncludeCache; private final GroupIncludeCache includeCache;
private final Set<AccountGroup.UUID> includes; private final IdentifiedUser user;
private final Queue<AccountGroup.UUID> groupQueue; private final Map<AccountGroup.UUID, Boolean> memberOf;
private Set<AccountGroup.UUID> knownGroups;
@Inject @Inject
IncludingGroupMembership( IncludingGroupMembership(GroupIncludeCache includeCache,
GroupIncludeCache groupIncludeCache, @Assisted IdentifiedUser user) {
@Assisted Iterable<AccountGroup.UUID> seedGroups) { this.includeCache = includeCache;
this.groupIncludeCache = groupIncludeCache; this.user = user;
this.includes = Sets.newHashSet(seedGroups);
this.groupQueue = Lists.newLinkedList(seedGroups); Set<AccountGroup.UUID> groups = user.state().getInternalGroups();
memberOf = Maps.newHashMapWithExpectedSize(groups.size());
for (AccountGroup.UUID g : groups) {
memberOf.put(g, true);
}
} }
@Override @Override
@@ -51,23 +64,40 @@ public class IncludingGroupMembership implements GroupMembership {
if (id == null) { if (id == null) {
return false; return false;
} }
if (includes.contains(id)) {
return true; Boolean b = memberOf.get(id);
} return b != null ? b : containsAnyOf(ImmutableSet.of(id));
return findIncludedGroup(Collections.singleton(id));
} }
@Override @Override
public boolean containsAnyOf(Iterable<AccountGroup.UUID> ids) { public boolean containsAnyOf(Iterable<AccountGroup.UUID> queryIds) {
Set<AccountGroup.UUID> query = Sets.newHashSet(); // Prefer lookup of a cached result over expanding includes.
for (AccountGroup.UUID groupId : ids) { boolean tryExpanding = false;
if (includes.contains(groupId)) { for (AccountGroup.UUID id : queryIds) {
Boolean b = memberOf.get(id);
if (b == null) {
tryExpanding = true;
} else if (b) {
return true; return true;
} }
query.add(groupId);
} }
return findIncludedGroup(query); if (tryExpanding) {
for (AccountGroup.UUID id : queryIds) {
if (memberOf.containsKey(id)) {
// Membership was earlier proven to be false.
continue;
}
memberOf.put(id, false);
if (search(includeCache.membersOf(id))) {
memberOf.put(id, true);
return true;
}
}
}
return false;
} }
@Override @Override
@@ -81,25 +111,31 @@ public class IncludingGroupMembership implements GroupMembership {
return r; return r;
} }
private boolean findIncludedGroup(Set<AccountGroup.UUID> query) { private boolean search(Set<AccountGroup.UUID> ids) {
boolean found = false; return user.getEffectiveGroups().containsAnyOf(ids);
while (!found && !groupQueue.isEmpty()) { }
AccountGroup.UUID id = groupQueue.remove();
for (final AccountGroup.UUID groupId : groupIncludeCache.getByInclude(id)) { private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {
if (includes.add(groupId)) { Set<AccountGroup.UUID> direct = user.state().getInternalGroups();
groupQueue.add(groupId); Set<AccountGroup.UUID> r = Sets.newHashSet(direct);
found |= query.contains(groupId); List<AccountGroup.UUID> q = Lists.newArrayList(r);
while (!q.isEmpty()) {
AccountGroup.UUID id = q.remove(q.size() - 1);
for (AccountGroup.UUID g : includeCache.memberIn(id)) {
if (r.add(g)) {
q.add(g);
memberOf.put(g, true);
} }
} }
} }
return ImmutableSet.copyOf(r);
return found;
} }
@Override @Override
public Set<AccountGroup.UUID> getKnownGroups() { public Set<AccountGroup.UUID> getKnownGroups() {
findIncludedGroup(Collections.<AccountGroup.UUID>emptySet()); // find all if (knownGroups == null) {
return Sets.newHashSet(includes); knownGroups = computeKnownGroups();
}
return knownGroups;
} }
} }

7
gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
View File

@@ -28,9 +28,7 @@ import com.google.inject.Singleton;
import java.util.Collection; import java.util.Collection;
/** /** Implementation of GroupBackend for the internal group system. */
* Implementation of GroupBackend for the internal group system.
*/
@Singleton @Singleton
public class InternalGroupBackend implements GroupBackend { public class InternalGroupBackend implements GroupBackend {
private static final Function<AccountGroup, GroupReference> ACT_GROUP_TO_GROUP_REF = private static final Function<AccountGroup, GroupReference> ACT_GROUP_TO_GROUP_REF =
@@ -45,7 +43,6 @@ public class InternalGroupBackend implements GroupBackend {
private final GroupCache groupCache; private final GroupCache groupCache;
private final IncludingGroupMembership.Factory groupMembershipFactory; private final IncludingGroupMembership.Factory groupMembershipFactory;
@Inject @Inject
InternalGroupBackend(GroupControl.Factory groupControlFactory, InternalGroupBackend(GroupControl.Factory groupControlFactory,
GroupCache groupCache, GroupCache groupCache,
@@ -89,6 +86,6 @@ public class InternalGroupBackend implements GroupBackend {
@Override @Override
public GroupMembership membershipsOf(IdentifiedUser user) { public GroupMembership membershipsOf(IdentifiedUser user) {
return groupMembershipFactory.create(user.state().getInternalGroups()); return groupMembershipFactory.create(user);
} }
} }

3
gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java
View File

@@ -128,6 +128,7 @@ public class PerformCreateGroup {
if (initialGroups != null) { if (initialGroups != null) {
addGroups(groupId, initialGroups); addGroups(groupId, initialGroups);
groupIncludeCache.evictMembersOf(uuid);
} }
groupCache.onCreateGroup(nameKey); groupCache.onCreateGroup(nameKey);
@@ -177,7 +178,7 @@ public class PerformCreateGroup {
db.accountGroupIncludesByUuidAudit().insert(includesAudit); db.accountGroupIncludesByUuidAudit().insert(includesAudit);
for (AccountGroup.UUID uuid : groups) { for (AccountGroup.UUID uuid : groups) {
groupIncludeCache.evictInclude(uuid); groupIncludeCache.evictMemberIn(uuid);
} }
} }
} }