opendev/gerrit
Code Issues Proposed changes

Switch to UUID of groups for all public methods of GroupsUpdate

Browse Source 
Change-Id: I9a4bb23b69bf50b19b30f29ddbf65e621e42968a
This commit is contained in:
Alice Kober-Sotzek
2017-07-28 11:27:25 +02:00
parent 6f2b3c95d6
commit 01154ca574
7 changed files with 36 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
View File

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
import static java.nio.charset.StandardCharsets.US_ASCII; import static java.nio.charset.StandardCharsets.US_ASCII;
import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
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.reviewdb.server.ReviewDb;
@@ -28,6 +29,7 @@ import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.VersionedAuthorizedKeys; import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; 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.GroupsUpdate;
import com.google.gerrit.server.group.ServerInitiated; import com.google.gerrit.server.group.ServerInitiated;
import com.google.gerrit.server.ssh.SshKeyCache; import com.google.gerrit.server.ssh.SshKeyCache;
@@ -44,6 +46,7 @@ import java.util.ArrayList;
import java.util.HashMap; import java.util.HashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Optional;
@Singleton @Singleton
public class AccountCreator { public class AccountCreator {
@@ -53,6 +56,7 @@ public class AccountCreator {
private final Sequences sequences; private final Sequences sequences;
private final AccountsUpdate.Server accountsUpdate; private final AccountsUpdate.Server accountsUpdate;
private final VersionedAuthorizedKeys.Accessor authorizedKeys; private final VersionedAuthorizedKeys.Accessor authorizedKeys;
private final Groups groups;
private final Provider<GroupsUpdate> groupsUpdateProvider; private final Provider<GroupsUpdate> groupsUpdateProvider;
private final SshKeyCache sshKeyCache; private final SshKeyCache sshKeyCache;
private final AccountCache accountCache; private final AccountCache accountCache;
@@ -66,6 +70,7 @@ public class AccountCreator {
Sequences sequences, Sequences sequences,
AccountsUpdate.Server accountsUpdate, AccountsUpdate.Server accountsUpdate,
VersionedAuthorizedKeys.Accessor authorizedKeys, VersionedAuthorizedKeys.Accessor authorizedKeys,
Groups groups,
@ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider, @ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider,
SshKeyCache sshKeyCache, SshKeyCache sshKeyCache,
AccountCache accountCache, AccountCache accountCache,
@@ -77,6 +82,7 @@ public class AccountCreator {
this.sequences = sequences; this.sequences = sequences;
this.accountsUpdate = accountsUpdate; this.accountsUpdate = accountsUpdate;
this.authorizedKeys = authorizedKeys; this.authorizedKeys = authorizedKeys;
this.groups = groups;
this.groupsUpdateProvider = groupsUpdateProvider; this.groupsUpdateProvider = groupsUpdateProvider;
this.sshKeyCache = sshKeyCache; this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache; this.accountCache = accountCache;
@@ -89,7 +95,7 @@ public class AccountCreator {
@Nullable String username, @Nullable String username,
@Nullable String email, @Nullable String email,
@Nullable String fullName, @Nullable String fullName,
String... groups) String... groupNames)
throws Exception { throws Exception {
TestAccount account = accounts.get(username); TestAccount account = accounts.get(username);
@@ -121,10 +127,14 @@ public class AccountCreator {
a.setPreferredEmail(email); a.setPreferredEmail(email);
}); });
if (groups != null) { if (groupNames != null) {
for (String n : groups) { for (String n : groupNames) {
AccountGroup.NameKey k = new AccountGroup.NameKey(n); AccountGroup.NameKey k = new AccountGroup.NameKey(n);
groupsUpdateProvider.get().addGroupMember(db, k, id); Optional<AccountGroup> group = groups.get(db, k);
if (!group.isPresent()) {
throw new NoSuchGroupException(n);
}
groupsUpdateProvider.get().addGroupMember(db, group.get().getGroupUUID(), id);
} }
} }

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

@@ -132,7 +132,7 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
"Username '" + username + "' must contain only letters, numbers, _, - or ."); "Username '" + username + "' must contain only letters, numbers, _, - or .");
} }
Set<AccountGroup.Id> groups = parseGroups(input.groups); Set<AccountGroup.UUID> groups = parseGroups(input.groups);
Account.Id id = new Account.Id(seq.nextAccountId()); Account.Id id = new Account.Id(seq.nextAccountId());
@@ -185,8 +185,8 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
a.setPreferredEmail(input.email); a.setPreferredEmail(input.email);
}); });
for (AccountGroup.Id groupId : groups) { for (AccountGroup.UUID groupUuid : groups) {
groupsUpdate.get().addGroupMember(db, groupId, id); groupsUpdate.get().addGroupMember(db, groupUuid, id);
} }
if (input.sshKey != null) { if (input.sshKey != null) {
@@ -207,14 +207,15 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
return Response.created(info); return Response.created(info);
} }
private Set<AccountGroup.Id> parseGroups(List<String> groups) private Set<AccountGroup.UUID> parseGroups(List<String> groups)
throws UnprocessableEntityException { throws UnprocessableEntityException {
Set<AccountGroup.Id> groupIds = new HashSet<>(); Set<AccountGroup.UUID> groupUuids = new HashSet<>();
if (groups != null) { if (groups != null) {
for (String g : groups) { for (String g : groups) {
groupIds.add(GroupDescriptions.toAccountGroup(groupsCollection.parseInternal(g)).getId()); AccountGroup group = GroupDescriptions.toAccountGroup(groupsCollection.parseInternal(g));
groupUuids.add(group.getGroupUUID());
} }
} }
return groupIds; return groupUuids;
} }
} }

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

