Return 422 if entity from request body cannot be processed

If a group/project/account which is referenced in the body of a request
cannot be found or is not processable the response code should be '422
Unprocessable Entity'. At the moment we are throwing '404 Not Found' or
'400 Bad Request'. '404 Not Found' is wrong since this status code must
only be used if the resource from the URL is not found. '400 Bad
Request' would be okay, but should rather be used if the request body
is not parseable.

Change-Id: Idcfff67c81eac2e3ea19d73078d2a7ff599d7d02
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/group/GroupPropertiesIT.java

@@ -195,7 +195,7 @@ public class GroupPropertiesIT extends AbstractDaemonTest {
     in = new GroupOwnerInput();
     in.owner = "Non-Existing Group";
     r = session.put(url, in);
-    assertEquals(HttpStatus.SC_BAD_REQUEST, r.getStatusCode());
+    assertEquals(HttpStatus.SC_UNPROCESSABLE_ENTITY, r.getStatusCode());
     r.consume();
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/BadRequestHandler.java

@@ -1,57 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server;
-
-import com.google.common.collect.Lists;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-
-import java.util.List;
-
-public class BadRequestHandler {
-
-  private final List<String> errors = Lists.newLinkedList();
-  private String action;
-
-  public BadRequestHandler(final String action) {
-    this.action = action;
-  }
-
-  public void addError(final String message) {
-    errors.add(message);
-  }
-
-  public void addError(final Throwable t) {
-    errors.add(t.getMessage());
-  }
-
-  public void failOnError()
-      throws BadRequestException {
-    if (errors.isEmpty()) {
-      return;
-    }
-
-    if (errors.size() == 1) {
-      throw new BadRequestException(action + " failed: " + errors.get(0));
-    }
-
-    final StringBuilder b = new StringBuilder();
-    b.append("Multiple errors on " + action + ":");
-    for (final String error : errors) {
-      b.append("\n");
-      b.append(error);
-    }
-    throw new BadRequestException(b.toString());
-  }
-}

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

@@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.CurrentUser;
@@ -56,11 +57,35 @@ public class AccountsCollection implements
   @Override
   public AccountResource parse(TopLevelResource root, IdString id)
       throws ResourceNotFoundException, AuthException, OrmException {
-    return new AccountResource(parse(id.get()));
+    IdentifiedUser user = _parse(id.get());
+    if (user == null) {
+      throw new ResourceNotFoundException(id);
+    }
+    return new AccountResource(user);
   }
 
+  /**
+   * Parses a account ID from a request body and returns the user.
+   *
+   * @param id ID of the account, can be a string of the format
+   *        "Full Name <email@example.com>", just the email address, a full name
+   *        if it is unique, an account ID, a user name or 'self' for the
+   *        calling user
+   * @return the project
+   * @throws UnprocessableEntityException thrown if the account ID cannot be
+   *         resolved or if the account is not visible to the calling user
+   */
   public IdentifiedUser parse(String id) throws AuthException,
-      ResourceNotFoundException, OrmException {
+      UnprocessableEntityException, OrmException {
+    IdentifiedUser user = _parse(id);
+    if (user == null) {
+      throw new UnprocessableEntityException(String.format(
+          "Account Not Found: %s", id));
+    }
+    return user;
+  }
+
+  private IdentifiedUser _parse(String id) throws AuthException, OrmException {
     CurrentUser user = self.get();
 
     if (id.equals("self")) {
@@ -69,13 +94,13 @@ public class AccountsCollection implements
       } else if (user instanceof AnonymousUser) {
         throw new AuthException("Authentication required");
       } else {
-        throw new ResourceNotFoundException(id);
+        return null;
       }
     }
 
     Set<Account.Id> matches = resolver.findAll(id);
     if (matches.size() != 1) {
-      throw new ResourceNotFoundException(id);
+      return null;
     }
 
     Account.Id a = Iterables.getOnlyElement(matches);
@@ -83,7 +108,7 @@ public class AccountsCollection implements
         || user.getCapabilities().canAdministrateServer()) {
       return userFactory.create(a);
     } else {
-      throw new ResourceNotFoundException(id);
+      return null;
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java

@@ -22,7 +22,6 @@ import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
 import com.google.gerrit.common.data.LabelTypes;
 import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.common.errors.NoSuchGroupException;
@@ -31,6 +30,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.ApprovalCategory;
@@ -126,7 +126,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
   @Override
   public PostResult apply(ChangeResource rsrc, Input input)
       throws BadRequestException, ResourceNotFoundException, AuthException,
-      OrmException, EmailException {
+      UnprocessableEntityException, OrmException, EmailException {
     if (input.reviewer == null) {
       throw new BadRequestException("missing reviewer field");
     }
@@ -139,10 +139,6 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
       }
     } catch (NoSuchChangeException e) {
       throw new ResourceNotFoundException(e.getMessage());
-    } catch (NoSuchGroupException e) {
-      throw new ResourceNotFoundException(e.getMessage());
-    } catch (NoSuchProjectException e) {
-      throw new ResourceNotFoundException(e.getMessage());
     }
   }
 
@@ -154,13 +150,10 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
   }
 
   private PostResult putGroup(ChangeResource rsrc, Input input)
