From 4a0057373df210c20fbd83b334a34c9aaab68ac1 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 28 Jul 2017 18:30:10 +0200 Subject: [PATCH] Throw an exception when a referenced group is not found Change-Id: Id3c61dcf70492ba05475df4656fd89bad40c9324 --- .../gerrit/acceptance/AccountCreator.java | 2 +- .../gerrit/server/account/AccountManager.java | 10 ++- .../gerrit/server/account/CreateAccount.java | 7 +- .../gerrit/server/account/GroupCacheImpl.java | 6 +- .../server/account/GroupDetailFactory.java | 2 +- .../server/account/GroupIncludeCacheImpl.java | 4 +- .../gerrit/server/account/PutAgreement.java | 13 ++- .../server/group/AddIncludedGroups.java | 13 ++- .../gerrit/server/group/AddMembers.java | 12 ++- .../gerrit/server/group/CreateGroup.java | 13 ++- .../server/group/DeleteIncludedGroups.java | 18 ++-- .../gerrit/server/group/DeleteMembers.java | 15 ++-- .../google/gerrit/server/group/Groups.java | 54 +++++------- .../gerrit/server/group/GroupsUpdate.java | 83 +++++-------------- .../group/IncludedGroupsCollection.java | 13 ++- .../server/group/MembersCollection.java | 14 +++- .../gerrit/server/group/PutDescription.java | 16 ++-- .../google/gerrit/server/group/PutName.java | 20 ++--- .../gerrit/server/group/PutOptions.java | 18 ++-- .../google/gerrit/server/group/PutOwner.java | 19 ++--- .../gerrit/server/mail/send/ProjectWatch.java | 10 ++- 21 files changed, 184 insertions(+), 178 deletions(-) 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 2894399c8e..bfd012bb23 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 @@ -130,7 +130,7 @@ public class AccountCreator { if (groupNames != null) { for (String n : groupNames) { AccountGroup.NameKey k = new AccountGroup.NameKey(n); - Optional group = groups.get(db, k); + Optional group = groups.getGroup(db, k); if (!group.isPresent()) { throw new NoSuchGroupException(n); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index 72a1bf9240..d493cf527e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java @@ -20,6 +20,7 @@ import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.errors.NameAlreadyUsedException; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.client.AccountFieldName; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -125,7 +126,8 @@ public class AccountManager { * @param who identity of the user, with any details we received about them. * @return the result of authenticating the user. * @throws AccountException the account does not exist, and cannot be created, or exists, but - * cannot be located, or is inactive. + * cannot be located, or is inactive, or cannot be added to the admin group (only for the + * first account). */ public AuthResult authenticate(AuthRequest who) throws AccountException, IOException { who = realm.authenticate(who); @@ -264,7 +266,11 @@ public class AccountManager { GroupsUpdate groupsUpdate = groupsUpdateProvider.get(); // The user initiated this request by logging in. -> Attribute all modifications to that user. groupsUpdate.setCurrentUser(user); - groupsUpdate.addGroupMember(db, uuid, newId); + try { + groupsUpdate.addGroupMember(db, uuid, newId); + } catch (NoSuchGroupException e) { + throw new AccountException(String.format("Group %s not found", uuid)); + } } if (who.getUserName() != null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index 23dcd54598..c921c04723 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -20,6 +20,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.errors.InvalidSshKeyException; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.annotations.RequiresCapability; import com.google.gerrit.extensions.api.accounts.AccountInput; import com.google.gerrit.extensions.common.AccountInfo; @@ -186,7 +187,11 @@ public class CreateAccount implements RestModifyView load(AccountGroup.Id key) throws Exception { try (ReviewDb db = schema.open()) { - return groups.get(db, key); + return groups.getGroup(db, key); } } } @@ -205,7 +205,7 @@ public class GroupCacheImpl implements GroupCache { @Override public Optional load(String name) throws Exception { try (ReviewDb db = schema.open()) { - return groups.get(db, new AccountGroup.NameKey(name)); + return groups.getGroup(db, new AccountGroup.NameKey(name)); } } } @@ -223,7 +223,7 @@ public class GroupCacheImpl implements GroupCache { @Override public Optional load(String uuid) throws Exception { try (ReviewDb db = schema.open()) { - return groups.get(db, new AccountGroup.UUID(uuid)); + return groups.getGroup(db, new AccountGroup.UUID(uuid)); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java index 2515401877..47bba6af3e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java @@ -64,7 +64,7 @@ public class GroupDetailFactory implements Callable { return new GroupDetail(members, includes); } - private ImmutableSet loadMembers() throws OrmException { + private ImmutableSet loadMembers() throws OrmException, NoSuchGroupException { return groups.getMembers(db, groupUuid).filter(control::canSeeMember).collect(toImmutableSet()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java index a70abbf501..0d453978f1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; @@ -146,7 +147,8 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache { } @Override - public ImmutableList load(AccountGroup.UUID key) throws OrmException { + public ImmutableList load(AccountGroup.UUID key) + throws OrmException, NoSuchGroupException { try (ReviewDb db = schema.open()) { return groups.getIncludes(db, key).collect(toImmutableList()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java index 1675126cac..f5b2e6e450 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.account; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.data.ContributorAgreement; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.common.AgreementInput; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -43,7 +44,6 @@ import org.eclipse.jgit.lib.Config; @Singleton public class PutAgreement implements RestModifyView { private final ProjectCache projectCache; - private final GroupCache groupCache; private final Provider self; private final AgreementSignup agreementSignup; private final AddMembers addMembers; @@ -52,13 +52,11 @@ public class PutAgreement implements RestModifyView self, AgreementSignup agreementSignup, AddMembers addMembers, @GerritServerConfig Config config) { this.projectCache = projectCache; - this.groupCache = groupCache; this.self = self; this.agreementSignup = agreementSignup; this.addMembers = addMembers; @@ -92,13 +90,12 @@ public class PutAgreement implements RestModifyView { @Override public List apply(GroupResource resource, Input input) - throws MethodNotAllowedException, AuthException, UnprocessableEntityException, OrmException { + throws MethodNotAllowedException, AuthException, UnprocessableEntityException, OrmException, + ResourceNotFoundException { AccountGroup group = resource.toAccountGroup(); if (group == null) { throw new MethodNotAllowedException(); @@ -104,9 +106,12 @@ public class AddIncludedGroups implements RestModifyView { result.add(json.format(d)); } - groupsUpdateProvider - .get() - .addIncludedGroups(db.get(), group.getGroupUUID(), includedGroupUuids); + AccountGroup.UUID groupUuid = group.getGroupUUID(); + try { + groupsUpdateProvider.get().addIncludedGroups(db.get(), groupUuid, includedGroupUuids); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); + } return result; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index db8315f5be..15bcc18564 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.restapi.AuthException; @@ -109,7 +110,7 @@ public class AddMembers implements RestModifyView { @Override public List apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException, ConfigInvalidException { + IOException, ConfigInvalidException, ResourceNotFoundException { AccountGroup internalGroup = resource.toAccountGroup(); if (internalGroup == null) { throw new MethodNotAllowedException(); @@ -131,7 +132,12 @@ public class AddMembers implements RestModifyView { newMemberIds.add(a.getId()); } - addMembers(internalGroup.getGroupUUID(), newMemberIds); + AccountGroup.UUID groupUuid = internalGroup.getGroupUUID(); + try { + addMembers(groupUuid, newMemberIds); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); + } return toAccountInfoList(newMemberIds); } @@ -169,7 +175,7 @@ public class AddMembers implements RestModifyView { } public void addMembers(AccountGroup.UUID groupUuid, Collection newMemberIds) - throws OrmException, IOException { + throws OrmException, IOException, NoSuchGroupException { groupsUpdateProvider .get() .addGroupMembers(db.get(), groupUuid, ImmutableSet.copyOf(newMemberIds)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java index 1be2be7b87..261269eb9d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java @@ -20,6 +20,7 @@ import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupDescriptions; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.annotations.RequiresCapability; import com.google.gerrit.extensions.api.groups.GroupInput; import com.google.gerrit.extensions.client.ListGroupsOption; @@ -28,6 +29,7 @@ import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; @@ -118,7 +120,8 @@ public class CreateGroup implements RestModifyView @Override public GroupInfo apply(TopLevelResource resource, GroupInput input) throws AuthException, BadRequestException, UnprocessableEntityException, - ResourceConflictException, OrmException, IOException, ConfigInvalidException { + ResourceConflictException, OrmException, IOException, ConfigInvalidException, + ResourceNotFoundException { if (input == null) { input = new GroupInput(); } @@ -170,7 +173,7 @@ public class CreateGroup implements RestModifyView } private AccountGroup createGroup(CreateGroupArgs createGroupArgs) - throws OrmException, ResourceConflictException, IOException { + throws OrmException, ResourceConflictException, IOException, ResourceNotFoundException { // Do not allow creating groups with the same name as system groups for (String name : systemGroupBackend.getNames()) { @@ -211,7 +214,11 @@ public class CreateGroup implements RestModifyView "group '" + createGroupArgs.getGroupName() + "' already exists"); } - addMembers.addMembers(uuid, createGroupArgs.initialMembers); + try { + addMembers.addMembers(uuid, createGroupArgs.initialMembers); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", uuid)); + } groupCache.onCreateGroup(createGroupArgs.getGroup()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java index 4bb892c2c0..ebdb12d62c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java @@ -16,8 +16,10 @@ package com.google.gerrit.server.group; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.data.GroupDescription; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; @@ -50,7 +52,8 @@ public class DeleteIncludedGroups implements RestModifyView apply(GroupResource resource, Input input) - throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException { + throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, + ResourceNotFoundException { AccountGroup internalGroup = resource.toAccountGroup(); if (internalGroup == null) { throw new MethodNotAllowedException(); @@ -69,9 +72,12 @@ public class DeleteIncludedGroups implements RestModifyView apply(IncludedGroupResource resource, Input input) - throws AuthException, MethodNotAllowedException, UnprocessableEntityException, - OrmException { + throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, + ResourceNotFoundException { AddIncludedGroups.Input in = new AddIncludedGroups.Input(); in.groups = ImmutableList.of(resource.getMember().get()); return delete.get().apply(resource, in); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java index e29fa0067c..babff37ecb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java @@ -14,8 +14,10 @@ package com.google.gerrit.server.group; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; @@ -53,7 +55,7 @@ public class DeleteMembers implements RestModifyView { @Override public Response apply(GroupResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException, ConfigInvalidException { + IOException, ConfigInvalidException, ResourceNotFoundException { AccountGroup internalGroup = resource.toAccountGroup(); if (internalGroup == null) { throw new MethodNotAllowedException(); @@ -70,9 +72,12 @@ public class DeleteMembers implements RestModifyView { Account a = accounts.parse(nameOrEmail).getAccount(); membersToRemove.add(a.getId()); } - groupsUpdateProvider - .get() - .removeGroupMembers(db.get(), internalGroup.getGroupUUID(), membersToRemove); + AccountGroup.UUID groupUuid = internalGroup.getGroupUUID(); + try { + groupsUpdateProvider.get().removeGroupMembers(db.get(), groupUuid, membersToRemove); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); + } return Response.none(); } @@ -91,7 +96,7 @@ public class DeleteMembers implements RestModifyView { @Override public Response apply(MemberResource resource, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException, - IOException, ConfigInvalidException { + IOException, ConfigInvalidException, ResourceNotFoundException { AddMembers.Input in = new AddMembers.Input(); in._oneMember = resource.getMember().getAccountId().toString(); return delete.get().apply(resource, in); 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 9adfc1da40..578e4d1f79 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 @@ -16,6 +16,7 @@ package com.google.gerrit.server.group; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupById; @@ -33,11 +34,18 @@ import java.util.stream.Stream; @Singleton public class Groups { - public Optional get(ReviewDb db, AccountGroup.Id groupId) throws OrmException { + public AccountGroup getExistingGroup(ReviewDb db, AccountGroup.UUID groupUuid) + throws OrmException, NoSuchGroupException { + Optional group = getGroup(db, groupUuid); + return group.orElseThrow(() -> new NoSuchGroupException(groupUuid)); + } + + public Optional getGroup(ReviewDb db, AccountGroup.Id groupId) throws OrmException { return Optional.ofNullable(db.accountGroups().get(groupId)); } - public Optional get(ReviewDb db, AccountGroup.UUID groupUuid) throws OrmException { + public Optional getGroup(ReviewDb db, AccountGroup.UUID groupUuid) + throws OrmException { List accountGroups = db.accountGroups().byUUID(groupUuid).toList(); if (accountGroups.size() == 1) { return Optional.of(Iterables.getOnlyElement(accountGroups)); @@ -48,7 +56,7 @@ public class Groups { } } - public Optional get(ReviewDb db, AccountGroup.NameKey groupName) + public Optional getGroup(ReviewDb db, AccountGroup.NameKey groupName) throws OrmException { AccountGroupName accountGroupName = db.accountGroupNames().get(groupName); if (accountGroupName == null) { @@ -64,55 +72,31 @@ public class Groups { } public boolean isMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId) - throws OrmException { - Optional foundGroup = get(db, groupUuid); - if (!foundGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return false; - } - - AccountGroup group = foundGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup group = getExistingGroup(db, groupUuid); AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, group.getId()); return db.accountGroupMembers().get(key) != null; } public boolean isIncluded( ReviewDb db, AccountGroup.UUID parentGroupUuid, AccountGroup.UUID includedGroupUuid) - throws OrmException { - Optional foundParentGroup = get(db, parentGroupUuid); - if (!foundParentGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return false; - } - - AccountGroup parentGroup = foundParentGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup parentGroup = getExistingGroup(db, parentGroupUuid); AccountGroupById.Key key = new AccountGroupById.Key(parentGroup.getId(), includedGroupUuid); return db.accountGroupById().get(key) != null; } public Stream getMembers(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException { - Optional foundGroup = get(db, groupUuid); - if (!foundGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return Stream.empty(); - } - - AccountGroup group = foundGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup group = getExistingGroup(db, groupUuid); ResultSet accountGroupMembers = db.accountGroupMembers().byGroup(group.getId()); return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId); } public Stream getIncludes(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException { - Optional foundGroup = get(db, groupUuid); - if (!foundGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return Stream.empty(); - } - - AccountGroup group = foundGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup group = getExistingGroup(db, groupUuid); ResultSet accountGroupByIds = db.accountGroupById().byGroup(group.getId()); return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct(); } 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 3731097fe9..0aea001f6f 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 @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet; import com.google.gerrit.audit.AuditService; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.NameAlreadyUsedException; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupById; @@ -37,7 +38,6 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.io.IOException; import java.util.HashSet; -import java.util.Optional; import java.util.Set; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -121,40 +121,26 @@ public class GroupsUpdate { db.accountGroups().insert(ImmutableList.of(group)); } - public Optional updateGroup( + public void updateGroup( ReviewDb db, AccountGroup.UUID groupUuid, Consumer groupConsumer) - throws OrmException, IOException { - Optional updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer); - if (updatedGroup.isPresent()) { - groupCache.evict(updatedGroup.get()); - } - return updatedGroup; + throws OrmException, IOException, NoSuchGroupException { + AccountGroup updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer); + groupCache.evict(updatedGroup); } @VisibleForTesting - public Optional updateGroupInDb( + public AccountGroup updateGroupInDb( ReviewDb db, AccountGroup.UUID groupUuid, Consumer groupConsumer) - throws OrmException, IOException { - Optional foundGroup = groups.get(db, groupUuid); - if (!foundGroup.isPresent()) { - return Optional.empty(); - } - - AccountGroup group = foundGroup.get(); + throws OrmException, IOException, NoSuchGroupException { + AccountGroup group = groups.getExistingGroup(db, groupUuid); groupConsumer.accept(group); db.accountGroups().update(ImmutableList.of(group)); - return Optional.of(group); + return group; } - public Optional renameGroup( - ReviewDb db, AccountGroup.UUID groupUuid, AccountGroup.NameKey newName) - throws OrmException, IOException, NameAlreadyUsedException { - Optional foundGroup = groups.get(db, groupUuid); - if (!foundGroup.isPresent()) { - return Optional.empty(); - } - - AccountGroup group = foundGroup.get(); + public void renameGroup(ReviewDb db, AccountGroup.UUID groupUuid, AccountGroup.NameKey newName) + throws OrmException, IOException, NameAlreadyUsedException, NoSuchGroupException { + AccountGroup group = groups.getExistingGroup(db, groupUuid); AccountGroup.NameKey oldName = group.getNameKey(); try { @@ -165,7 +151,7 @@ public class GroupsUpdate { if (other != null) { // If we are using this identity, don't report the exception. if (other.getId().equals(group.getId())) { - return Optional.of(group); + return; } // Otherwise, someone else has this identity. @@ -187,23 +173,16 @@ public class GroupsUpdate { renameGroupOpFactory .create(committerIdent, groupUuid, oldName.get(), newName.get()) .start(0, TimeUnit.MILLISECONDS); - return Optional.of(group); } public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId) - throws OrmException, IOException { + throws OrmException, IOException, NoSuchGroupException { addGroupMembers(db, groupUuid, ImmutableSet.of(accountId)); } public void addGroupMembers(ReviewDb db, AccountGroup.UUID groupUuid, Set accountIds) - throws OrmException, IOException { - Optional foundGroup = groups.get(db, groupUuid); - if (!foundGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return; - } - - AccountGroup group = foundGroup.get(); + throws OrmException, IOException, NoSuchGroupException { + AccountGroup group = groups.getExistingGroup(db, groupUuid); AccountGroup.Id groupId = group.getId(); Set newMembers = new HashSet<>(); for (Account.Id accountId : accountIds) { @@ -229,14 +208,8 @@ public class GroupsUpdate { public void removeGroupMembers( ReviewDb db, AccountGroup.UUID groupUuid, Set accountIds) - throws OrmException, IOException { - Optional foundGroup = groups.get(db, groupUuid); - if (!foundGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return; - } - - AccountGroup group = foundGroup.get(); + throws OrmException, IOException, NoSuchGroupException { + AccountGroup group = groups.getExistingGroup(db, groupUuid); AccountGroup.Id groupId = group.getId(); Set membersToRemove = new HashSet<>(); for (Account.Id accountId : accountIds) { @@ -262,14 +235,8 @@ public class GroupsUpdate { public void addIncludedGroups( ReviewDb db, AccountGroup.UUID parentGroupUuid, Set includedGroupUuids) - throws OrmException { - Optional foundParentGroup = groups.get(db, parentGroupUuid); - if (!foundParentGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return; - } - - AccountGroup parentGroup = foundParentGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid); AccountGroup.Id parentGroupId = parentGroup.getId(); Set newIncludedGroups = new HashSet<>(); for (AccountGroup.UUID includedGroupUuid : includedGroupUuids) { @@ -296,14 +263,8 @@ public class GroupsUpdate { public void deleteIncludedGroups( ReviewDb db, AccountGroup.UUID parentGroupUuid, Set includedGroupUuids) - throws OrmException { - Optional foundParentGroup = groups.get(db, parentGroupUuid); - if (!foundParentGroup.isPresent()) { - // TODO(aliceks): Throw an exception? - return; - } - - AccountGroup parentGroup = foundParentGroup.get(); + throws OrmException, NoSuchGroupException { + AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid); AccountGroup.Id parentGroupId = parentGroup.getId(); Set includedGroupsToRemove = new HashSet<>(); for (AccountGroup.UUID includedGroupUuid : includedGroupUuids) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java index af0dc1780c..1fecc38c11 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.group; import com.google.gerrit.common.data.GroupDescription; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AcceptsCreate; import com.google.gerrit.extensions.restapi.AuthException; @@ -73,14 +74,20 @@ public class IncludedGroupsCollection GroupDescription.Basic member = groupsCollection.parse(TopLevelResource.INSTANCE, id).getGroup(); - if (isMember(parent, member) && resource.getControl().canSeeGroup()) { + if (resource.getControl().canSeeGroup() && isMember(parent, member)) { return new IncludedGroupResource(resource, member); } throw new ResourceNotFoundException(id); } - private boolean isMember(AccountGroup parent, GroupDescription.Basic member) throws OrmException { - return groups.isIncluded(dbProvider.get(), parent.getGroupUUID(), member.getGroupUUID()); + private boolean isMember(AccountGroup parent, GroupDescription.Basic member) + throws OrmException, ResourceNotFoundException { + try { + return groups.isIncluded(dbProvider.get(), parent.getGroupUUID(), member.getGroupUUID()); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException( + String.format("Group %s not found", parent.getGroupUUID())); + } } @SuppressWarnings("unchecked") diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java index 9c9c0d6a57..66f5b264e4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.group; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AcceptsCreate; import com.google.gerrit.extensions.restapi.AuthException; @@ -76,13 +77,22 @@ public class MembersCollection } IdentifiedUser user = accounts.parse(TopLevelResource.INSTANCE, id).getUser(); - if (groups.isMember(db.get(), group.getGroupUUID(), user.getAccountId()) - && parent.getControl().canSeeMember(user.getAccountId())) { + if (parent.getControl().canSeeMember(user.getAccountId()) && isMember(group, user)) { return new MemberResource(parent, user); } throw new ResourceNotFoundException(id); } + private boolean isMember(AccountGroup group, IdentifiedUser user) + throws OrmException, ResourceNotFoundException { + AccountGroup.UUID groupUuid = group.getGroupUUID(); + try { + return groups.isMember(db.get(), groupUuid, user.getAccountId()); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); + } + } + @SuppressWarnings("unchecked") @Override public PutMember create(GroupResource group, IdString id) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java index 270fbac23c..f0709b49a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.DefaultInput; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; @@ -30,7 +31,6 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.Objects; -import java.util.Optional; @Singleton public class PutDescription implements RestModifyView { @@ -65,13 +65,13 @@ public class PutDescription implements RestModifyView { String newDescription = Strings.emptyToNull(input.description); if (!Objects.equals(accountGroup.getDescription(), newDescription)) { - Optional updatedGroup = - groupsUpdateProvider - .get() - .updateGroup( - db.get(), resource.getGroupUUID(), group -> group.setDescription(newDescription)); - if (!updatedGroup.isPresent()) { - throw new ResourceNotFoundException(); + AccountGroup.UUID groupUuid = resource.getGroupUUID(); + try { + groupsUpdateProvider + .get() + .updateGroup(db.get(), groupUuid, group -> group.setDescription(newDescription)); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java index a6dae11fba..9ef8d87273 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; import com.google.gerrit.common.errors.NameAlreadyUsedException; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.DefaultInput; @@ -31,7 +32,6 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Optional; @Singleton public class PutName implements RestModifyView { @@ -68,19 +68,19 @@ public class PutName implements RestModifyView { return newName; } - return renameGroup(rsrc.toAccountGroup(), newName); + renameGroup(rsrc.toAccountGroup(), newName); + return newName; } - private String renameGroup(AccountGroup group, String newName) + private void renameGroup(AccountGroup group, String newName) throws ResourceConflictException, ResourceNotFoundException, OrmException, IOException { + AccountGroup.UUID groupUuid = group.getGroupUUID(); try { - Optional renamedGroup = - groupsUpdateProvider - .get() - .renameGroup(db.get(), group.getGroupUUID(), new AccountGroup.NameKey(newName)); - return renamedGroup - .map(AccountGroup::getName) - .orElseThrow(() -> new ResourceNotFoundException("Group not found")); + groupsUpdateProvider + .get() + .renameGroup(db.get(), groupUuid, new AccountGroup.NameKey(newName)); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } catch (NameAlreadyUsedException e) { throw new ResourceConflictException("group with name " + newName + " already exists"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java index 1559153275..69ce64b0e8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.group; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.common.GroupOptionsInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -27,7 +28,6 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Optional; @Singleton public class PutOptions implements RestModifyView { @@ -59,15 +59,13 @@ public class PutOptions implements RestModifyView updatedGroup = - groupsUpdateProvider - .get() - .updateGroup( - db.get(), - accountGroup.getGroupUUID(), - group -> group.setVisibleToAll(input.visibleToAll)); - if (!updatedGroup.isPresent()) { - throw new ResourceNotFoundException(); + AccountGroup.UUID groupUuid = accountGroup.getGroupUUID(); + try { + groupsUpdateProvider + .get() + .updateGroup(db.get(), groupUuid, group -> group.setVisibleToAll(input.visibleToAll)); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java index 953a0d23b6..6dd0809711 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.group; import com.google.common.base.Strings; import com.google.gerrit.common.data.GroupDescription; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -32,7 +33,6 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Optional; @Singleton public class PutOwner implements RestModifyView { @@ -74,15 +74,14 @@ public class PutOwner implements RestModifyView { GroupDescription.Basic owner = groupsCollection.parse(input.owner); if (!accountGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) { - Optional updatedGroup = - groupsUpdateProvider - .get() - .updateGroup( - db.get(), - resource.getGroupUUID(), - group -> group.setOwnerGroupUUID(owner.getGroupUUID())); - if (!updatedGroup.isPresent()) { - throw new ResourceNotFoundException(); + AccountGroup.UUID groupUuid = resource.getGroupUUID(); + try { + groupsUpdateProvider + .get() + .updateGroup( + db.get(), groupUuid, group -> group.setOwnerGroupUUID(owner.getGroupUUID())); + } catch (NoSuchGroupException e) { + throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } } return json.format(owner); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java index d950a57cba..6363afacfc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java @@ -18,6 +18,7 @@ import com.google.common.base.Strings; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; @@ -165,6 +166,9 @@ public class ProjectWatch { while (!q.isEmpty()) { AccountGroup.UUID uuid = q.remove(q.size() - 1); GroupDescription.Basic group = args.groupBackend.get(uuid); + if (group == null) { + continue; + } if (!Strings.isNullOrEmpty(group.getEmailAddress())) { // If the group has an email address, do not expand membership. matching.emails.add(new Address(group.getEmailAddress())); @@ -177,7 +181,11 @@ public class ProjectWatch { continue; } - args.groups.getMembers(db, ig.getGroupUUID()).forEach(matching.accounts::add); + try { + args.groups.getMembers(db, ig.getGroupUUID()).forEach(matching.accounts::add); + } catch (NoSuchGroupException e) { + continue; + } for (AccountGroup.UUID m : args.groupIncludes.subgroupsOf(uuid)) { if (seen.add(m)) { q.add(m);