@@ -98,7 +98,7 @@ public class PutAgreement implements RestModifyView<AccountResource, AgreementIn
} }
Account account = self.get().getAccount(); Account account = self.get().getAccount();
addMembers.addMembers(group.getId(), ImmutableList.of(account.getId())); addMembers.addMembers(group.getGroupUUID(), ImmutableList.of(account.getId()));
agreementSignup.fire(account, agreementName); agreementSignup.fire(account, agreementName);
return Response.ok(agreementName); return Response.ok(agreementName);

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

@@ -131,7 +131,7 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
newMemberIds.add(a.getId()); newMemberIds.add(a.getId());
} }
addMembers(internalGroup.getId(), newMemberIds); addMembers(internalGroup.getGroupUUID(), newMemberIds);
return toAccountInfoList(newMemberIds); return toAccountInfoList(newMemberIds);
} }
@@ -168,11 +168,11 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
} }
} }
public void addMembers(AccountGroup.Id groupId, Collection<? extends Account.Id> newMemberIds) public void addMembers(AccountGroup.UUID groupUuid, Collection<? extends Account.Id> newMemberIds)
throws OrmException, IOException { throws OrmException, IOException {
groupsUpdateProvider groupsUpdateProvider
.get() .get()
.addGroupMembers(db.get(), groupId, ImmutableSet.copyOf(newMemberIds)); .addGroupMembers(db.get(), groupUuid, ImmutableSet.copyOf(newMemberIds));
} }
private Account createAccountByLdap(String user) throws IOException { private Account createAccountByLdap(String user) throws IOException {

2
gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
View File

@@ -211,7 +211,7 @@ public class CreateGroup implements RestModifyView<TopLevelResource, GroupInput>
"group '" + createGroupArgs.getGroupName() + "' already exists"); "group '" + createGroupArgs.getGroupName() + "' already exists");
} }
addMembers.addMembers(groupId, createGroupArgs.initialMembers); addMembers.addMembers(uuid, createGroupArgs.initialMembers);
groupCache.onCreateGroup(createGroupArgs.getGroup()); groupCache.onCreateGroup(createGroupArgs.getGroup());

49
gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
View File

