Remove ConfigUtil.groupsFor() methods.

This method did not embrace guice and directly queried the database,
where GroupCache should have been used. Updated the calling code
accordingly.

Change-Id: I094c9ec8aa4fb2d23e61f9a717203c33ff785776
This commit is contained in:
Colby Ranger
2012-05-02 10:33:39 -07:00
parent f93b3eecec
commit 55a39e9b98
6 changed files with 46 additions and 124 deletions

View File

@@ -16,27 +16,10 @@ package com.google.gerrit.server.config;
import static org.eclipse.jgit.util.StringUtils.equalsIgnoreCase; 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.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.gwtorm.server.SchemaFactory;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
import org.slf4j.Logger;
import java.lang.reflect.InvocationTargetException; import java.lang.reflect.InvocationTargetException;
import java.text.MessageFormat;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
import java.util.regex.Matcher; import java.util.regex.Matcher;
import java.util.regex.Pattern; import java.util.regex.Pattern;
@@ -303,83 +286,6 @@ public class ConfigUtil {
} }
} }
/**
* Resolve groups from group names, via the database. Group names not found in
* the database will be skipped.
*
* @param dbfactory database to resolve from.
* @param groupNames group names to resolve.
* @param log log for any warnings and errors.
* @param groupNotFoundWarning formatted message to output to the log for each
* group name which is not found in the database. <code>{0}</code> will
* be replaced with the group name.
* @return the actual groups resolved from the database. If no groups are
* found, returns an empty {@code Set}, never {@code null}.
*/
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 {
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 = ags.next();
if (ag == null) {
log.warn(MessageFormat.format(groupNotFoundWarning, groupNames[i]));
} else {
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 result;
}
/**
* Resolve groups from group names, via the database. Group names not found in
* the database will be skipped.
*
* @param dbfactory database to resolve from.
* @param groupNames group names to resolve.
* @param log log for any warnings and errors.
* @return the actual groups resolved from the database. If no groups are
* found, returns an empty {@code Set}, never {@code null}.
*/
public static Set<AccountGroup.UUID> groupsFor(
SchemaFactory<ReviewDb> dbfactory, String[] groupNames, Logger log) {
return groupsFor(dbfactory, groupNames, log,
"Group \"{0}\" not in database, skipping.");
}
private static boolean match(final String a, final String... cases) { private static boolean match(final String a, final String... cases) {
for (final String b : cases) { for (final String b : cases) {
if (equalsIgnoreCase(a, b)) { if (equalsIgnoreCase(a, b)) {

View File

@@ -15,8 +15,7 @@
package com.google.gerrit.server.config; package com.google.gerrit.server.config;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupCache;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject; import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
@@ -25,9 +24,9 @@ import java.util.Collections;
public class GitReceivePackGroupsProvider extends GroupSetProvider { public class GitReceivePackGroupsProvider extends GroupSetProvider {
@Inject @Inject
public GitReceivePackGroupsProvider(@GerritServerConfig Config config, public GitReceivePackGroupsProvider(GroupCache gc,
SchemaFactory<ReviewDb> db) { @GerritServerConfig Config config) {
super(config, db, "receive", null, "allowGroup"); super(gc, config, "receive", null, "allowGroup");
// If no group was set, default to "registered users" // If no group was set, default to "registered users"
// //

View File

@@ -15,8 +15,7 @@
package com.google.gerrit.server.config; package com.google.gerrit.server.config;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupCache;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject; import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
@@ -26,9 +25,9 @@ import java.util.HashSet;
public class GitUploadPackGroupsProvider extends GroupSetProvider { public class GitUploadPackGroupsProvider extends GroupSetProvider {
@Inject @Inject
public GitUploadPackGroupsProvider(@GerritServerConfig Config config, public GitUploadPackGroupsProvider(GroupCache gc,
SchemaFactory<ReviewDb> db) { @GerritServerConfig Config config) {
super(config, db, "upload", null, "allowGroup"); super(gc, config, "upload", null, "allowGroup");
// If no group was set, default to "registered users" and "anonymous" // If no group was set, default to "registered users" and "anonymous"
// //

View File

@@ -14,12 +14,9 @@
package com.google.gerrit.server.config; package com.google.gerrit.server.config;
import static com.google.gerrit.server.config.ConfigUtil.groupsFor; import com.google.common.collect.ImmutableSet;
import static java.util.Collections.unmodifiableSet;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupCache;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
@@ -37,10 +34,20 @@ public abstract class GroupSetProvider implements
protected Set<AccountGroup.UUID> groupIds; protected Set<AccountGroup.UUID> groupIds;
@Inject @Inject
protected GroupSetProvider(@GerritServerConfig Config config, protected GroupSetProvider(GroupCache groupCache,
SchemaFactory<ReviewDb> db, String section, String subsection, String name) { @GerritServerConfig Config config, String section,
String subsection, String name) {
String[] groupNames = config.getStringList(section, subsection, name); String[] groupNames = config.getStringList(section, subsection, name);
groupIds = unmodifiableSet(groupsFor(db, groupNames, log)); ImmutableSet.Builder<AccountGroup.UUID> builder = ImmutableSet.builder();
for (String n : groupNames) {
AccountGroup g = groupCache.get(new AccountGroup.NameKey(n));
if (g != null) {
builder.add(g.getGroupUUID());
} else {
log.warn("Group \"{0}\" not in database, skipping.", n);
}
}
groupIds = builder.build();
} }
@Override @Override

View File

@@ -14,8 +14,7 @@
package com.google.gerrit.server.config; package com.google.gerrit.server.config;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupCache;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject; import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
@@ -33,8 +32,8 @@ import org.eclipse.jgit.lib.Config;
*/ */
public class ProjectOwnerGroupsProvider extends GroupSetProvider { public class ProjectOwnerGroupsProvider extends GroupSetProvider {
@Inject @Inject
public ProjectOwnerGroupsProvider( public ProjectOwnerGroupsProvider(GroupCache gc,
@GerritServerConfig final Config config, final SchemaFactory<ReviewDb> db) { @GerritServerConfig final Config config) {
super(config, db, "repository", "*", "ownerGroup"); super(gc, config, "repository", "*", "ownerGroup");
} }
} }

View File

@@ -14,13 +14,15 @@
package com.google.gerrit.server.git; package com.google.gerrit.server.git;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.ReplicationUser; import com.google.gerrit.server.ReplicationUser;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership; 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.FactoryModule;
import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
@@ -99,11 +101,12 @@ public class PushReplication implements ReplicationQueue {
private final SchemaFactory<ReviewDb> database; private final SchemaFactory<ReviewDb> database;
private final ReplicationUser.Factory replicationUserFactory; private final ReplicationUser.Factory replicationUserFactory;
private final GitRepositoryManager gitRepositoryManager; private final GitRepositoryManager gitRepositoryManager;
private final GroupCache groupCache;
@Inject @Inject
PushReplication(final Injector i, final WorkQueue wq, final SitePaths site, PushReplication(final Injector i, final WorkQueue wq, final SitePaths site,
final ReplicationUser.Factory ruf, final SchemaFactory<ReviewDb> db, final ReplicationUser.Factory ruf, final SchemaFactory<ReviewDb> db,
final GitRepositoryManager grm) final GitRepositoryManager grm, GroupCache gc)
throws ConfigInvalidException, IOException { throws ConfigInvalidException, IOException {
injector = i; injector = i;
workQueue = wq; workQueue = wq;
@@ -111,6 +114,7 @@ public class PushReplication implements ReplicationQueue {
replicationUserFactory = ruf; replicationUserFactory = ruf;
gitRepositoryManager = grm; gitRepositoryManager = grm;
configs = allConfigs(site); configs = allConfigs(site);
groupCache = gc;
} }
@Override @Override
@@ -195,7 +199,6 @@ public class PushReplication implements ReplicationQueue {
} }
} }
if (c.getPushRefSpecs().isEmpty()) { if (c.getPushRefSpecs().isEmpty()) {
RefSpec spec = new RefSpec(); RefSpec spec = new RefSpec();
spec = spec.setSourceDestination("refs/*", "refs/*"); spec = spec.setSourceDestination("refs/*", "refs/*");
@@ -204,7 +207,7 @@ public class PushReplication implements ReplicationQueue {
} }
r.add(new ReplicationConfig(injector, workQueue, c, cfg, database, r.add(new ReplicationConfig(injector, workQueue, c, cfg, database,
replicationUserFactory, gitRepositoryManager)); replicationUserFactory, gitRepositoryManager, groupCache));
} }
return Collections.unmodifiableList(r); return Collections.unmodifiableList(r);
} }
@@ -392,7 +395,8 @@ public class PushReplication implements ReplicationQueue {
ReplicationConfig(final Injector injector, final WorkQueue workQueue, ReplicationConfig(final Injector injector, final WorkQueue workQueue,
final RemoteConfig rc, final Config cfg, SchemaFactory<ReviewDb> db, final RemoteConfig rc, final Config cfg, SchemaFactory<ReviewDb> db,
final ReplicationUser.Factory replicationUserFactory, final ReplicationUser.Factory replicationUserFactory,
final GitRepositoryManager gitRepositoryManager) { final GitRepositoryManager gitRepositoryManager,
GroupCache groupCache) {
remote = rc; remote = rc;
delay = Math.max(0, getInt(rc, cfg, "replicationdelay", 15)); delay = Math.max(0, getInt(rc, cfg, "replicationdelay", 15));
@@ -406,8 +410,16 @@ public class PushReplication implements ReplicationQueue {
cfg.getStringList("remote", rc.getName(), "authGroup"); cfg.getStringList("remote", rc.getName(), "authGroup");
final GroupMembership authGroups; final GroupMembership authGroups;
if (authGroupNames.length > 0) { if (authGroupNames.length > 0) {
authGroups = new ListGroupMembership(ConfigUtil.groupsFor(db, authGroupNames, // ImmutableSet.Builder<AccountGroup.UUID> builder = ImmutableSet.builder();
log, "Group \"{0}\" not in database, removing from authGroup")); for (String name : authGroupNames) {
AccountGroup g = groupCache.get(new AccountGroup.NameKey(name));
if (g != null) {
builder.add(g.getGroupUUID());
} else {
log.warn("Group \"{0}\" not in database, removing from authGroup", name);
}
}
authGroups = new ListGroupMembership(builder.build());
} else { } else {
authGroups = ReplicationUser.EVERYTHING_VISIBLE; authGroups = ReplicationUser.EVERYTHING_VISIBLE;
} }