Support GET, PUT, DELETE on /changes/{id}/topic

The topic is now modified by REST API calls against /topic. The web UI
uses PUT to alter the topic with a JSON payload, but the server
accepts quite a few different REST forms that clients can easily send.

Change-Id: Ia5edbb232bc288acdad8b145956ad275d170629a

gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java

@@ -42,9 +42,4 @@ public interface ChangeDetailService extends RemoteJsonService {
   @SignInRequired
   void patchSetPublishDetail(PatchSet.Id key,
       AsyncCallback<PatchSetPublishDetail> callback);
-
-  @Audit
-  @SignInRequired
-  void alterTopic(Change.Id id, String topic, String message,
-      AsyncCallback<ChangeDetail> callback);
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.client.changes;
 
+import com.google.gerrit.client.rpc.NativeString;
 import com.google.gerrit.client.rpc.RestApi;
 import com.google.gwt.core.client.JavaScriptObject;
 import com.google.gwtjsonrpc.common.AsyncCallback;
@@ -25,10 +26,6 @@ import com.google.gwtjsonrpc.common.AsyncCallback;
 public class ChangeApi {
   private static final String URI = "/changes/";
 
-  protected static class Message extends JavaScriptObject {
-    public final native void setMessage(String value) /*-{ this.message = value; }-*/;
-  }
-
   /**
    * Sends a REST call to abandon a change and notify a callback. TODO: switch
    * to use the new id triplet (project~branch~change) once that data is
@@ -36,8 +33,31 @@ public class ChangeApi {
    */
   public static void abandon(int changeId, String message,
       AsyncCallback<ChangeInfo> callback) {
-    Message msg = (Message) JavaScriptObject.createObject();
-    msg.setMessage(message);
-    new RestApi(URI + changeId + "/abandon").data(msg).post(callback);
+    Input input = Input.create();
+    input.setMessage(emptyToNull(message));
+    new RestApi(URI + changeId + "/abandon").data(input).post(callback);
+  }
+
+  public static void topic(int id, String topic, String msg, AsyncCallback<String> cb) {
+    Input input = Input.create();
+    input.setTopic(emptyToNull(topic));
+    input.setMessage(emptyToNull(msg));
+    new RestApi(URI + id + "/topic").data(input).put(NativeString.unwrap(cb));
+  }
+
+  private static class Input extends JavaScriptObject {
+    final native void setTopic(String t) /*-{ this.topic = t; }-*/;
+    final native void setMessage(String m) /*-{ this.message = m; }-*/;
+
+    static Input create() {
+      return (Input) JavaScriptObject.createObject();
+    }
+
+    protected Input() {
+    }
+  }
+
+  private static String emptyToNull(String str) {
+    return str == null || str.isEmpty() ? null : str;
   }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfoBlock.java

@@ -21,6 +21,7 @@ import com.google.gerrit.client.ui.AccountLink;
 import com.google.gerrit.client.ui.CommentedActionDialog;
 import com.google.gerrit.client.ui.BranchLink;
 import com.google.gerrit.client.ui.ProjectLink;
+import com.google.gerrit.common.PageLinks;
 import com.google.gerrit.common.data.AccountInfoCache;
 import com.google.gerrit.common.data.ChangeDetail;
 import com.google.gerrit.common.data.SubmitTypeRecord;
@@ -39,6 +40,7 @@ import com.google.gwt.user.client.ui.InlineLabel;
 import com.google.gwt.user.client.ui.TextBox;
 import com.google.gwt.user.client.ui.Widget;
 import com.google.gwtexpui.clippy.client.CopyableLabel;
+import com.google.gwtjsonrpc.common.AsyncCallback;
 
 public class ChangeInfoBlock extends Composite {
   private static final int R_CHANGE_ID = 0;
@@ -185,9 +187,19 @@ public class ChangeInfoBlock extends Composite {
     @Override
     public void onSend() {
       String topic = newTopic.getText();
-      Util.DETAIL_SVC.alterTopic(change.getId(), topic,
-        getMessageText(), createCallback());
+      ChangeApi.topic(change.getId().get(), topic, getMessageText(),
+        new AsyncCallback<String>() {
+        @Override
+        public void onSuccess(String result) {
+          sent = true;
+          Gerrit.display(PageLinks.toChange(change.getId()));
+          hide();
+        }
+
+        @Override
+        public void onFailure(Throwable caught) {
+          enableButtons(true);
+        }});
     }
   }
-
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/NativeString.java

@@ -0,0 +1,41 @@
+// 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.client.rpc;
+
+import com.google.gwt.core.client.JavaScriptObject;
+import com.google.gwtjsonrpc.common.AsyncCallback;
+
+/** Wraps a String that was returned from a JSON API. */
+public final class NativeString extends JavaScriptObject {
+  public final native String asString() /*-{ return this; }-*/;
+
+  public static final AsyncCallback<NativeString>
+  unwrap(final AsyncCallback<String> cb) {
+    return new AsyncCallback<NativeString>() {
+      @Override
+      public void onSuccess(NativeString result) {
+        cb.onSuccess(result.asString());
+      }
+
+      @Override
+      public void onFailure(Throwable caught) {
+        cb.onFailure(caught);
+      }
+    };
+  }
+
+  protected NativeString() {
+  }
+}

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AlterTopicHandler.java

@@ -1,79 +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.httpd.rpc.changedetail;
-
-import com.google.gerrit.common.data.ChangeDetail;
-import com.google.gerrit.common.errors.NoSuchEntityException;
-import com.google.gerrit.httpd.rpc.Handler;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.server.changedetail.AlterTopic;
-import com.google.gerrit.server.mail.EmailException;
-import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
-import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.assistedinject.Assisted;
-
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
-
-import java.io.IOException;
-
-import javax.annotation.Nullable;
-
-class AlterTopicHandler extends Handler<ChangeDetail> {
-
-  interface Factory {
-    AlterTopicHandler create(@Assisted Change.Id changeId,
-                             @Assisted("topic") String topic,
-                             @Assisted("message") @Nullable String message);
-  }
-
-  private final Provider<AlterTopic> alterTopicProvider;
-  private final ChangeDetailFactory.Factory changeDetailFactory;
-
-  private final Change.Id changeId;
-  private final String topic;
-  @Nullable
-  private final String message;
-
-  @Inject
-  AlterTopicHandler(final Provider<AlterTopic> alterTopicProvider,
-      final ChangeDetailFactory.Factory changeDetailFactory,
-      @Assisted final Change.Id changeId,
-      @Assisted("topic") final String topic,
-      @Assisted("message") @Nullable final String message) {
-    this.alterTopicProvider = alterTopicProvider;
-    this.changeDetailFactory = changeDetailFactory;
-
-    this.changeId = changeId;
-    this.topic = topic;
-    this.message = message;
-  }
-
-  @Override
-  public ChangeDetail call() throws EmailException, IOException,
-      NoSuchChangeException, NoSuchEntityException, OrmException,
-      PatchSetInfoNotAvailableException, RepositoryNotFoundException,
-      InvalidChangeOperationException {
-    final AlterTopic alterTopic = alterTopicProvider.get();
-    alterTopic.setChangeId(changeId);
-    alterTopic.setTopic(topic);
-    alterTopic.setMessage(message);
-    alterTopic.call();
-    return changeDetailFactory.create(changeId).call();
-  }
-}

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java

@@ -30,19 +30,16 @@ class ChangeDetailServiceImpl implements ChangeDetailService {
   private final IncludedInDetailFactory.Factory includedInDetail;
   private final PatchSetDetailFactory.Factory patchSetDetail;
   private final PatchSetPublishDetailFactory.Factory patchSetPublishDetail;
-  private final AlterTopicHandler.Factory alterTopic;
 
   @Inject
   ChangeDetailServiceImpl(final ChangeDetailFactory.Factory changeDetail,
       final IncludedInDetailFactory.Factory includedInDetail,
       final PatchSetDetailFactory.Factory patchSetDetail,
-      final PatchSetPublishDetailFactory.Factory patchSetPublishDetail,
-      final AlterTopicHandler.Factory alterTopic) {
+      final PatchSetPublishDetailFactory.Factory patchSetPublishDetail) {
     this.changeDetail = changeDetail;
     this.includedInDetail = includedInDetail;
     this.patchSetDetail = patchSetDetail;
     this.patchSetPublishDetail = patchSetPublishDetail;
-    this.alterTopic = alterTopic;
   }
 
   public void changeDetail(final Change.Id id,
@@ -69,9 +66,4 @@ class ChangeDetailServiceImpl implements ChangeDetailService {
       final AsyncCallback<PatchSetPublishDetail> callback) {
     patchSetPublishDetail.create(id).to(callback);
   }
-
-  public void alterTopic(final Change.Id id, final String topic,
-      final String message, final AsyncCallback<ChangeDetail> callback) {
-    alterTopic.create(id, topic, message).to(callback);
-  }
 }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeModule.java

@@ -28,7 +28,6 @@ public class ChangeModule extends RpcServletModule {
     install(new FactoryModule() {
       @Override
       protected void configure() {
-        factory(AlterTopicHandler.Factory.class);
         factory(RestoreChangeHandler.Factory.class);
         factory(RevertChange.Factory.class);
         factory(RebaseChangeHandler.Factory.class);

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

@@ -0,0 +1,26 @@
+// 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.Strings;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gwtorm.server.OrmException;
+
+class GetTopic implements RestReadView<ChangeResource> {
+  @Override
+  public Object apply(ChangeResource rsrc) throws OrmException {
+    return Strings.nullToEmpty(rsrc.getChange().getTopic());
+  }
+}

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

@@ -30,6 +30,9 @@ public class Module extends RestApiModule {
     DynamicMap.mapOf(binder(), REVISION_KIND);
 
     get(CHANGE_KIND).to(GetChange.class);
+    get(CHANGE_KIND, "topic").to(GetTopic.class);
+    put(CHANGE_KIND, "topic").to(PutTopic.class);
+    delete(CHANGE_KIND, "topic").to(PutTopic.class);
     post(CHANGE_KIND, "abandon").to(Abandon.class);
     child(CHANGE_KIND, "reviewers").to(Reviewers.class);
 

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

@@ -0,0 +1,120 @@
+// 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.Strings;
+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.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.PutTopic.Input;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gwtorm.server.AtomicUpdate;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+import java.util.Collections;
+
+class PutTopic implements RestModifyView<ChangeResource, Input> {
+  private final Provider<ReviewDb> dbProvider;
+
+  static class Input {
+    @DefaultInput
+    String topic;
+    String message;
+  }
+
+  @Inject
+  PutTopic(Provider<ReviewDb> dbProvider) {
+    this.dbProvider = dbProvider;
+  }
+
+  @Override
+  public Class<Input> inputType() {
+    return Input.class;
+  }
+
+  @Override
+  public Object apply(ChangeResource req, Input input)
+      throws BadRequestException, AuthException,
+      ResourceConflictException, Exception {
+    if (input == null) {
+      input = new Input();
+    }
+
+    ChangeControl control = req.getControl();
+    Change change = req.getChange();
+    if (!control.canEditTopicName()) {
+      throw new AuthException("changing topic not permitted");
+    } else if (!change.getStatus().isOpen()) {
+      throw new ResourceConflictException("change is " + status(change));
+    }
+
+    ReviewDb db = dbProvider.get();
+    final String newTopicName = Strings.nullToEmpty(input.topic);
+    String oldTopicName = Strings.nullToEmpty(change.getTopic());
+    if (!oldTopicName.equals(newTopicName)) {
+      String summary;
+      if (oldTopicName.isEmpty()) {
+        summary = "Topic set to \"" + newTopicName + "\".";
+      } else if (newTopicName.isEmpty()) {
+        summary = "Topic \"" + oldTopicName + "\" removed.";
+      } else {
+        summary = String.format(
+            "Topic updated from \"%s\" to \"%s\".",
+            oldTopicName, newTopicName);
+      }
+
+      ChangeMessage cmsg = new ChangeMessage(
+          new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
+          ((IdentifiedUser) control.getCurrentUser()).getAccountId(),
+          change.currentPatchSetId());
+      StringBuilder msgBuf = new StringBuilder(summary);
+      if (!Strings.isNullOrEmpty(input.message)) {
+        msgBuf.append("\n\n");
+        msgBuf.append(input.message);
+      }
+      cmsg.setMessage(msgBuf.toString());
+
+      Change updatedChange = db.changes().atomicUpdate(change.getId(),
+        new AtomicUpdate<Change>() {
+          @Override
+          public Change update(Change change) {
+            if (change.getStatus().isOpen()) {
+              change.setTopic(Strings.emptyToNull(newTopicName));
+              return change;
+            }
+            return null;
+          }
+        });
+      if (updatedChange == null) {
+        change = db.changes().get(change.getId());
+        throw new ResourceConflictException("change is " + status(change));
+      }
+      db.changeMessages().insert(Collections.singleton(cmsg));
+    }
+    return Strings.nullToEmpty(newTopicName);
+  }
+
+  private static String status(Change change) {
+    return change != null ? change.getStatus().name().toLowerCase() : "deleted";
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AlterTopic.java

@@ -1,135 +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.changedetail;
-
-import com.google.gerrit.common.data.ReviewResult;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.ChangeMessage;
-import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.mail.EmailException;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
-import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gwtorm.server.AtomicUpdate;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-
-import java.util.Collections;
-import java.util.concurrent.Callable;
-
-import org.kohsuke.args4j.Argument;
-import org.kohsuke.args4j.Option;
-
-public class AlterTopic implements Callable<ReviewResult> {
-
-  private final ChangeControl.Factory changeControlFactory;
-  private final ReviewDb db;
-  private final IdentifiedUser currentUser;
-
-  @Argument(index = 0, required = true, multiValued = false,
-            usage = "change with topic to change")
-  private Change.Id changeId;
-
-  public void setChangeId(final Change.Id changeId) {
-    this.changeId = changeId;
-  }
-
-  @Argument(index = 1, required = true, multiValued = false, usage = "new topic")
-  private String newTopicName;
-
-  public void setTopic(final String topic) {
-    this.newTopicName = topic.trim();
-  }
-
-  @Option(name = "--message", aliases = {"-m"},
-          usage = "optional message to append to change")
-  private String message;
-
-  public void setMessage(final String message) {
-    this.message = message;
-  }
-
-  @Inject
-  AlterTopic(final ChangeControl.Factory changeControlFactory, final ReviewDb db,
-      final IdentifiedUser currentUser) {
-    this.changeControlFactory = changeControlFactory;
-    this.db = db;
-    this.currentUser = currentUser;
-
-    changeId = null;
-    newTopicName = null;
-    message = null;
-  }
-
-  @Override
-  public ReviewResult call() throws EmailException,
-      InvalidChangeOperationException, NoSuchChangeException, OrmException {
-    final ChangeControl control = changeControlFactory.validateFor(changeId);
-    final ReviewResult result = new ReviewResult();
-    result.setChangeId(changeId);
-
-    if (!control.canAddPatchSet()) {
-      throw new NoSuchChangeException(changeId);
-    }
-    if (!control.canEditTopicName()) {
-      result.addError(new ReviewResult.Error(
-          ReviewResult.Error.Type.EDIT_TOPIC_NAME_NOT_PERMITTED));
-      return result;
-    }
-
-    final Change change = db.changes().get(changeId);
-    final String oldTopicName = change.getTopic() != null ? change.getTopic() : "";
-    if (!oldTopicName.equals(newTopicName)) {
-      String summary;
-      if (oldTopicName.isEmpty()) {
-        summary = "Topic set to \"" + newTopicName + "\"";
-      } else if (newTopicName.isEmpty()) {
-        summary = "Topic \"" + oldTopicName + "\" removed";
-      } else {
-        summary = "Topic changed from \"" + oldTopicName //
-            + "\" to \"" + newTopicName + "\"";
-      }
-      final ChangeMessage cmsg = new ChangeMessage(
-          new ChangeMessage.Key(changeId, ChangeUtil.messageUUID(db)),
-          currentUser.getAccountId(), change.currentPatchSetId());
-      final StringBuilder msgBuf = new StringBuilder(summary);
-      if (message != null && message.length() > 0) {
-        msgBuf.append("\n\n");
-        msgBuf.append(message);
-      }
-      cmsg.setMessage(msgBuf.toString());
-
-      final Change updatedChange = db.changes().atomicUpdate(changeId,
-          new AtomicUpdate<Change>() {
-        @Override
-        public Change update(Change change) {
-          change.setTopic(newTopicName);
-          return change;
-        }
-      });
-
-      if (updatedChange == null) {
-        String err = "Patchset is not latest";
-        throw new InvalidChangeOperationException(err);
-      }
-      db.changeMessages().insert(Collections.singleton(cmsg));
-    }
-
-    return result;
-  }
-}