-      throws ResourceNotFoundException, AuthException, NoSuchGroupException,
+      throws ResourceNotFoundException, AuthException, BadRequestException,
-      NoSuchProjectException, OrmException, NoSuchChangeException,
+      UnprocessableEntityException, OrmException, NoSuchChangeException,
       EmailException {
-    GroupDescription.Basic group = groupsCollection.get().parse(input.reviewer);
+    GroupDescription.Basic group = groupsCollection.get().parseInternal(input.reviewer);
-    if (GroupDescriptions.toAccountGroup(group) == null) {
-      throw new ResourceNotFoundException(input.reviewer);
-    }
     PostResult result = new PostResult();
     if (!isLegalReviewerGroup(group.getGroupUUID())) {
       result.error = MessageFormat.format(
@@ -170,8 +163,15 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
 
     Set<IdentifiedUser> reviewers = Sets.newLinkedHashSet();
     ChangeControl control = rsrc.getControl();
-    Set<Account> members = groupMembersFactory.create(control.getCurrentUser())
+    Set<Account> members;
-        .listAccounts(group.getGroupUUID(), control.getProject().getNameKey());
+    try {
+      members = groupMembersFactory.create(control.getCurrentUser()).listAccounts(
+              group.getGroupUUID(), control.getProject().getNameKey());
+    } catch (NoSuchGroupException e) {
+      throw new UnprocessableEntityException(e.getMessage());
+    } catch (NoSuchProjectException e) {
+      throw new BadRequestException(e.getMessage());
+    }
 
     // if maxAllowed is set to 0, it is allowed to add any number of
     // reviewers

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

@@ -19,19 +19,17 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 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.BadRequestException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuidAudit;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.BadRequestHandler;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.account.GroupIncludeCache;
@@ -83,7 +81,7 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
   @Override
   public List<GroupInfo> apply(GroupResource resource, Input input)
       throws MethodNotAllowedException, AuthException, BadRequestException,
-      OrmException {
+      UnprocessableEntityException, OrmException {
     AccountGroup group = resource.toAccountGroup();
     if (group == null) {
       throw new MethodNotAllowedException();
@@ -93,19 +91,11 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
     GroupControl control = resource.getControl();
     Map<AccountGroup.UUID, AccountGroupIncludeByUuid> newIncludedGroups = Maps.newHashMap();
     List<AccountGroupIncludeByUuidAudit> newIncludedGroupsAudits = Lists.newLinkedList();
-    BadRequestHandler badRequest = new BadRequestHandler("adding groups");
     List<GroupInfo> result = Lists.newLinkedList();
     Account.Id me = ((IdentifiedUser) control.getCurrentUser()).getAccountId();
 
     for (String includedGroup : input.groups) {
-      GroupDescription.Basic d;
+      GroupDescription.Basic d = groupsCollection.get().parse(includedGroup);
-      try {
-        d = groupsCollection.get().parse(includedGroup);
-      } catch (ResourceNotFoundException e) {
-        badRequest.addError(new NoSuchGroupException(includedGroup));
-        continue;
-      }
-
       if (!control.canAddGroup(d.getGroupUUID())) {
         throw new AuthException(String.format("Cannot add group: %s",
             d.getName()));
@@ -125,8 +115,6 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
       result.add(json.format(d));
     }
 
-    badRequest.failOnError();
-
     if (!newIncludedGroups.isEmpty()) {
       db.accountGroupIncludesByUuidAudit().insert(newIncludedGroupsAudits);
       db.accountGroupIncludesByUuid().insert(newIncludedGroups.values());
@@ -154,7 +142,7 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
     @Override
     public GroupInfo apply(GroupResource resource, Input input)
         throws MethodNotAllowedException, AuthException, BadRequestException,
-        OrmException {
+        UnprocessableEntityException, OrmException {
       AddIncludedGroups.Input in = new AddIncludedGroups.Input();
       in.groups = ImmutableList.of(id);
       List<GroupInfo> list = put.get().apply(resource, in);

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

@@ -17,21 +17,17 @@ package com.google.gerrit.server.group;
 import com.google.common.base.Strings;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
-import com.google.gerrit.common.errors.InactiveAccountException;
-import com.google.gerrit.common.errors.NoSuchAccountException;
 import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.client.AuthType;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.BadRequestHandler;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountException;
@@ -95,8 +91,8 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
 
   @Override
   public List<AccountInfo> apply(GroupResource resource, Input input)
-      throws AuthException, MethodNotAllowedException, BadRequestException,
+      throws AuthException, MethodNotAllowedException,
-      OrmException {
+      UnprocessableEntityException, OrmException {
     AccountGroup internalGroup = resource.toAccountGroup();
     if (internalGroup == null) {
       throw new MethodNotAllowedException();
@@ -106,20 +102,14 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
     GroupControl control = resource.getControl();
     Map<Account.Id, AccountGroupMember> newAccountGroupMembers = Maps.newHashMap();
     List<AccountGroupMemberAudit> newAccountGroupMemberAudits = Lists.newLinkedList();
-    BadRequestHandler badRequest = new BadRequestHandler("adding new group members");
     List<AccountInfo> result = Lists.newLinkedList();
     Account.Id me = ((IdentifiedUser) control.getCurrentUser()).getAccountId();
 
     for (String nameOrEmail : input.members) {
       Account a = findAccount(nameOrEmail);
-      if (a == null) {
-        badRequest.addError(new NoSuchAccountException(nameOrEmail));
-        continue;
-      }
-
       if (!a.isActive()) {
-        badRequest.addError(new InactiveAccountException(a.getFullName()));
+        throw new UnprocessableEntityException(String.format(
-        continue;
+            "Account Inactive: %s", nameOrEmail));
       }
 
       if (!control.canAddMember(a.getId())) {
@@ -139,8 +129,6 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
       result.add(AccountInfo.parse(a, true));
     }
 
-    badRequest.failOnError();
-
     db.accountGroupMembersAudit().insert(newAccountGroupMemberAudits);
     db.accountGroupMembers().insert(newAccountGroupMembers.values());
     for (AccountGroupMember m : newAccountGroupMembers.values()) {
@@ -150,13 +138,13 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
     return result;
   }
 
-  private Account findAccount(String nameOrEmail) throws OrmException {
+  private Account findAccount(String nameOrEmail) throws AuthException,
+      UnprocessableEntityException, OrmException {
     try {
       return accounts.get().parse(nameOrEmail).getAccount();
-    } catch (AuthException e) {
+    } catch (UnprocessableEntityException e) {
-      return null;
+      // might be because the account does not exist or because the account is
-    } catch (ResourceNotFoundException e) {
+      // not visible
-      // might be because the account does not exist or because the account is not visible
       switch (authType) {
         case HTTP_LDAP:
         case CLIENT_SSL_CERT_LDAP:
@@ -168,7 +156,7 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
           break;
         default:
       }
-      return null;
+      throw e;
     }
   }
 
@@ -201,8 +189,8 @@ class AddMembers implements RestModifyView<GroupResource, Input> {
 
     @Override
     public Object apply(GroupResource resource, PutMember.Input input)
-        throws AuthException, MethodNotAllowedException, BadRequestException,
+        throws AuthException, MethodNotAllowedException,
-        OrmException {
+        UnprocessableEntityException, OrmException {
       AddMembers.Input in = new AddMembers.Input();
       in._oneMember = id;
       List<AccountInfo> list = put.get().apply(resource, in);

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

@@ -24,9 +24,9 @@ import com.google.gerrit.common.errors.PermissionDeniedException;
 import com.google.gerrit.extensions.annotations.RequiresCapability;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
-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;
 import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -78,7 +78,7 @@ class CreateGroup implements RestModifyView<TopLevelResource, Input> {
 
   @Override
   public GroupInfo apply(TopLevelResource resource, Input input)
-      throws AuthException, BadRequestException,
+      throws AuthException, BadRequestException, UnprocessableEntityException,
       NameAlreadyUsedException, OrmException {
     if (input == null) {
       input = new Input();
@@ -105,18 +105,11 @@ class CreateGroup implements RestModifyView<TopLevelResource, Input> {
     return json.format(GroupDescriptions.forAccountGroup(group));
   }
 
-  private AccountGroup.Id owner(Input input) throws BadRequestException {
+  private AccountGroup.Id owner(Input input)
+      throws UnprocessableEntityException {
     if (input.ownerId != null) {
-      try {
+      GroupDescription.Basic d = groups.parseInternal(Url.decode(input.ownerId));
-        GroupDescription.Basic d = groups.parse(Url.decode(input.ownerId));
+      return GroupDescriptions.toAccountGroup(d).getId();
-        AccountGroup owner = GroupDescriptions.toAccountGroup(d);
-        if (owner == null) {
-          throw new BadRequestException("ownerId must be internal group");
-        }
-        return owner.getId();
-      } catch (ResourceNotFoundException e) {
-        throw new BadRequestException("ownerId cannot be resolved");
-      }
     }
     return null;
   }

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

@@ -18,19 +18,17 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 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.BadRequestException;
 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;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuidAudit;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.BadRequestHandler;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupControl;
@@ -62,7 +60,7 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
   @Override
   public Object apply(GroupResource resource, Input input)
       throws MethodNotAllowedException, AuthException, BadRequestException,
-      OrmException {
+      UnprocessableEntityException, OrmException {
     AccountGroup internalGroup = resource.toAccountGroup();
     if (internalGroup == null) {
       throw new MethodNotAllowedException();
@@ -72,17 +70,9 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
     final GroupControl control = resource.getControl();
     final Map<AccountGroup.UUID, AccountGroupIncludeByUuid> includedGroups = getIncludedGroups(internalGroup.getId());
     final List<AccountGroupIncludeByUuid> toRemove = Lists.newLinkedList();
-    final BadRequestHandler badRequest = new BadRequestHandler("removing included groups");
 
     for (final String includedGroup : input.groups) {
-      GroupDescription.Basic d;
+      GroupDescription.Basic d = groupsCollection.get().parse(includedGroup);
-      try {
-        d = groupsCollection.get().parse(includedGroup);
-      } catch (ResourceNotFoundException e) {
-        badRequest.addError(new NoSuchGroupException(includedGroup));
-        continue;
-      }
-
       if (!control.canRemoveGroup(d.getGroupUUID())) {
         throw new AuthException(String.format("Cannot delete group: %s",
             d.getName()));
@@ -93,7 +83,6 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
         toRemove.add(g);
       }
     }
-    badRequest.failOnError();
 
     if (!toRemove.isEmpty()) {
       writeAudits(toRemove);
@@ -155,7 +144,7 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
     @Override
     public Object apply(IncludedGroupResource resource, Input input)
         throws MethodNotAllowedException, AuthException, BadRequestException,
-        OrmException {
+        UnprocessableEntityException, OrmException {
       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

@@ -16,20 +16,17 @@ package com.google.gerrit.server.group;
 
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
-import com.google.gerrit.common.errors.NoSuchAccountException;
 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.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;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.BadRequestHandler;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
@@ -60,8 +57,8 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
 
   @Override
   public Object apply(GroupResource resource, Input input)
-      throws AuthException, MethodNotAllowedException, BadRequestException,
+      throws AuthException, MethodNotAllowedException,
-      OrmException {
+      UnprocessableEntityException, OrmException {
     AccountGroup internalGroup = resource.toAccountGroup();
     if (internalGroup == null) {
       throw new MethodNotAllowedException();
@@ -71,14 +68,9 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
     final GroupControl control = resource.getControl();
     final Map<Account.Id, AccountGroupMember> members = getMembers(internalGroup.getId());
     final List<AccountGroupMember> toRemove = Lists.newLinkedList();
-    final BadRequestHandler badRequest = new BadRequestHandler("removing group members");
 
     for (final String nameOrEmail : input.members) {
-      Account a = parse(nameOrEmail);
+      Account a = accounts.get().parse(nameOrEmail).getAccount();
-      if (a == null) {
-        badRequest.addError(new NoSuchAccountException(nameOrEmail));
-        continue;
-      }
 
       if (!control.canRemoveMember(a.getId())) {
         throw new AuthException("Cannot delete member: " + a.getFullName());
@@ -90,8 +82,6 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
       }
     }
 
-    badRequest.failOnError();
-
     writeAudits(toRemove);
     db.accountGroupMembers().delete(toRemove);
     for (final AccountGroupMember m : toRemove) {
@@ -101,16 +91,6 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
     return Response.none();
   }
 
-  private Account parse(String nameOrEmail) throws OrmException {
-    try {
-      return accounts.get().parse(nameOrEmail).getAccount();
-    } catch (AuthException e) {
-      return null;
-    } catch (ResourceNotFoundException e) {
-      return null;
-    }
-  }
-
   private void writeAudits(final List<AccountGroupMember> toBeRemoved)
       throws OrmException {
     final Account.Id me = ((IdentifiedUser) self.get()).getAccountId();
@@ -161,8 +141,8 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
 
     @Override
     public Object apply(MemberResource resource, Input input)
-        throws AuthException, MethodNotAllowedException, BadRequestException,
+        throws AuthException, MethodNotAllowedException,
-        OrmException, NoSuchGroupException {
+        UnprocessableEntityException, OrmException, NoSuchGroupException {
       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/GroupsCollection.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.group;
 
 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.extensions.registration.DynamicMap;
@@ -25,6 +26,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.CurrentUser;
@@ -75,7 +77,7 @@ public class GroupsCollection implements
 
   @Override
   public GroupResource parse(TopLevelResource parent, IdString id)
-      throws ResourceNotFoundException, Exception {
+      throws AuthException, ResourceNotFoundException {
     final CurrentUser user = self.get();
     if (user instanceof AnonymousUser) {
       throw new AuthException("Authentication required");
@@ -83,14 +85,58 @@ public class GroupsCollection implements
       throw new ResourceNotFoundException(id);
     }
 
-    GroupControl ctl = groupControlFactory.controlFor(parse(id.get()));
+    GroupDescription.Basic group = _parse(id.get());
+    if (group == null) {
+      throw new ResourceNotFoundException(id.get());
+    }
+    GroupControl ctl = groupControlFactory.controlFor(group);
     if (!ctl.isVisible()) {
       throw new ResourceNotFoundException(id);
     }
     return new GroupResource(ctl);
   }
 
-  public GroupDescription.Basic parse(String id) throws ResourceNotFoundException {
+  /**
+   * Parses a group ID from a request body and returns the group.
+   *
+   * @param id ID of the group, can be a group UUID, a group name or a legacy
+   *        group ID
+   * @return the group
+   * @throws UnprocessableEntityException thrown if the group ID cannot be
+   *         resolved or if the group is not visible to the calling user
+   */
+  public GroupDescription.Basic parse(String id)
+      throws UnprocessableEntityException {
+    GroupDescription.Basic group = _parse(id);
+    if (group == null || !groupControlFactory.controlFor(group).isVisible()) {
+      throw new UnprocessableEntityException(String.format(
+          "Group Not Found: %s", id));
+    }
+    return group;
+  }
+
+  /**
+   * Parses a group ID from a request body and returns the group if it is a
+   * Gerrit internal group.
+   *
+   * @param id ID of the group, can be a group UUID, a group name or a legacy
+   *        group ID
+   * @return the group
+   * @throws UnprocessableEntityException thrown if the group ID cannot be
+   *         resolved, if the group is not visible to the calling user or if
+   *         it's an external group
+   */
+  public GroupDescription.Basic parseInternal(String id)
+      throws UnprocessableEntityException {
+    GroupDescription.Basic group = parse(id);
+    if (GroupDescriptions.toAccountGroup(group) == null) {
+      throw new UnprocessableEntityException(String.format(
+          "External Group Not Allowed: %s", id));
+    }
+    return group;
+  }
+
+  private GroupDescription.Basic _parse(String id) {
     AccountGroup.UUID uuid = new AccountGroup.UUID(id);
     if (groupBackend.handles(uuid)) {
       GroupDescription.Basic d = groupBackend.get(uuid);
@@ -118,7 +164,7 @@ public class GroupsCollection implements
       }
     }
 
-    throw new ResourceNotFoundException(id);
+    return null;
   }
 
   @SuppressWarnings("unchecked")

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

@@ -17,10 +17,12 @@ package com.google.gerrit.server.group;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AcceptsCreate;
+import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ChildCollection;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -57,13 +59,14 @@ public class IncludedGroupsCollection implements
 
   @Override
   public IncludedGroupResource parse(GroupResource resource, IdString id)
-      throws ResourceNotFoundException, OrmException {
+      throws AuthException, ResourceNotFoundException, OrmException {
     AccountGroup parent = resource.toAccountGroup();
     if (parent == null) {
       throw new ResourceNotFoundException(id);
     }
 
-    GroupDescription.Basic member = groupsCollection.get().parse(id.get());
+    GroupDescription.Basic member =
+        groupsCollection.get().parse(TopLevelResource.INSTANCE, id).getGroup();
     if (isMember(parent, member)
         && resource.getControl().canSeeGroup(member.getGroupUUID())) {
       return new IncludedGroupResource(resource, member);

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

@@ -21,11 +21,13 @@ import com.google.gerrit.extensions.restapi.ChildCollection;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountsCollection;
 import com.google.gerrit.server.group.AddMembers.PutMember;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -59,12 +61,12 @@ public class MembersCollection implements
 
   @Override
   public MemberResource parse(GroupResource parent, IdString id)
-      throws ResourceNotFoundException, Exception {
+      throws AuthException, ResourceNotFoundException, OrmException {
     if (parent.toAccountGroup() == null) {
       throw new ResourceNotFoundException(id);
     }
 
-    IdentifiedUser user = accounts.get().parse(id.get());
+    IdentifiedUser user = accounts.get().parse(TopLevelResource.INSTANCE, id).getUser();
     AccountGroupMember.Key key =
         new AccountGroupMember.Key(user.getAccountId(), parent.toAccountGroup().getId());
     if (db.get().accountGroupMembers().get(key) != null

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

@@ -16,17 +16,16 @@ 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.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.group.GroupJson.GroupInfo;
 import com.google.gerrit.server.group.PutOwner.Input;
 import com.google.gwtorm.server.OrmException;
@@ -43,24 +42,23 @@ public class PutOwner implements RestModifyView<GroupResource, Input> {
 
   private final Provider<GroupsCollection> groupsCollection;
   private final GroupCache groupCache;
-  private final GroupControl.Factory controlFactory;
   private final ReviewDb db;
   private final GroupJson json;
 
   @Inject
   PutOwner(Provider<GroupsCollection> groupsCollection, GroupCache groupCache,
-      GroupControl.Factory controlFactory, ReviewDb db, GroupJson json) {
+      ReviewDb db, GroupJson json) {
     this.groupsCollection = groupsCollection;
     this.groupCache = groupCache;
-    this.controlFactory = controlFactory;
     this.db = db;
     this.json = json;
   }
 
   @Override
   public GroupInfo apply(GroupResource resource, Input input)
-      throws MethodNotAllowedException, AuthException, BadRequestException,
+      throws ResourceNotFoundException, MethodNotAllowedException,
-      ResourceNotFoundException, OrmException {
+      AuthException, BadRequestException, UnprocessableEntityException,
+      OrmException {
     AccountGroup group = resource.toAccountGroup();
     if (group == null) {
       throw new MethodNotAllowedException();
@@ -72,28 +70,17 @@ public class PutOwner implements RestModifyView<GroupResource, Input> {
       throw new BadRequestException("owner is required");
     }
 
-    GroupDescription.Basic owner;
+    group = db.accountGroups().get(group.getId());
-    try {
+    if (group == null) {
-      owner = groupsCollection.get().parse(input.owner);
+      throw new ResourceNotFoundException();
-    } catch (ResourceNotFoundException e) {
-      throw new BadRequestException(String.format("No such group: %s", input.owner));
     }
 
-    try {
+    GroupDescription.Basic owner = groupsCollection.get().parse(input.owner);
-      GroupControl c = controlFactory.validateFor(owner.getGroupUUID());
+    if (!group.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
-      group = db.accountGroups().get(group.getId());
+      group.setOwnerGroupUUID(owner.getGroupUUID());
-      if (group == null) {
+      db.accountGroups().update(Collections.singleton(group));
-        throw new ResourceNotFoundException();
+      groupCache.evict(group);
-      }
-
-      if (!group.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
-        group.setOwnerGroupUUID(owner.getGroupUUID());
-        db.accountGroups().update(Collections.singleton(group));
-        groupCache.evict(group);
-      }
-      return json.format(c.getGroup());
-    } catch (NoSuchGroupException e) {
-      throw new BadRequestException(String.format("No such group: %s", input.owner));
     }
+    return json.format(owner);
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java

@@ -21,7 +21,6 @@ import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.errors.ProjectCreationFailedException;
 import com.google.gerrit.extensions.annotations.RequiresCapability;
 import com.google.gerrit.extensions.restapi.BadRequestException;
-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.TopLevelResource;
@@ -91,13 +90,7 @@ class CreateProject implements RestModifyView<TopLevelResource, Input> {
     final CreateProjectArgs args = new CreateProjectArgs();
     args.setProjectName(name);
     if (!Strings.isNullOrEmpty(input.parent)) {
-      try {
+      args.newParent = projectsCollection.get().parse(input.parent).getControl();
-        args.newParent =
-            projectsCollection.get().parse(input.parent).getControl();
-      } catch (ResourceNotFoundException e) {
-        throw new UnprocessableEntityException(String.format(
-            "parent project \"%s\" not found", input.parent));
-      }
     }
     args.createEmptyCommit = input.createEmptyCommit;
     args.permissionsOnly = input.permissionsOnly;
@@ -109,12 +102,7 @@ class CreateProject implements RestModifyView<TopLevelResource, Input> {
       List<AccountGroup.UUID> ownerIds =
           Lists.newArrayListWithCapacity(input.owners.size());
       for (String owner : input.owners) {
-        try {
+        ownerIds.add(groupsCollection.get().parse(owner).getGroupUUID());
-          ownerIds.add(groupsCollection.get().parse(owner).getGroupUUID());
-        } catch (ResourceNotFoundException e) {
-          throw new UnprocessableEntityException(String.format(
-              "group \"%s\" not found", owner));
-        }
       }
       args.ownerIds = ownerIds;
     }

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectsCollection.java

@@ -21,6 +21,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.OutputFormat;
@@ -56,21 +57,41 @@ public class ProjectsCollection implements
   @Override
   public ProjectResource parse(TopLevelResource parent, IdString id)
       throws ResourceNotFoundException {
-    return parse(id.get());
+    ProjectResource rsrc = _parse(id.get());
+    if (rsrc == null) {
+      throw new ResourceNotFoundException(id);
+    }
+    return rsrc;
   }
 
-  public ProjectResource parse(String id)
+  /**
-      throws ResourceNotFoundException {
+   * Parses a project ID from a request body and returns the project.
+   *
+   * @param id ID of the project, can be a project name
+   * @return the project
+   * @throws UnprocessableEntityException thrown if the project ID cannot be
+   *         resolved or if the project is not visible to the calling user
+   */
+  public ProjectResource parse(String id) throws UnprocessableEntityException {
+    ProjectResource rsrc = _parse(id);
+    if (rsrc == null) {
+      throw new UnprocessableEntityException(String.format(
+          "Project Not Found: %s", id));
+    }
+    return rsrc;
+  }
+
+  private ProjectResource _parse(String id) {
     ProjectControl ctl;
     try {
       ctl = controlFactory.controlFor(
           new Project.NameKey(id),
           user.get());
     } catch (NoSuchProjectException e) {
-      throw new ResourceNotFoundException(id);
+      return null;
     }
     if (!ctl.isVisible() && !ctl.isOwner()) {
-      throw new ResourceNotFoundException(id);
+      return null;
     }
     return new ProjectResource(ctl);
   }