Perform group lookups by name from the group index
Now that we have the group index, we can use it to look up groups by
name. This change is a prerequisite for migrating groups to NoteDb.
A lot of tests didn't check for null for values retrieved from the group
cache. This behavior isn't changed as improving it is beyond the scope
of this change.
Change-Id: Id0ddc3bd43aa6e6b06833c712a51e2fb80402abe
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 4943b7e..1af8f7d 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1067,7 +1067,8 @@
protected void grant(Project.NameKey project, String ref, String permission, boolean force)
throws RepositoryNotFoundException, IOException, ConfigInvalidException {
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
grant(project, ref, permission, force, adminGroup.getGroupUUID());
}
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
index faa674e..a8f7767 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
@@ -24,11 +24,12 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
-import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.GroupsUpdate;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ServerInitiated;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gwtorm.server.SchemaFactory;
@@ -54,7 +55,7 @@
private final Sequences sequences;
private final AccountsUpdate.Server accountsUpdate;
private final VersionedAuthorizedKeys.Accessor authorizedKeys;
- private final Groups groups;
+ private final GroupCache groupCache;
private final Provider<GroupsUpdate> groupsUpdateProvider;
private final SshKeyCache sshKeyCache;
private final ExternalIdsUpdate.Server externalIdsUpdate;
@@ -66,7 +67,7 @@
Sequences sequences,
AccountsUpdate.Server accountsUpdate,
VersionedAuthorizedKeys.Accessor authorizedKeys,
- Groups groups,
+ GroupCache groupCache,
@ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider,
SshKeyCache sshKeyCache,
ExternalIdsUpdate.Server externalIdsUpdate,
@@ -76,7 +77,7 @@
this.sequences = sequences;
this.accountsUpdate = accountsUpdate;
this.authorizedKeys = authorizedKeys;
- this.groups = groups;
+ this.groupCache = groupCache;
this.groupsUpdateProvider = groupsUpdateProvider;
this.sshKeyCache = sshKeyCache;
this.externalIdsUpdate = externalIdsUpdate;
@@ -121,7 +122,7 @@
if (groupNames != null) {
for (String n : groupNames) {
AccountGroup.NameKey k = new AccountGroup.NameKey(n);
- Optional<AccountGroup> group = groups.getGroup(db, k);
+ Optional<InternalGroup> group = groupCache.get(k);
if (!group.isPresent()) {
throw new NoSuchGroupException(n);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 8ac063b..84f3533 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -91,6 +91,7 @@
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
import com.google.gerrit.server.project.RefPattern;
@@ -1014,7 +1015,8 @@
String userRef = RefNames.refsUsers(foo.id);
accountIndexedCounter.clear();
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID());
grantLabel("Code-Review", -2, 2, allUsers, userRef, false, adminGroup.getGroupUUID(), false);
grant(allUsers, userRef, Permission.SUBMIT, false, adminGroup.getGroupUUID());
@@ -1179,7 +1181,8 @@
accountsUpdate.create().update(foo.id, a -> a.setPreferredEmail(noEmail));
accountIndexedCounter.clear();
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID());
TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
@@ -1214,7 +1217,8 @@
String userRef = RefNames.refsUsers(foo.id);
accountIndexedCounter.clear();
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID());
TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
@@ -1273,7 +1277,8 @@
String userRef = RefNames.refsUsers(foo.id);
accountIndexedCounter.clear();
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID());
TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d399e2b..a8a712c 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1225,7 +1225,7 @@
Util.allow(
cfg,
Permission.READ,
- groupCache.get(new AccountGroup.NameKey("Administrators")).getGroupUUID(),
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID(),
"refs/*");
Util.block(cfg, Permission.READ, REGISTERED_USERS, "refs/*");
saveProjectConfig(p, cfg);
@@ -1302,7 +1302,7 @@
Util.allow(
cfg,
Permission.READ,
- groupCache.get(new AccountGroup.NameKey("Administrators")).getGroupUUID(),
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID(),
"refs/*");
Util.block(cfg, Permission.READ, REGISTERED_USERS, "refs/*");
saveProjectConfig(p, cfg);
@@ -1349,7 +1349,7 @@
Util.allow(
cfg,
Permission.READ,
- groupCache.get(new AccountGroup.NameKey("Administrators")).getGroupUUID(),
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID(),
"refs/*");
Util.block(cfg, Permission.READ, REGISTERED_USERS, "refs/*");
saveProjectConfig(p, cfg);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupAssert.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupAssert.java
index ccdd03b..dd891ce 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupAssert.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupAssert.java
@@ -19,7 +19,7 @@
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.restapi.Url;
-import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.group.InternalGroup;
import java.util.Set;
public class GroupAssert {
@@ -31,7 +31,7 @@
assertWithMessage("unexpected groups: " + actual).that(actual).isEmpty();
}
- public static void assertGroupInfo(AccountGroup group, GroupInfo info) {
+ public static void assertGroupInfo(InternalGroup group, GroupInfo info) {
if (info.name != null) {
// 'name' is not set if returned in a map
assertThat(info.name).isEqualTo(group.getName());
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 1b5e544a..eb4df15 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -47,6 +47,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.group.GroupsUpdate;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ServerInitiated;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Inject;
@@ -257,13 +258,13 @@
@Test
public void getGroup() throws Exception {
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup = getFromCache("Administrators");
testGetGroup(adminGroup.getGroupUUID().get(), adminGroup);
testGetGroup(adminGroup.getName(), adminGroup);
testGetGroup(adminGroup.getId().get(), adminGroup);
}
- private void testGetGroup(Object id, AccountGroup expectedGroup) throws Exception {
+ private void testGetGroup(Object id, InternalGroup expectedGroup) throws Exception {
GroupInfo group = gApi.groups().id(id.toString()).get();
assertGroupInfo(expectedGroup, group);
}
@@ -559,7 +560,7 @@
@Test
public void allGroupInfoFieldsSetCorrectly() throws Exception {
- AccountGroup adminGroup = getFromCache("Administrators");
+ InternalGroup adminGroup = getFromCache("Administrators");
Map<String, GroupInfo> groups = gApi.groups().list().addGroup(adminGroup.getName()).getAsMap();
assertThat(groups).hasSize(1);
assertThat(groups).containsKey("Administrators");
@@ -683,8 +684,8 @@
assertThat(gApi.groups().id(group).includedGroups()).isEmpty();
}
- private AccountGroup getFromCache(String name) throws Exception {
- return groupCache.get(new AccountGroup.NameKey(name));
+ private InternalGroup getFromCache(String name) throws Exception {
+ return groupCache.get(new AccountGroup.NameKey(name)).orElse(null);
}
private void setCreatedOnToNull(AccountGroup.UUID groupUuid) throws Exception {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/CheckAccessIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
index b471efc..2f92e7a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import java.util.List;
import org.junit.Before;
@@ -39,14 +40,15 @@
private Project.NameKey secretProject;
private Project.NameKey secretRefProject;
private TestAccount privilegedUser;
- private AccountGroup privilegedGroup;
+ private InternalGroup privilegedGroup;
@Before
public void setUp() throws Exception {
normalProject = createProject("normal");
secretProject = createProject("secret");
secretRefProject = createProject("secretRef");
- privilegedGroup = groupCache.get(new AccountGroup.NameKey(createGroup("privilegedGroup")));
+ privilegedGroup =
+ groupCache.get(new AccountGroup.NameKey(createGroup("privilegedGroup"))).orElse(null);
privilegedUser = accountCreator.create("privilegedUser", "snowden@nsa.gov", "Ed Snowden");
gApi.groups().id(privilegedGroup.getGroupUUID().get()).addMembers(privilegedUser.username);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 84c2901..4dac61f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -82,7 +82,7 @@
@Before
public void setUp() throws Exception {
- admins = groupCache.get(new AccountGroup.NameKey("Administrators")).getGroupUUID();
+ admins = groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID();
setUpPermissions();
setUpChanges();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/FlushCacheIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/FlushCacheIT.java
index 41f7d4a..b586ab2 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/FlushCacheIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/FlushCacheIT.java
@@ -23,13 +23,14 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.config.ListCaches.CacheInfo;
+import com.google.gerrit.server.group.InternalGroup;
import org.junit.Test;
public class FlushCacheIT extends AbstractDaemonTest {
@Test
public void flushCache() throws Exception {
- AccountGroup group = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup group = groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
assertWithMessage("Precondition: The group 'Administrators' was loaded by the group cache")
.that(group)
.isNotNull();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
index b6ac5e9..ccbb71c 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -41,6 +41,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllProjectsNameProvider;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import java.util.HashMap;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -394,7 +395,8 @@
@Test
public void addNonGlobalCapabilityToGlobalCapabilities() throws Exception {
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
ProjectAccessInput accessInput = newProjectAccessInput();
AccessSectionInfo accessSectionInfo = newAccessSectionInfo();
@@ -423,7 +425,8 @@
@Test
public void removeGlobalCapabilityAsAdmin() throws Exception {
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
+ InternalGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null);
ProjectAccessInput accessInput = newProjectAccessInput();
AccessSectionInfo accessSectionInfo = newAccessSectionInfo();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
index 7640328..0409fbc 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
@@ -191,7 +191,11 @@
in.owners.add(SystemGroupBackend.REGISTERED_USERS.get()); // by UUID
in.owners.add(
Integer.toString(
- groupCache.get(new AccountGroup.NameKey("Administrators")).getId().get())); // by ID
+ groupCache
+ .get(new AccountGroup.NameKey("Administrators"))
+ .orElse(null)
+ .getId()
+ .get())); // by ID
gApi.projects().create(in);
ProjectState projectState = projectCache.get(new Project.NameKey(newProjectName));
Set<AccountGroup.UUID> expectedOwnerIds = Sets.newHashSetWithExpectedSize(3);
@@ -293,7 +297,7 @@
}
private AccountGroup.UUID groupUuid(String groupName) {
- return groupCache.get(new AccountGroup.NameKey(groupName)).getGroupUUID();
+ return groupCache.get(new AccountGroup.NameKey(groupName)).orElse(null).getGroupUUID();
}
private void assertHead(String projectName, String expectedRef) throws Exception {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
index 82f1559..122c0bc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
@@ -24,7 +24,14 @@
public interface GroupCache {
AccountGroup get(AccountGroup.Id groupId);
- AccountGroup get(AccountGroup.NameKey name);
+ /**
+ * Looks up an internal group by its name.
+ *
+ * @param name the name of the internal group
+ * @return an {@code Optional} of the internal group, or an empty {@code Optional} if no internal
+ * group with this name exists on this server or an error occurred during lookup
+ */
+ Optional<InternalGroup> get(AccountGroup.NameKey name);
/**
* Looks up an internal group by its UUID.
@@ -39,11 +46,10 @@
ImmutableList<AccountGroup> all();
/** Notify the cache that a new group was constructed. */
- void onCreateGroup(AccountGroup.NameKey newGroupName) throws IOException;
+ void onCreateGroup(AccountGroup group) throws IOException;
void evict(AccountGroup.UUID groupUuid, AccountGroup.Id groupId, AccountGroup.NameKey groupName)
throws IOException;
- void evictAfterRename(AccountGroup.NameKey oldName, AccountGroup.NameKey newName)
- throws IOException;
+ void evictAfterRename(AccountGroup.NameKey oldName) throws IOException;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
index 2901501..c95517b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.group.GroupIndexer;
+import com.google.gerrit.server.query.group.InternalGroupQuery;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
@@ -59,7 +60,7 @@
cache(BYID_NAME, AccountGroup.Id.class, new TypeLiteral<Optional<AccountGroup>>() {})
.loader(ByIdLoader.class);
- cache(BYNAME_NAME, String.class, new TypeLiteral<Optional<AccountGroup>>() {})
+ cache(BYNAME_NAME, String.class, new TypeLiteral<Optional<InternalGroup>>() {})
.loader(ByNameLoader.class);
cache(BYUUID_NAME, String.class, new TypeLiteral<Optional<InternalGroup>>() {})
@@ -72,7 +73,7 @@
}
private final LoadingCache<AccountGroup.Id, Optional<AccountGroup>> byId;
- private final LoadingCache<String, Optional<AccountGroup>> byName;
+ private final LoadingCache<String, Optional<InternalGroup>> byName;
private final LoadingCache<String, Optional<InternalGroup>> byUUID;
private final SchemaFactory<ReviewDb> schema;
private final Provider<GroupIndexer> indexer;
@@ -81,7 +82,7 @@
@Inject
GroupCacheImpl(
@Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<AccountGroup>> byId,
- @Named(BYNAME_NAME) LoadingCache<String, Optional<AccountGroup>> byName,
+ @Named(BYNAME_NAME) LoadingCache<String, Optional<InternalGroup>> byName,
@Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID,
SchemaFactory<ReviewDb> schema,
Provider<GroupIndexer> indexer,
@@ -122,27 +123,22 @@
}
@Override
- public void evictAfterRename(final AccountGroup.NameKey oldName, AccountGroup.NameKey newName)
- throws IOException {
+ public void evictAfterRename(AccountGroup.NameKey oldName) throws IOException {
if (oldName != null) {
byName.invalidate(oldName.get());
}
- if (newName != null) {
- byName.invalidate(newName.get());
- }
- indexer.get().index(get(newName).getGroupUUID());
}
@Override
- public AccountGroup get(AccountGroup.NameKey name) {
+ public Optional<InternalGroup> get(AccountGroup.NameKey name) {
if (name == null) {
- return null;
+ return Optional.empty();
}
try {
- return byName.get(name.get()).orElse(null);
+ return byName.get(name.get());
} catch (ExecutionException e) {
log.warn(String.format("Cannot look up group %s by name", name.get()), e);
- return null;
+ return Optional.empty();
}
}
@@ -171,9 +167,8 @@
}
@Override
- public void onCreateGroup(AccountGroup.NameKey newGroupName) throws IOException {
- byName.invalidate(newGroupName.get());
- indexer.get().index(get(newGroupName).getGroupUUID());
+ public void onCreateGroup(AccountGroup group) throws IOException {
+ indexer.get().index(group.getGroupUUID());
}
private static AccountGroup missing(AccountGroup.Id key) {
@@ -199,21 +194,17 @@
}
}
- static class ByNameLoader extends CacheLoader<String, Optional<AccountGroup>> {
- private final SchemaFactory<ReviewDb> schema;
- private final Groups groups;
+ static class ByNameLoader extends CacheLoader<String, Optional<InternalGroup>> {
+ private final Provider<InternalGroupQuery> groupQueryProvider;
@Inject
- ByNameLoader(SchemaFactory<ReviewDb> sf, Groups groups) {
- schema = sf;
- this.groups = groups;
+ ByNameLoader(Provider<InternalGroupQuery> groupQueryProvider) {
+ this.groupQueryProvider = groupQueryProvider;
}
@Override
- public Optional<AccountGroup> load(String name) throws Exception {
- try (ReviewDb db = schema.open()) {
- return groups.getGroup(db, new AccountGroup.NameKey(name));
- }
+ public Optional<InternalGroup> load(String name) throws Exception {
+ return groupQueryProvider.get().byName(new AccountGroup.NameKey(name));
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupIdHandler.java b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupIdHandler.java
index d41f02c..f3393c1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupIdHandler.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupIdHandler.java
@@ -16,8 +16,10 @@
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Optional;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.OptionDef;
@@ -41,11 +43,11 @@
@Override
public final int parseArguments(Parameters params) throws CmdLineException {
final String n = params.getParameter(0);
- final AccountGroup group = groupCache.get(new AccountGroup.NameKey(n));
- if (group == null) {
+ Optional<InternalGroup> group = groupCache.get(new AccountGroup.NameKey(n));
+ if (!group.isPresent()) {
throw new CmdLineException(owner, "Group \"" + n + "\" does not exist");
}
- setter.addValue(group.getId());
+ setter.addValue(group.get().getId());
return 1;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
index b835d22..a1947ec 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
@@ -21,7 +21,6 @@
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupById;
import com.google.gerrit.reviewdb.client.AccountGroupMember;
-import com.google.gerrit.reviewdb.client.AccountGroupName;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.gwtorm.server.OrmException;
@@ -93,25 +92,6 @@
}
}
- /**
- * Returns the {@code AccountGroup} for the specified name if it exists.
- *
- * @param db the {@code ReviewDb} instance to use for lookups
- * @param groupName the name of the group
- * @return the found {@code AccountGroup} if it exists, or else an empty {@code Optional}
- * @throws OrmException if the group couldn't be retrieved from ReviewDb
- */
- public Optional<AccountGroup> getGroup(ReviewDb db, AccountGroup.NameKey groupName)
- throws OrmException {
- AccountGroupName accountGroupName = db.accountGroupNames().get(groupName);
- if (accountGroupName == null) {
- return Optional.empty();
- }
-
- AccountGroup.Id groupId = accountGroupName.getId();
- return Optional.ofNullable(db.accountGroups().get(groupId));
- }
-
public Stream<AccountGroup> getAll(ReviewDb db) throws OrmException {
return Streams.stream(db.accountGroups().all());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
index d688e4c..b432af7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
@@ -135,7 +135,7 @@
throws OrmException, IOException {
addNewGroup(db, group);
addNewGroupMembers(db, group, memberIds);
- groupCache.onCreateGroup(group.getNameKey());
+ groupCache.onCreateGroup(group);
}
/**
@@ -221,8 +221,8 @@
db.accountGroupNames().deleteKeys(ImmutableList.of(oldName));
+ groupCache.evictAfterRename(oldName);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
- groupCache.evictAfterRename(oldName, newName);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/InternalGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/InternalGroup.java
index 228d86f..fafc591 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/InternalGroup.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/InternalGroup.java
@@ -19,10 +19,12 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.io.Serializable;
import java.sql.Timestamp;
@AutoValue
-public abstract class InternalGroup {
+public abstract class InternalGroup implements Serializable {
+ private static final long serialVersionUID = 1L;
public static InternalGroup create(
AccountGroup accountGroup,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
new file mode 100644
index 0000000..b261e25
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
@@ -0,0 +1,63 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.group;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.query.InternalQuery;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.index.group.GroupIndexCollection;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import java.util.List;
+import java.util.Optional;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Query wrapper for the group index.
+ *
+ * <p>Instances are one-time-use. Other singleton classes should inject a Provider rather than
+ * holding on to a single instance.
+ */
+public class InternalGroupQuery extends InternalQuery<InternalGroup> {
+ private static final Logger log = LoggerFactory.getLogger(InternalGroupQuery.class);
+
+ @Inject
+ InternalGroupQuery(
+ GroupQueryProcessor queryProcessor, GroupIndexCollection indexes, IndexConfig indexConfig) {
+ super(queryProcessor, indexes, indexConfig);
+ }
+
+ public Optional<InternalGroup> byName(AccountGroup.NameKey groupName) throws OrmException {
+ List<InternalGroup> groups = query(GroupPredicates.name(groupName.get()));
+ if (groups.isEmpty()) {
+ return Optional.empty();
+ }
+
+ if (groups.size() == 1) {
+ return Optional.of(Iterables.getOnlyElement(groups));
+ }
+
+ ImmutableList<AccountGroup.UUID> groupUuids =
+ groups.stream().map(InternalGroup::getGroupUUID).collect(toImmutableList());
+ log.warn(String.format("Ambiguous group name '%s' for groups %s.", groupName, groupUuids));
+ return Optional.empty();
+ }
+}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/CommitsCollectionTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/CommitsCollectionTest.java
index 0d8080f..7f1b233 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/CommitsCollectionTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/CommitsCollectionTest.java
@@ -90,7 +90,7 @@
// registered user.
// See AccountManager#create().
accountManager.authenticate(AuthRequest.forUser("admin")).getAccountId();
- admins = groupCache.get(new AccountGroup.NameKey("Administrators")).getGroupUUID();
+ admins = groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID();
setUpPermissions();
Account.Id userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListMembersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListMembersCommand.java
index 1c903c7..ffaf923 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListMembersCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListMembersCommand.java
@@ -23,6 +23,7 @@
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ListMembers;
import com.google.gerrit.server.ioutil.ColumnFormatter;
import com.google.gerrit.sshd.CommandMetaData;
@@ -31,6 +32,7 @@
import com.google.inject.Inject;
import java.io.PrintWriter;
import java.util.List;
+import java.util.Optional;
import org.kohsuke.args4j.Argument;
/** Implements a command that allows the user to see the members of a group. */
@@ -68,16 +70,16 @@
}
void display(PrintWriter writer) throws OrmException {
- AccountGroup group = groupCache.get(new AccountGroup.NameKey(name));
+ Optional<InternalGroup> group = groupCache.get(new AccountGroup.NameKey(name));
String errorText = "Group not found or not visible\n";
- if (group == null) {
+ if (!group.isPresent()) {
writer.write(errorText);
writer.flush();
return;
}
- List<AccountInfo> members = apply(group.getGroupUUID());
+ List<AccountInfo> members = apply(group.get().getGroupUUID());
ColumnFormatter formatter = new ColumnFormatter(writer, '\t');
formatter.addColumn("id");
formatter.addColumn("username");