@@ -147,9 +147,9 @@ public class GroupsUpdate {
} }
public Optional<AccountGroup> renameGroup( public Optional<AccountGroup> renameGroup(
ReviewDb db, AccountGroup.Id groupId, AccountGroup.NameKey newName) ReviewDb db, AccountGroup.UUID groupUuid, AccountGroup.NameKey newName)
throws OrmException, IOException, NameAlreadyUsedException { throws OrmException, IOException, NameAlreadyUsedException {
Optional<AccountGroup> foundGroup = groups.get(db, groupId); Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
if (!foundGroup.isPresent()) { if (!foundGroup.isPresent()) {
return Optional.empty(); return Optional.empty();
} }
@@ -158,13 +158,13 @@ public class GroupsUpdate {
AccountGroup.NameKey oldName = group.getNameKey(); AccountGroup.NameKey oldName = group.getNameKey();
try { try {
AccountGroupName id = new AccountGroupName(newName, groupId); AccountGroupName id = new AccountGroupName(newName, group.getId());
db.accountGroupNames().insert(ImmutableList.of(id)); db.accountGroupNames().insert(ImmutableList.of(id));
} catch (OrmException e) { } catch (OrmException e) {
AccountGroupName other = db.accountGroupNames().get(newName); AccountGroupName other = db.accountGroupNames().get(newName);
if (other != null) { if (other != null) {
// If we are using this identity, don't report the exception. // If we are using this identity, don't report the exception.
if (other.getId().equals(groupId)) { if (other.getId().equals(group.getId())) {
return Optional.of(group); return Optional.of(group);
} }
@@ -185,25 +185,18 @@ public class GroupsUpdate {
@SuppressWarnings("unused") @SuppressWarnings("unused")
Future<?> possiblyIgnoredError = Future<?> possiblyIgnoredError =
renameGroupOpFactory renameGroupOpFactory
.create(committerIdent, group.getGroupUUID(), oldName.get(), newName.get()) .create(committerIdent, groupUuid, oldName.get(), newName.get())
.start(0, TimeUnit.MILLISECONDS); .start(0, TimeUnit.MILLISECONDS);
return Optional.of(group); return Optional.of(group);
} }
public void addGroupMember(ReviewDb db, AccountGroup.NameKey groupName, Account.Id accountId)
throws OrmException, IOException {
Optional<AccountGroup> foundGroup = groups.get(db, groupName);
if (!foundGroup.isPresent()) {
// TODO(aliceks): Throw an exception?
return;
}
AccountGroup group = foundGroup.get();
addGroupMembers(db, group, ImmutableSet.of(accountId));
}
public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId) public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId)
throws OrmException, IOException { throws OrmException, IOException {
addGroupMembers(db, groupUuid, ImmutableSet.of(accountId));
}
public void addGroupMembers(ReviewDb db, AccountGroup.UUID groupUuid, Set<Account.Id> accountIds)
throws OrmException, IOException {
Optional<AccountGroup> foundGroup = groups.get(db, groupUuid); Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
if (!foundGroup.isPresent()) { if (!foundGroup.isPresent()) {
// TODO(aliceks): Throw an exception? // TODO(aliceks): Throw an exception?
@@ -211,28 +204,6 @@ public class GroupsUpdate {
} }
AccountGroup group = foundGroup.get(); AccountGroup group = foundGroup.get();
addGroupMembers(db, group, ImmutableSet.of(accountId));
}
public void addGroupMember(ReviewDb db, AccountGroup.Id groupId, Account.Id accountId)
throws OrmException, IOException {
addGroupMembers(db, groupId, ImmutableSet.of(accountId));
}
public void addGroupMembers(ReviewDb db, AccountGroup.Id groupId, Set<Account.Id> accountIds)
throws OrmException, IOException {
Optional<AccountGroup> foundGroup = groups.get(db, groupId);
if (!foundGroup.isPresent()) {
// TODO(aliceks): Throw an exception?
return;
}
AccountGroup group = foundGroup.get();
addGroupMembers(db, group, accountIds);
}
private void addGroupMembers(ReviewDb db, AccountGroup group, Set<Account.Id> accountIds)
throws OrmException, IOException {
AccountGroup.Id groupId = group.getId(); AccountGroup.Id groupId = group.getId();
Set<AccountGroupMember> newMembers = new HashSet<>(); Set<AccountGroupMember> newMembers = new HashSet<>();
for (Account.Id accountId : accountIds) { for (Account.Id accountId : accountIds) {

2
gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
View File

@@ -77,7 +77,7 @@ public class PutName implements RestModifyView<GroupResource, Input> {
Optional<AccountGroup> renamedGroup = Optional<AccountGroup> renamedGroup =
groupsUpdateProvider groupsUpdateProvider
.get() .get()
.renameGroup(db.get(), group.getId(), new AccountGroup.NameKey(newName)); .renameGroup(db.get(), group.getGroupUUID(), new AccountGroup.NameKey(newName));
return renamedGroup return renamedGroup
.map(AccountGroup::getName) .map(AccountGroup::getName)
.orElseThrow(() -> new ResourceNotFoundException("Group not found")); .orElseThrow(() -> new ResourceNotFoundException("Group not found"));