opendev/gerrit
Code Issues Proposed changes

Revert ConfigUtil groupsFor() to return a Set.

Browse Source 
There is no reason to return a GroupMembership from
ConfigUtil.groupsFor().  GroupSetProvider needs an explicit Set,
and uses getKnownGroups(), which eventually will be removed.

Also, I updated the implementation of groupsFor() to use the batch
get api on the Access.

Change-Id: I98811c1db3acda9aaec7c0ababdc5828b874145f
This commit is contained in:
Colby Ranger
2012-04-04 14:22:21 -07:00
parent 9906ec1e8a
commit ca1532d6ca
3 changed files with 41 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigUtil.java
View File

@@ -16,12 +16,14 @@ package com.google.gerrit.server.config;
import static org.eclipse.jgit.util.StringUtils.equalsIgnoreCase;
import com.google.common.base.Function;
import com.google.common.base.Predicates;
import com.google.common.collect.Iterables;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupName;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.gwtorm.server.SchemaFactory;
import org.eclipse.jgit.lib.Config;
@@ -30,7 +32,9 @@ import org.slf4j.Logger;
import java.lang.reflect.InvocationTargetException;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
@@ -312,36 +316,52 @@ public class ConfigUtil {
* @return the actual groups resolved from the database. If no groups are
* found, returns an empty {@code Set}, never {@code null}.
*/
public static GroupMembership groupsFor(
public static Set<AccountGroup.UUID> groupsFor(
SchemaFactory<ReviewDb> dbfactory, String[] groupNames, Logger log,
String groupNotFoundWarning) {
final Set<AccountGroup.UUID> result = new HashSet<AccountGroup.UUID>();
try {
final ReviewDb db = dbfactory.open();
try {
for (String name : groupNames) {
AccountGroupName group =
db.accountGroupNames().get(new AccountGroup.NameKey(name));
if (group == null) {
log.warn(MessageFormat.format(groupNotFoundWarning, name));
List<AccountGroupName> groups = db.accountGroupNames().get(
Iterables.transform(Arrays.asList(groupNames),
new Function<String, AccountGroup.NameKey>() {
@Override
public AccountGroup.NameKey apply(String name) {
return new AccountGroup.NameKey(name);
}
})).toList();
Iterator<AccountGroup> ags = db.accountGroups().get(
Iterables.transform(Iterables.filter(groups, Predicates.notNull()),
new Function<AccountGroupName, AccountGroup.Id>() {
@Override
public AccountGroup.Id apply(AccountGroupName group) {
return group.getId();
}
})).iterator();
for (int i = 0; i < groupNames.length; i++) {
if (groups.get(i) == null) {
log.warn(MessageFormat.format(groupNotFoundWarning, groupNames[i]));
continue;
}
AccountGroup ag = db.accountGroups().get(group.getId());
AccountGroup ag = ags.next();
if (ag == null) {
log.warn(MessageFormat.format(groupNotFoundWarning, name));
continue;
log.warn(MessageFormat.format(groupNotFoundWarning, groupNames[i]));
} else {
result.add(ag.getGroupUUID());
}
result.add(ag.getGroupUUID());
}
} finally {
db.close();
}
} catch (OrmRuntimeException e) {
log.error("Database error, cannot load groups", e);
} catch (OrmException e) {
log.error("Database error, cannot load groups", e);
}
return new ListGroupMembership(result);
return result;
}
/**
@@ -354,7 +374,7 @@ public class ConfigUtil {
* @return the actual groups resolved from the database. If no groups are
* found, returns an empty {@code Set}, never {@code null}.
*/
public static GroupMembership groupsFor(
public static Set<AccountGroup.UUID> groupsFor(
SchemaFactory<ReviewDb> dbfactory, String[] groupNames, Logger log) {
return groupsFor(dbfactory, groupNames, log,
"Group \"{0}\" not in database, skipping.");

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GroupSetProvider.java
View File

@@ -40,7 +40,7 @@ public abstract class GroupSetProvider implements
protected GroupSetProvider(@GerritServerConfig Config config,
SchemaFactory<ReviewDb> db, String section, String subsection, String name) {
String[] groupNames = config.getStringList(section, subsection, name);
groupIds = unmodifiableSet(groupsFor(db, groupNames, log).getKnownGroups());
groupIds = unmodifiableSet(groupsFor(db, groupNames, log));
}
@Override

9
gerrit-server/src/main/java/com/google/gerrit/server/git/PushReplication.java
View File

@@ -14,12 +14,12 @@
package com.google.gerrit.server.git;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.ReplicationUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.FactoryModule;
import com.google.gerrit.server.config.SitePaths;
@@ -67,7 +67,6 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
/** Manages automatic replication to remote repositories. */
@@ -354,7 +353,7 @@ public class PushReplication implements ReplicationQueue {
}
@Override
public synchronized void write(final int b) throws IOException {
public synchronized void write(final int b) {
if (b == '\r') {
return;
}
@@ -407,8 +406,8 @@ public class PushReplication implements ReplicationQueue {
cfg.getStringList("remote", rc.getName(), "authGroup");
final GroupMembership authGroups;
if (authGroupNames.length > 0) {
authGroups = ConfigUtil.groupsFor(db, authGroupNames, //
log, "Group \"{0}\" not in database, removing from authGroup");
authGroups = new ListGroupMembership(ConfigUtil.groupsFor(db, authGroupNames, //
log, "Group \"{0}\" not in database, removing from authGroup"));
} else {
authGroups = ReplicationUser.EVERYTHING_VISIBLE;
}