Hashtags: Refactoring of the REST API

Sending data with a DELETE is not a common standard. Replace the PUT and
DELETE endpoints with a POST method which accepts both adds and removals.

Instead of sending hashtags as a comma-separated list, send as a set and
raise HTTP 400 Bad Request if any hashtag contains a comma.

Return results as an ordered list. Having the results in a deterministic
order makes it easier to test.

Only perform notes db update and indexing if any tags were actually added
or removed.

Examples:

  GET /a/changes/1/hashtags HTTP/1.1

  Response:
  HTTP/1.1 200 OK
  [
    "tag1",
    "tag2"
  ]

  POST /a/changes/1/hashtags HTTP/1.1
  { "add" : "tag3", "remove" : "tag2" }

  Response:
  HTTP/1.1 200 OK
  [
    "tag1",
    "tag3"
  ]

Change-Id: Idde97697ecc6bc1a51404bef67baf645d8909555

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

@@ -26,11 +26,12 @@ import com.google.inject.Singleton;
 
 import java.io.IOException;
 import java.util.Set;
+import java.util.TreeSet;
 
 @Singleton
 public class GetHashtags implements RestReadView<ChangeResource> {
   @Override
-  public Response<Set<String>> apply(ChangeResource req)
+  public Response<? extends Set<String>> apply(ChangeResource req)
       throws AuthException, OrmException, IOException, BadRequestException {
 
     ChangeControl control = req.getControl();
@@ -39,6 +40,6 @@ public class GetHashtags implements RestReadView<ChangeResource> {
     if (hashtags == null) {
       hashtags = ImmutableSet.of();
     }
-    return Response.ok(hashtags);
+    return Response.ok(new TreeSet<String>(hashtags));
   }
 }

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

@@ -53,11 +53,10 @@ public class Module extends RestApiModule {
     get(CHANGE_KIND, "in").to(IncludedIn.class);
     get(CHANGE_KIND, "hashtags").to(GetHashtags.class);
     put(CHANGE_KIND, "topic").to(PutTopic.class);
-    put(CHANGE_KIND, "hashtags").to(PutHashtags.class);
     delete(CHANGE_KIND, "topic").to(PutTopic.class);
     delete(CHANGE_KIND).to(DeleteDraftChange.class);
-    delete(CHANGE_KIND, "hashtags").to(DeleteHashtags.class);
     post(CHANGE_KIND, "abandon").to(Abandon.class);
+    post(CHANGE_KIND, "hashtags").to(PostHashtags.class);
     post(CHANGE_KIND, "publish").to(Publish.CurrentRevision.class);
     post(CHANGE_KIND, "restore").to(Restore.class);
     post(CHANGE_KIND, "revert").to(Revert.class);

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

@@ -14,17 +14,14 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.base.CharMatcher;
+import com.google.common.collect.ImmutableSet;
-import com.google.common.base.Splitter;
-import com.google.common.base.Strings;
-import com.google.common.collect.Lists;
 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.Response;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.change.DeleteHashtags.Input;
+import com.google.gerrit.server.change.PostHashtags.Input;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -37,50 +34,81 @@ import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.TreeSet;
 
 @Singleton
-public class DeleteHashtags implements RestModifyView<ChangeResource, Input> {
+public class PostHashtags implements RestModifyView<ChangeResource, Input> {
   private final ChangeUpdate.Factory updateFactory;
   private final Provider<ReviewDb> dbProvider;
   private final ChangeIndexer indexer;
 
   public static class Input {
     @DefaultInput
-    public String hashtags;
+    public Set<String> add;
+    public Set<String> remove;
   }
 
   @Inject
-  DeleteHashtags(ChangeUpdate.Factory updateFactory,
+  PostHashtags(ChangeUpdate.Factory updateFactory,
       Provider<ReviewDb> dbProvider, ChangeIndexer indexer) {
     this.updateFactory = updateFactory;
     this.dbProvider = dbProvider;
     this.indexer = indexer;
   }
 
+  private Set<String> extractTags(Set<String> input)
+      throws BadRequestException {
+    if (input == null) {
+      return ImmutableSet.of();
+    } else {
+      HashSet<String> result = new HashSet<>();
+      for (String hashtag : input) {
+        if (hashtag.contains(",")) {
+          throw new BadRequestException("Hashtags may not contain commas");
+        }
+        if (!hashtag.trim().isEmpty()) {
+          result.add(hashtag.trim());
+        }
+      }
+      return result;
+    }
+  }
+
   @Override
-  public Response<Set<String>> apply(ChangeResource req, Input input)
+  public Response<? extends Set<String>> apply(ChangeResource req, Input input)
       throws AuthException, OrmException, IOException, BadRequestException {
-    if (input == null || Strings.isNullOrEmpty(input.hashtags)) {
+    if (input == null
+        || (input.add == null && input.remove == null)) {
       throw new BadRequestException("Hashtags are required");
     }
 
     ChangeControl control = req.getControl();
-    if(!control.canEditHashtags()){
+    if (!control.canEditHashtags()) {
       throw new AuthException("Editing hashtags not permitted");
     }
     ChangeUpdate update = updateFactory.create(control);
     ChangeNotes notes = control.getNotes().load();
-    Set<String> hashtags = new HashSet<String>();
-    Set<String> oldHashtags = notes.getHashtags();
-    if (oldHashtags != null) {
-      hashtags.addAll(oldHashtags);
-    }
-    hashtags.removeAll(Lists.newArrayList(Splitter.on(CharMatcher.anyOf(",;"))
-        .trimResults().omitEmptyStrings().split(input.hashtags)));
-    update.setHashtags(hashtags);
-    update.commit();
-    indexer.index(dbProvider.get(), update.getChange());
 
-    return Response.ok(hashtags);
+    Set<String> existingHashtags = notes.getHashtags();
+    Set<String> updatedHashtags = new HashSet<>();
+    Set<String> toAdd = new HashSet<>(extractTags(input.add));
+    Set<String> toRemove = new HashSet<>(extractTags(input.remove));
+
+    if (existingHashtags != null && !existingHashtags.isEmpty()) {
+      updatedHashtags.addAll(existingHashtags);
+      toAdd.removeAll(existingHashtags);
+      toRemove.retainAll(existingHashtags);
+    }
+
+    if (toAdd.size() > 0 || toRemove.size() > 0) {
+      updatedHashtags.addAll(toAdd);
+      updatedHashtags.removeAll(toRemove);
+      update.setHashtags(updatedHashtags);
+      update.commit();
+
+      indexer.index(dbProvider.get(), update.getChange());
+    }
+
+    return Response.ok(new TreeSet<String>(updatedHashtags));
   }
 }

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

@@ -1,86 +0,0 @@
-// Copyright (C) 2012 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.change;
-
-import com.google.common.base.CharMatcher;
-import com.google.common.base.Splitter;
-import com.google.common.base.Strings;
-import com.google.common.collect.Lists;
-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.Response;
-import com.google.gerrit.extensions.restapi.RestModifyView;
-import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.change.PutHashtags.Input;
-import com.google.gerrit.server.index.ChangeIndexer;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.Singleton;
-
-import java.io.IOException;
-import java.util.HashSet;
-import java.util.Set;
-
-@Singleton
-public class PutHashtags implements RestModifyView<ChangeResource, Input> {
-  private final ChangeUpdate.Factory updateFactory;
-  private final Provider<ReviewDb> dbProvider;
-  private final ChangeIndexer indexer;
-
-  public static class Input {
-    @DefaultInput
-    public String hashtags;
-  }
-
-  @Inject
-  PutHashtags(ChangeUpdate.Factory updateFactory,
-      Provider<ReviewDb> dbProvider, ChangeIndexer indexer) {
-    this.updateFactory = updateFactory;
-    this.dbProvider = dbProvider;
-    this.indexer = indexer;
-  }
-
-  @Override
-  public Response<Set<String>> apply(ChangeResource req, Input input)
-      throws AuthException, OrmException, IOException, BadRequestException {
-    if (input == null || Strings.isNullOrEmpty(input.hashtags)) {
-      throw new BadRequestException("Hashtags are required");
-    }
-
-    ChangeControl control = req.getControl();
-    if (!control.canEditHashtags()) {
-      throw new AuthException("Editing hashtags not permitted");
-    }
-    ChangeUpdate update = updateFactory.create(control);
-    ChangeNotes notes = control.getNotes().load();
-    Set<String> oldHashtags = notes.getHashtags();
-    Set<String> hashtags = new HashSet<String>();
-    if (oldHashtags != null) {
-      hashtags.addAll(oldHashtags);
-    };
-    hashtags.addAll(Lists.newArrayList(Splitter.on(CharMatcher.anyOf(",;"))
-        .trimResults().omitEmptyStrings().split(input.hashtags)));
-    update.setHashtags(hashtags);
-    update.commit();
-    indexer.index(dbProvider.get(), update.getChange());
-
-    return Response.ok(hashtags);
-  }
-}