Throw an exception when a referenced group is not found

Change-Id: Id3c61dcf70492ba05475df4656fd89bad40c9324

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<AccountGroup> group = groups.get(db, k);
+          Optional<AccountGroup> group = groups.getGroup(db, k);
           if (!group.isPresent()) {
             throw new NoSuchGroupException(n);
           }

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) {

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<TopLevelResource, AccountIn
             });
 
     for (AccountGroup.UUID groupUuid : groups) {
-      groupsUpdate.get().addGroupMember(db, groupUuid, id);
+      try {
+        groupsUpdate.get().addGroupMember(db, groupUuid, id);
+      } catch (NoSuchGroupException e) {
+        throw new UnprocessableEntityException(String.format("Group %s not found", groupUuid));
+      }
     }
 
     if (input.sshKey != null) {

gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java

@@ -187,7 +187,7 @@ public class GroupCacheImpl implements GroupCache {
     @Override
     public Optional<AccountGroup> 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<AccountGroup> 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<AccountGroup> 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));
       }
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java

@@ -64,7 +64,7 @@ public class GroupDetailFactory implements Callable<GroupDetail> {
     return new GroupDetail(members, includes);
   }
 
-  private ImmutableSet<Account.Id> loadMembers() throws OrmException {
+  private ImmutableSet<Account.Id> loadMembers() throws OrmException, NoSuchGroupException {
     return groups.getMembers(db, groupUuid).filter(control::canSeeMember).collect(toImmutableSet());
   }
 

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<AccountGroup.UUID> load(AccountGroup.UUID key) throws OrmException {
+    public ImmutableList<AccountGroup.UUID> load(AccountGroup.UUID key)
+        throws OrmException, NoSuchGroupException {
       try (ReviewDb db = schema.open()) {
         return groups.getIncludes(db, key).collect(toImmutableList());
       }

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<AccountResource, AgreementInput> {
   private final ProjectCache projectCache;
-  private final GroupCache groupCache;
   private final Provider<IdentifiedUser> self;
   private final AgreementSignup agreementSignup;
   private final AddMembers addMembers;
@@ -52,13 +52,11 @@ public class PutAgreement implements RestModifyView<AccountResource, AgreementIn
   @Inject
   PutAgreement(
       ProjectCache projectCache,
-      GroupCache groupCache,
       Provider<IdentifiedUser> 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<AccountResource, AgreementIn
       throw new ResourceConflictException("autoverify group uuid not found");
     }
 
-    AccountGroup group = groupCache.get(uuid);
+    Account account = self.get().getAccount();
-    if (group == null) {
+    try {
+      addMembers.addMembers(uuid, ImmutableList.of(account.getId()));
+    } catch (NoSuchGroupException e) {
       throw new ResourceConflictException("autoverify group not found");
     }
-
-    Account account = self.get().getAccount();
-    addMembers.addMembers(group.getGroupUUID(), ImmutableList.of(account.getId()));
     agreementSignup.fire(account, agreementName);
 
     return Response.ok(agreementName);

gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java

@@ -18,6 +18,7 @@ import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 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.DefaultInput;
@@ -84,7 +85,8 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
 
   @Override
   public List<GroupInfo> 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<GroupResource, Input> {
       result.add(json.format(d));
     }
 
-    groupsUpdateProvider
+    AccountGroup.UUID groupUuid = group.getGroupUUID();
-        .get()
+    try {
-        .addIncludedGroups(db.get(), group.getGroupUUID(), includedGroupUuids);
+      groupsUpdateProvider.get().addIncludedGroups(db.get(), groupUuid, includedGroupUuids);
+    } catch (NoSuchGroupException e) {
+      throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+    }
     return result;
   }
 

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<GroupResource, Input> {
   @Override
   public List<AccountInfo> 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<GroupResource, Input> {
       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<GroupResource, Input> {
   }
 
   public void addMembers(AccountGroup.UUID groupUuid, Collection<? extends Account.Id> newMemberIds)
-      throws OrmException, IOException {
+      throws OrmException, IOException, NoSuchGroupException {
     groupsUpdateProvider
         .get()
         .addGroupMembers(db.get(), groupUuid, ImmutableSet.copyOf(newMemberIds));

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<TopLevelResource, GroupInput>
   @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<TopLevelResource, GroupInput>
   }
 
   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<TopLevelResource, GroupInput>
           "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());
 

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<GroupResource, Input
 
   @Override
   public Response<?> 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<GroupResource, Input
       internalGroupsToRemove.add(d.getGroupUUID());
     }
 
-    groupsUpdateProvider
+    AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
-        .get()
+    try {
-        .deleteIncludedGroups(db.get(), internalGroup.getGroupUUID(), internalGroupsToRemove);
+      groupsUpdateProvider.get().deleteIncludedGroups(db.get(), groupUuid, internalGroupsToRemove);
+    } catch (NoSuchGroupException e) {
+      throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+    }
 
     return Response.none();
   }
@@ -90,8 +96,8 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
 
     @Override
     public Response<?> apply(IncludedGroupResource resource, Input input)
-        throws AuthException, MethodNotAllowedException, UnprocessableEntityException,
+        throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
-            OrmException {
+            ResourceNotFoundException {
       AddIncludedGroups.Input in = new AddIncludedGroups.Input();
       in.groups = ImmutableList.of(resource.getMember().get());
       return delete.get().apply(resource, in);

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<GroupResource, Input> {
   @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<GroupResource, Input> {
       Account a = accounts.parse(nameOrEmail).getAccount();
       membersToRemove.add(a.getId());
     }
-    groupsUpdateProvider
+    AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
-        .get()
+    try {
-        .removeGroupMembers(db.get(), internalGroup.getGroupUUID(), membersToRemove);
+      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<GroupResource, Input> {
     @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);

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<AccountGroup> get(ReviewDb db, AccountGroup.Id groupId) throws OrmException {
+  public AccountGroup getExistingGroup(ReviewDb db, AccountGroup.UUID groupUuid)
+      throws OrmException, NoSuchGroupException {
+    Optional<AccountGroup> group = getGroup(db, groupUuid);
+    return group.orElseThrow(() -> new NoSuchGroupException(groupUuid));
+  }
+
+  public Optional<AccountGroup> getGroup(ReviewDb db, AccountGroup.Id groupId) throws OrmException {
     return Optional.ofNullable(db.accountGroups().get(groupId));
   }
 
-  public Optional<AccountGroup> get(ReviewDb db, AccountGroup.UUID groupUuid) throws OrmException {
+  public Optional<AccountGroup> getGroup(ReviewDb db, AccountGroup.UUID groupUuid)
+      throws OrmException {
     List<AccountGroup> 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<AccountGroup> get(ReviewDb db, AccountGroup.NameKey groupName)
+  public Optional<AccountGroup> 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 {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = get(db, groupUuid);
+    AccountGroup group = getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return false;
-    }
-
-    AccountGroup group = foundGroup.get();
     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 {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundParentGroup = get(db, parentGroupUuid);
+    AccountGroup parentGroup = getExistingGroup(db, parentGroupUuid);
-    if (!foundParentGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return false;
-    }
-
-    AccountGroup parentGroup = foundParentGroup.get();
     AccountGroupById.Key key = new AccountGroupById.Key(parentGroup.getId(), includedGroupUuid);
     return db.accountGroupById().get(key) != null;
   }
 
   public Stream<Account.Id> getMembers(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = get(db, groupUuid);
+    AccountGroup group = getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return Stream.empty();
-    }
-
-    AccountGroup group = foundGroup.get();
     ResultSet<AccountGroupMember> accountGroupMembers =
         db.accountGroupMembers().byGroup(group.getId());
     return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId);
   }
 
   public Stream<AccountGroup.UUID> getIncludes(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = get(db, groupUuid);
+    AccountGroup group = getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return Stream.empty();
-    }
-
-    AccountGroup group = foundGroup.get();
     ResultSet<AccountGroupById> accountGroupByIds = db.accountGroupById().byGroup(group.getId());
     return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct();
   }

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<AccountGroup> updateGroup(
+  public void updateGroup(
       ReviewDb db, AccountGroup.UUID groupUuid, Consumer<AccountGroup> groupConsumer)
-      throws OrmException, IOException {
+      throws OrmException, IOException, NoSuchGroupException {
-    Optional<AccountGroup> updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer);
+    AccountGroup updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer);
-    if (updatedGroup.isPresent()) {
+    groupCache.evict(updatedGroup);
-      groupCache.evict(updatedGroup.get());
-    }
-    return updatedGroup;
   }
 
   @VisibleForTesting
-  public Optional<AccountGroup> updateGroupInDb(
+  public AccountGroup updateGroupInDb(
       ReviewDb db, AccountGroup.UUID groupUuid, Consumer<AccountGroup> groupConsumer)
-      throws OrmException, IOException {
+      throws OrmException, IOException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
+    AccountGroup group = groups.getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      return Optional.empty();
-    }
-
-    AccountGroup group = foundGroup.get();
     groupConsumer.accept(group);
     db.accountGroups().update(ImmutableList.of(group));
-    return Optional.of(group);
+    return group;
   }
 
-  public Optional<AccountGroup> renameGroup(
+  public void renameGroup(ReviewDb db, AccountGroup.UUID groupUuid, AccountGroup.NameKey newName)
-      ReviewDb db, AccountGroup.UUID groupUuid, AccountGroup.NameKey newName)
+      throws OrmException, IOException, NameAlreadyUsedException, NoSuchGroupException {
-      throws OrmException, IOException, NameAlreadyUsedException {
+    AccountGroup group = groups.getExistingGroup(db, groupUuid);
-    Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      return Optional.empty();
-    }
-
-    AccountGroup group = foundGroup.get();
     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<Account.Id> accountIds)
-      throws OrmException, IOException {
+      throws OrmException, IOException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
+    AccountGroup group = groups.getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return;
-    }
-
-    AccountGroup group = foundGroup.get();
     AccountGroup.Id groupId = group.getId();
     Set<AccountGroupMember> newMembers = new HashSet<>();
     for (Account.Id accountId : accountIds) {
@@ -229,14 +208,8 @@ public class GroupsUpdate {
 
   public void removeGroupMembers(
       ReviewDb db, AccountGroup.UUID groupUuid, Set<Account.Id> accountIds)
-      throws OrmException, IOException {
+      throws OrmException, IOException, NoSuchGroupException {
-    Optional<AccountGroup> foundGroup = groups.get(db, groupUuid);
+    AccountGroup group = groups.getExistingGroup(db, groupUuid);
-    if (!foundGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return;
-    }
-
-    AccountGroup group = foundGroup.get();
     AccountGroup.Id groupId = group.getId();
     Set<AccountGroupMember> membersToRemove = new HashSet<>();
     for (Account.Id accountId : accountIds) {
@@ -262,14 +235,8 @@ public class GroupsUpdate {
 
   public void addIncludedGroups(
       ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> includedGroupUuids)
-      throws OrmException {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundParentGroup = groups.get(db, parentGroupUuid);
+    AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid);
-    if (!foundParentGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return;
-    }
-
-    AccountGroup parentGroup = foundParentGroup.get();
     AccountGroup.Id parentGroupId = parentGroup.getId();
     Set<AccountGroupById> newIncludedGroups = new HashSet<>();
     for (AccountGroup.UUID includedGroupUuid : includedGroupUuids) {
@@ -296,14 +263,8 @@ public class GroupsUpdate {
 
   public void deleteIncludedGroups(
       ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> includedGroupUuids)
-      throws OrmException {
+      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> foundParentGroup = groups.get(db, parentGroupUuid);
+    AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid);
-    if (!foundParentGroup.isPresent()) {
-      // TODO(aliceks): Throw an exception?
-      return;
-    }
-
-    AccountGroup parentGroup = foundParentGroup.get();
     AccountGroup.Id parentGroupId = parentGroup.getId();
     Set<AccountGroupById> includedGroupsToRemove = new HashSet<>();
     for (AccountGroup.UUID includedGroupUuid : includedGroupUuids) {

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 {
+  private boolean isMember(AccountGroup parent, GroupDescription.Basic member)
-    return groups.isIncluded(dbProvider.get(), parent.getGroupUUID(), member.getGroupUUID());
+      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")

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())
+    if (parent.getControl().canSeeMember(user.getAccountId()) && isMember(group, user)) {
-        && parent.getControl().canSeeMember(user.getAccountId())) {
       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) {

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<GroupResource, Input> {
@@ -65,13 +65,13 @@ public class PutDescription implements RestModifyView<GroupResource, Input> {
 
     String newDescription = Strings.emptyToNull(input.description);
     if (!Objects.equals(accountGroup.getDescription(), newDescription)) {
-      Optional<AccountGroup> updatedGroup =
+      AccountGroup.UUID groupUuid = resource.getGroupUUID();
-          groupsUpdateProvider
+      try {
-              .get()
+        groupsUpdateProvider
-              .updateGroup(
+            .get()
-                  db.get(), resource.getGroupUUID(), group -> group.setDescription(newDescription));
+            .updateGroup(db.get(), groupUuid, group -> group.setDescription(newDescription));
-      if (!updatedGroup.isPresent()) {
+      } catch (NoSuchGroupException e) {
-        throw new ResourceNotFoundException();
+        throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
       }
     }
 

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<GroupResource, Input> {
@@ -68,19 +68,19 @@ public class PutName implements RestModifyView<GroupResource, Input> {
       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<AccountGroup> renamedGroup =
+      groupsUpdateProvider
-          groupsUpdateProvider
+          .get()
-              .get()
+          .renameGroup(db.get(), groupUuid, new AccountGroup.NameKey(newName));
-              .renameGroup(db.get(), group.getGroupUUID(), new AccountGroup.NameKey(newName));
+    } catch (NoSuchGroupException e) {
-      return renamedGroup
+      throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
-          .map(AccountGroup::getName)
-          .orElseThrow(() -> new ResourceNotFoundException("Group not found"));
     } catch (NameAlreadyUsedException e) {
       throw new ResourceConflictException("group with name " + newName + " already exists");
     }

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<GroupResource, GroupOptionsInfo> {
@@ -59,15 +59,13 @@ public class PutOptions implements RestModifyView<GroupResource, GroupOptionsInf
     }
 
     if (accountGroup.isVisibleToAll() != input.visibleToAll) {
-      Optional<AccountGroup> updatedGroup =
+      AccountGroup.UUID groupUuid = accountGroup.getGroupUUID();
-          groupsUpdateProvider
+      try {
-              .get()
+        groupsUpdateProvider
-              .updateGroup(
+            .get()
-                  db.get(),
+            .updateGroup(db.get(), groupUuid, group -> group.setVisibleToAll(input.visibleToAll));
-                  accountGroup.getGroupUUID(),
+      } catch (NoSuchGroupException e) {
-                  group -> group.setVisibleToAll(input.visibleToAll));
+        throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
-      if (!updatedGroup.isPresent()) {
-        throw new ResourceNotFoundException();
       }
     }
 

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<GroupResource, Input> {
@@ -74,15 +74,14 @@ public class PutOwner implements RestModifyView<GroupResource, Input> {
 
     GroupDescription.Basic owner = groupsCollection.parse(input.owner);
     if (!accountGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
-      Optional<AccountGroup> updatedGroup =
+      AccountGroup.UUID groupUuid = resource.getGroupUUID();
-          groupsUpdateProvider
+      try {
-              .get()
+        groupsUpdateProvider
-              .updateGroup(
+            .get()
-                  db.get(),
+            .updateGroup(
-                  resource.getGroupUUID(),
+                db.get(), groupUuid, group -> group.setOwnerGroupUUID(owner.getGroupUUID()));
-                  group -> group.setOwnerGroupUUID(owner.getGroupUUID()));
+      } catch (NoSuchGroupException e) {
-      if (!updatedGroup.isPresent()) {
+        throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
-        throw new ResourceNotFoundException();
       }
     }
     return json.format(owner);

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);