opendev/gerrit
Code Issues Proposed changes

Perform group lookups by name from the group index

Browse Source 
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
This commit is contained in:
Alice Kober-Sotzek
2017-08-21 10:25:58 +02:00
parent c7be8f9f51
commit 1e1a63a6bd
20 changed files with 152 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -91,6 +91,7 @@ import com.google.gerrit.server.account.externalids.ExternalIds;
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 @@ public class AccountIT extends AbstractDaemonTest {
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 @@ public class AccountIT extends AbstractDaemonTest {
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 @@ public class AccountIT extends AbstractDaemonTest {
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 @@ public class AccountIT extends AbstractDaemonTest {
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);

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -1225,7 +1225,7 @@ public class ChangeIT extends AbstractDaemonTest {
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 @@ public class ChangeIT extends AbstractDaemonTest {
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 @@ public class ChangeIT extends AbstractDaemonTest {
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);

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupAssert.java
View File

@@ -19,7 +19,7 @@ import static com.google.common.truth.Truth.assertWithMessage;
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 @@ public class GroupAssert {
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());

11
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -47,6 +47,7 @@ import com.google.gerrit.extensions.restapi.Url;
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 @@ public class GroupsIT extends AbstractDaemonTest {
@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 @@ public class GroupsIT extends AbstractDaemonTest {
@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 @@ public class GroupsIT extends AbstractDaemonTest {
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 {

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
View File

@@ -28,6 +28,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
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 @@ public class CheckAccessIT extends AbstractDaemonTest {
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);

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
View File

@@ -82,7 +82,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
@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();
}

3
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/FlushCacheIT.java
View File

@@ -23,13 +23,14 @@ import com.google.gerrit.acceptance.RestResponse;
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();

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
View File

@@ -41,6 +41,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
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 @@ public class AccessIT extends AbstractDaemonTest {
@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 @@ public class AccessIT extends AbstractDaemonTest {
@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();

8
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
View File

@@ -191,7 +191,11 @@ public class CreateProjectIT extends AbstractDaemonTest {
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 @@ public class CreateProjectIT extends AbstractDaemonTest {
}
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 {