Merge "Reject invalid hashtags with 400 Bad Request"

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java

@@ -30,6 +30,7 @@ import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.changes.HashtagsInput;
 import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.testutil.TestTimeUtil;
 import org.junit.AfterClass;
 import org.junit.Before;
@@ -76,6 +77,15 @@ public class HashtagsIT extends AbstractDaemonTest {
     assertMessage(r, "Hashtag added: tag1");
   }
 
+  @Test
+  public void addInvalidHashtag() throws Exception {
+    PushOneCommit.Result r = createChange();
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("hashtags may not contain commas");
+    addHashtags(r, "invalid,hashtag");
+  }
+
   @Test
   public void addMultipleHashtags() throws Exception {
     PushOneCommit.Result r = createChange();

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

@@ -23,6 +23,18 @@ import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 public class HashtagsUtil {
+  public static class InvalidHashtagException extends Exception {
+    private static final long serialVersionUID = 1L;
+
+    static InvalidHashtagException hashtagsMayNotContainCommas() {
+      return new InvalidHashtagException("hashtags may not contain commas");
+    }
+
+    InvalidHashtagException(String message) {
+      super(message);
+    }
+  }
+
   private static final CharMatcher LEADER = CharMatcher.whitespace().or(CharMatcher.is('#'));
   private static final String PATTERN = "(?:\\s|\\A)#[\\p{L}[0-9]-_]+";
 
@@ -43,14 +55,14 @@ public class HashtagsUtil {
     return result;
   }
 
-  static Set<String> extractTags(Set<String> input) throws IllegalArgumentException {
+  static Set<String> extractTags(Set<String> input) throws InvalidHashtagException {
     if (input == null) {
       return Collections.emptySet();
     }
     HashSet<String> result = new HashSet<>();
     for (String hashtag : input) {
       if (hashtag.contains(",")) {
-        throw new IllegalArgumentException("Hashtags may not contain commas");
+        throw InvalidHashtagException.hashtagsMayNotContainCommas();
       }
       hashtag = cleanupHashtag(hashtag);
       if (!hashtag.isEmpty()) {

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

@@ -29,6 +29,7 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.change.HashtagsUtil.InvalidHashtagException;
 import com.google.gerrit.server.extensions.events.HashtagsEdited;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -99,31 +100,31 @@ public class SetHashtagsOp implements BatchUpdateOp {
     ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
     ChangeNotes notes = update.getNotes().load();
 
-    Set<String> existingHashtags = notes.getHashtags();
-    Set<String> updated = new HashSet<>();
-    toAdd = new HashSet<>(extractTags(input.add));
-    toRemove = new HashSet<>(extractTags(input.remove));
-
     try {
+      Set<String> existingHashtags = notes.getHashtags();
+      Set<String> updated = new HashSet<>();
+      toAdd = new HashSet<>(extractTags(input.add));
+      toRemove = new HashSet<>(extractTags(input.remove));
+
       for (HashtagValidationListener validator : validationListeners) {
         validator.validateHashtags(update.getChange(), toAdd, toRemove);
       }
-    } catch (ValidationException e) {
+
+      updated.addAll(existingHashtags);
+      toAdd.removeAll(existingHashtags);
+      toRemove.retainAll(existingHashtags);
+      if (updated()) {
+        updated.addAll(toAdd);
+        updated.removeAll(toRemove);
+        update.setHashtags(updated);
+        addMessage(ctx, update);
+      }
+
+      updatedHashtags = ImmutableSortedSet.copyOf(updated);
+      return true;
+    } catch (ValidationException | InvalidHashtagException e) {
       throw new BadRequestException(e.getMessage());
     }
-
-    updated.addAll(existingHashtags);
-    toAdd.removeAll(existingHashtags);
-    toRemove.retainAll(existingHashtags);
-    if (updated()) {
-      updated.addAll(toAdd);
-      updated.removeAll(toRemove);
-      update.setHashtags(updated);
-      addMessage(ctx, update);
-    }
-
-    updatedHashtags = ImmutableSortedSet.copyOf(updated);
-    return true;
   }
 
   private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {