Handle mutually exclusive star labels when change is starred or ignored

The star labels 'star' (default star) and 'ignore' (for ignoring a
change) are mutually exclusive. Trying to set one if the other is
already set failed with an internal server error. Handle this case and
return 409 Conflict instead.

In addition this adds a test to verify that setting an invalid star
label is correctly rejected with 400 Bad Request. This case was already
correctly handled in the Stars.Post REST endpoint (sets multiple stars
at once), but not in the StarredChanges.Create REST endpoint (adds a
single star).

Bug: Issue 7238
Change-Id: Id4bb5604116f4959d8ecba66146bab72437205f6
Signed-off-by: Edwin Kempin <ekempin@google.com>

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

@@ -81,6 +81,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.ReviewResult;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
+import com.google.gerrit.extensions.api.changes.StarsInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
 import com.google.gerrit.extensions.client.ChangeKind;
@@ -122,6 +123,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.PostReview;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
@@ -3465,4 +3467,51 @@ public class ChangeIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).ignore(true);
     assertThat(gApi.changes().id(changeId).ignored()).isFalse();
   }
+
+  @Test
+  public void cannotIgnoreStarredChange() throws Exception {
+    String changeId = createChange().getChangeId();
+
+    setApiUser(user);
+    gApi.accounts().self().starChange(changeId);
+    assertThat(gApi.changes().id(changeId).get().starred).isTrue();
+
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        "The labels "
+            + StarredChangesUtil.DEFAULT_LABEL
+            + " and "
+            + StarredChangesUtil.IGNORE_LABEL
+            + " are mutually exclusive. Only one of them can be set.");
+    gApi.changes().id(changeId).ignore(true);
+  }
+
+  @Test
+  public void cannotStarIgnoredChange() throws Exception {
+    String changeId = createChange().getChangeId();
+
+    setApiUser(user);
+    gApi.changes().id(changeId).ignore(true);
+    assertThat(gApi.changes().id(changeId).ignored()).isTrue();
+
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        "The labels "
+            + StarredChangesUtil.DEFAULT_LABEL
+            + " and "
+            + StarredChangesUtil.IGNORE_LABEL
+            + " are mutually exclusive. Only one of them can be set.");
+    gApi.accounts().self().starChange(changeId);
+  }
+
+  @Test
+  public void cannotSetInvalidLabel() throws Exception {
+    String changeId = createChange().getChangeId();
+
+    // label cannot contain whitespace
+    String invalidLabel = "invalid label";
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("invalid labels: " + invalidLabel);
+    gApi.accounts().self().setStars(changeId, new StarsInput(ImmutableSet.of(invalidLabel)));
+  }
 }

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

@@ -126,21 +126,28 @@ public class StarredChangesUtil {
   public static class IllegalLabelException extends IllegalArgumentException {
     private static final long serialVersionUID = 1L;
 
-    static IllegalLabelException invalidLabels(Set<String> invalidLabels) {
+    IllegalLabelException(String message) {
-      return new IllegalLabelException(
+      super(message);
-          String.format("invalid labels: %s", Joiner.on(", ").join(invalidLabels)));
     }
+  }
 
-    static IllegalLabelException mutuallyExclusiveLabels(String label1, String label2) {
+  public static class InvalidLabelsException extends IllegalLabelException {
-      return new IllegalLabelException(
+    private static final long serialVersionUID = 1L;
+
+    InvalidLabelsException(Set<String> invalidLabels) {
+      super(String.format("invalid labels: %s", Joiner.on(", ").join(invalidLabels)));
+    }
+  }
+
+  public static class MutuallyExclusiveLabelsException extends IllegalLabelException {
+    private static final long serialVersionUID = 1L;
+
+    MutuallyExclusiveLabelsException(String label1, String label2) {
+      super(
           String.format(
               "The labels %s and %s are mutually exclusive. Only one of them can be set.",
               label1, label2));
     }
-
-    IllegalLabelException(String message) {
-      super(message);
-    }
   }
 
   private static final Logger log = LoggerFactory.getLogger(StarredChangesUtil.class);
@@ -382,7 +389,7 @@ public class StarredChangesUtil {
 
   private static void checkMutuallyExclusiveLabels(Set<String> labels) {
     if (labels.containsAll(ImmutableSet.of(DEFAULT_LABEL, IGNORE_LABEL))) {
-      throw IllegalLabelException.mutuallyExclusiveLabels(DEFAULT_LABEL, IGNORE_LABEL);
+      throw new MutuallyExclusiveLabelsException(DEFAULT_LABEL, IGNORE_LABEL);
     }
   }
 
@@ -398,7 +405,7 @@ public class StarredChangesUtil {
       }
     }
     if (!invalidLabels.isEmpty()) {
-      throw IllegalLabelException.invalidLabels(invalidLabels);
+      throw new InvalidLabelsException(invalidLabels);
     }
   }
 

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

@@ -20,8 +20,10 @@ import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ChildCollection;
 import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.extensions.restapi.RestView;
@@ -30,6 +32,8 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException;
+import com.google.gerrit.server.StarredChangesUtil.MutuallyExclusiveLabelsException;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -129,7 +133,7 @@ public class StarredChanges
 
     @Override
     public Response<?> apply(AccountResource rsrc, EmptyInput in)
-        throws AuthException, OrmException, IOException {
+        throws RestApiException, OrmException, IOException {
       if (self.get() != rsrc.getUser()) {
         throw new AuthException("not allowed to add starred change");
       }
@@ -140,6 +144,10 @@ public class StarredChanges
             change.getId(),
             StarredChangesUtil.DEFAULT_LABELS,
             null);
+      } catch (MutuallyExclusiveLabelsException e) {
+        throw new ResourceConflictException(e.getMessage());
+      } catch (IllegalLabelException e) {
+        throw new BadRequestException(e.getMessage());
       } catch (OrmDuplicateKeyException e) {
         return Response.none();
       }

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

@@ -14,11 +14,13 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.webui.UiAction;
 import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.StarredChangesUtil.MutuallyExclusiveLabelsException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -55,6 +57,8 @@ public class Ignore
         stars.ignore(rsrc);
       }
       return Response.ok("");
+    } catch (MutuallyExclusiveLabelsException e) {
+      throw new ResourceConflictException(e.getMessage());
     } catch (OrmException e) {
       throw new RestApiException("failed to ignore change", e);
     }