opendev
/
gerrit
Code Issues Proposed changes

Revert "Perform user.getEffectiveGroups() less eagerly" 

This reverts commit f7569d0cb2.
The implementation removes any ability for slow GroupBackends
to perform their own per-request group level caching.

The correct way to do less eager lookups is for the LDAP
implementation of GroupBackend to create an emtpy membership
and then populate it on demand as requests arrive and need
to be looked up in the contains() methods.

Change-Id: I65210b24bffe10c5b158668867c8167c84712d17
This commit is contained in:
Shawn Pearce 2015-02-17 17:13:25 -08:00
parent 5d9fc0ce21
commit 7ab135e863
17 changed files with 27 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -14,8 +14,6 @@
package com.google.gerrit.server;
import com.google.common.collect.Iterables;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
@ -40,11 +38,6 @@ public class AnonymousUser extends CurrentUser {
return new ListGroupMembership(Collections.singleton(SystemGroupBackend.ANONYMOUS_USERS));
}
@Override
public boolean memberOfAny(Iterable<AccountGroup.UUID> ids) {
return Iterables.contains(ids, SystemGroupBackend.ANONYMOUS_USERS);
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

10
gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
View File

@ -14,14 +14,12 @@
package com.google.gerrit.server;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
import com.google.inject.servlet.RequestScoped;
import java.util.Arrays;
import java.util.Collection;
import java.util.Set;
@ -79,14 +77,6 @@ public abstract class CurrentUser {
*/
public abstract GroupMembership getEffectiveGroups();
public boolean memberOfAny(Iterable<AccountGroup.UUID> ids) {
return getEffectiveGroups().containsAnyOf(ids);
}
public boolean memberOf(AccountGroup.UUID id) {
return memberOfAny(Arrays.asList(id));
}
/** Set of changes starred by this user. */
public abstract Set<Change.Id> getStarredChanges();

6
gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
View File

@ -20,7 +20,6 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.AccountInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountDiffPreference;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.StarredChange;
@ -325,11 +324,6 @@ public class IdentifiedUser extends CurrentUser {
return effectiveGroups;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return groupBackend.memberOfAny(this, ids);
}
@Override
public Set<Change.Id> getStarredChanges() {
if (starredChanges == null) {

6
gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
View File

@ -15,7 +15,6 @@
package com.google.gerrit.server;
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
@ -52,11 +51,6 @@ public class InternalUser extends CurrentUser {
return GroupMembership.EMPTY;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return false;
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

6
gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java
View File

@ -14,7 +14,6 @@
package com.google.gerrit.server;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
@ -50,11 +49,6 @@ public class PeerDaemonUser extends CurrentUser {
return GroupMembership.EMPTY;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return false;
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

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

@ -15,15 +15,8 @@
package com.google.gerrit.server.account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
public abstract class AbstractGroupBackend implements GroupBackend {
@Override
public boolean memberOfAny(IdentifiedUser user, Iterable<AccountGroup.UUID> ids) {
return membershipsOf(user).containsAnyOf(ids);
}
@Override
public boolean isVisibleToAll(AccountGroup.UUID uuid) {
return false;

15
gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
View File

@ -183,9 +183,10 @@ public class CapabilityControl {
// the 'CI Servers' actually use the BATCH queue while everyone else gets
// to use the INTERACTIVE queue without additional grants.
//
GroupMembership groups = user.getEffectiveGroups();
boolean batch = false;
for (PermissionRule r : capabilities.priority) {
if (user.memberOf(r.getGroup().getUUID())) {
if (match(groups, r)) {
switch (r.getAction()) {
case INTERACTIVE:
if (!SystemGroupBackend.isAnonymousOrRegistered(r.getGroup())) {
@ -264,8 +265,9 @@ public class CapabilityControl {
return rules;
}
GroupMembership groups = user.getEffectiveGroups();
if (rules.size() == 1) {
if (!user.memberOf(rules.get(0).getGroup().getUUID())) {
if (!match(groups, rules.get(0))) {
rules = Collections.emptyList();
}
effective.put(permissionName, rules);
@ -274,7 +276,7 @@ public class CapabilityControl {
List<PermissionRule> mine = new ArrayList<>(rules.size());
for (PermissionRule rule : rules) {
if (user.memberOf(rule.getGroup().getUUID())) {
if (match(groups, rule)) {
mine.add(rule);
}
}
@ -302,6 +304,11 @@ public class CapabilityControl {
return rule.getGroup().getUUID();
}
});
return user.memberOfAny(ids);
return user.getEffectiveGroups().containsAnyOf(ids);
}
private static boolean match(GroupMembership groups,
PermissionRule rule) {
return groups.contains(rule.getGroup().getUUID());
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupBackend.java
View File

@ -51,14 +51,6 @@ public interface GroupBackend {
/** @return the group membership checker for the backend. */
GroupMembership membershipsOf(IdentifiedUser user);
/**
* Check if the user is member of any of the given groups.
*
* @return {@code true} if the user is a member of at least one of the
* given groups, {@code false} otherwise
*/
boolean memberOfAny(IdentifiedUser user, Iterable<AccountGroup.UUID> ids);
/**
* @return {@code true} if the group with the given UUID is visible to all
* registered users.

6
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
View File

@ -134,9 +134,9 @@ public class GroupControl {
*/
return user instanceof InternalUser
|| groupBackend.isVisibleToAll(group.getGroupUUID())
|| user.getEffectiveGroups().contains(group.getGroupUUID())
|| user.getCapabilities().canAdministrateServer()
|| isOwner()
|| user.memberOf(group.getGroupUUID());
|| isOwner();
}
public boolean isOwner() {
@ -145,7 +145,7 @@ public class GroupControl {
isOwner = false;
} else if (isOwner == null) {
AccountGroup.UUID ownerUUID = accountGroup.getOwnerGroupUUID();
isOwner = getCurrentUser().memberOf(ownerUUID)
isOwner = getCurrentUser().getEffectiveGroups().contains(ownerUUID)
|| getCurrentUser().getCapabilities().canAdministrateServer();
}
return isOwner;

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

@ -112,7 +112,7 @@ public class IncludingGroupMembership implements GroupMembership {
}
private boolean search(Set<AccountGroup.UUID> ids) {
return user.memberOfAny(ids);
return user.getEffectiveGroups().containsAnyOf(ids);
}
private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {

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

@ -31,7 +31,7 @@ import java.util.Collection;
/** Implementation of GroupBackend for the internal group system. */
@Singleton
public class InternalGroupBackend extends AbstractGroupBackend {
public class InternalGroupBackend implements GroupBackend {
private static final Function<AccountGroup, GroupReference> ACT_GROUP_TO_GROUP_REF =
new Function<AccountGroup, GroupReference>() {
@Override

23
gerrit-server/src/main/java/com/google/gerrit/server/account/UniversalGroupBackend.java
View File

@ -19,7 +19,6 @@ import static com.google.gerrit.server.account.GroupBackends.GROUP_REF_NAME_COMP
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
@ -27,7 +26,6 @@ import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.project.ProjectControl;
import com.google.inject.Inject;
@ -45,7 +43,7 @@ import java.util.Set;
* set of GroupBackends.
*/
@Singleton
public class UniversalGroupBackend extends AbstractGroupBackend {
public class UniversalGroupBackend implements GroupBackend {
private static final Logger log =
LoggerFactory.getLogger(UniversalGroupBackend.class);
@ -97,25 +95,6 @@ public class UniversalGroupBackend extends AbstractGroupBackend {
return new UniversalGroupMembership(user);
}
@Override
public boolean memberOfAny(IdentifiedUser user, Iterable<AccountGroup.UUID> ids) {
Multimap<GroupBackend, AccountGroup.UUID> groups = LinkedListMultimap.create();
for (AccountGroup.UUID uuid : ids) {
for (GroupBackend g : backends) {
if (g.handles(uuid)) {
groups.put(g, uuid);
}
}
}
for (Map.Entry<GroupBackend, Collection<UUID>> e : groups.asMap().entrySet()) {
if (e.getKey().memberOfAny(user, e.getValue())) {
return true;
}
}
return false;
}
private class UniversalGroupMembership implements GroupMembership {
private final Map<GroupBackend, GroupMembership> memberships;

4
gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java
View File

@ -29,7 +29,7 @@ import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AbstractGroupBackend;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.auth.ldap.Helper.LdapSchema;
@ -59,7 +59,7 @@ import javax.security.auth.login.LoginException;
/**
* Implementation of GroupBackend for the LDAP group system.
*/
public class LdapGroupBackend extends AbstractGroupBackend {
public class LdapGroupBackend implements GroupBackend {
private static final Logger log = LoggerFactory.getLogger(LdapGroupBackend.class);
private static final String LDAP_NAME = "ldap/";

10
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@ -34,6 +34,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GitReceivePackGroups;
@ -298,7 +299,8 @@ public class ProjectControl {
private boolean isDeclaredOwner() {
if (declaredOwner == null) {
declaredOwner = user.memberOfAny(state.getAllOwners());
GroupMembership effectiveGroups = user.getEffectiveGroups();
declaredOwner = effectiveGroups.containsAnyOf(state.getAllOwners());
}
return declaredOwner;
}
@ -372,11 +374,11 @@ public class ProjectControl {
}
}
if (iUser.memberOfAny(okGroupIds)) {
if (iUser.getEffectiveGroups().containsAnyOf(okGroupIds)) {
return Capable.OK;
}
if (iUser.memberOfAny(missingInfoGroupIds)) {
if (iUser.getEffectiveGroups().containsAnyOf(missingInfoGroupIds)) {
final StringBuilder msg = new StringBuilder();
for (ContributorAgreement ca : contributorAgreements) {
if (ca.isRequireContactInformation()) {
@ -510,7 +512,7 @@ public class ProjectControl {
} else if (SystemGroupBackend.CHANGE_OWNER.equals(uuid)) {
return isChangeOwner;
} else {
return user.memberOf(uuid);
return user.getEffectiveGroups().contains(uuid);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
View File

@ -131,7 +131,7 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
return false;
}
if (group != null && !reviewer.memberOf(group)) {
if (group != null && !reviewer.getEffectiveGroups().contains(group)) {
return false;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OwnerinPredicate.java
View File

@ -47,7 +47,7 @@ class OwnerinPredicate extends OperatorPredicate<ChangeData> {
}
final IdentifiedUser owner = userFactory.create(dbProvider,
change.getOwner());
return owner.memberOf(uuid);
return owner.getEffectiveGroups().contains(uuid);
}
@Override

6
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@ -26,7 +26,6 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
@ -357,11 +356,6 @@ public class Util {
return groups;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return getEffectiveGroups().containsAnyOf(ids);
}
@Override
public String getUserName() {
return username;