Use HTTP 201, 204 response codes in REST API
When creating a new entity on the server respond HTTP 201 Created
with the JSON representation of the entity in the body.
When deleting an existing entity, respond with HTTP 204 No Content,
telling the client there is no more content behind the resources
that was just deleted. This is a hint that a future GET is likely
to return 404 Not Found.
Change-Id: Ia7b3964267fcd55b4abcc49dcd6ba4c61f61fd5d
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java
new file mode 100644
index 0000000..b01146b
--- /dev/null
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java
@@ -0,0 +1,93 @@
+// 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.extensions.restapi;
+
+/** Special return value to mean specific HTTP status codes in a REST API. */
+public abstract class Response<T> {
+ @SuppressWarnings({"rawtypes"})
+ private static final Response NONE = new None();
+
+ /** HTTP 200 OK: pointless wrapper for type safety. */
+ public static <T> Response<T> ok(T value) {
+ return new Impl<T>(200, value);
+ }
+
+ /** HTTP 201 Created: typically used when a new resource is made. */
+ public static <T> Response<T> created(T value) {
+ return new Impl<T>(201, value);
+ }
+
+ /** HTTP 204 No Content: typically used when the resource is deleted. */
+ @SuppressWarnings("unchecked")
+ public static <T> Response<T> none() {
+ return NONE;
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ public static <T> T unwrap(T obj) {
+ while (obj instanceof Response) {
+ obj = (T) ((Response) obj).value();
+ }
+ return obj;
+ }
+
+ public abstract int statusCode();
+ public abstract T value();
+ public abstract String toString();
+
+ private static final class Impl<T> extends Response<T> {
+ private final int statusCode;
+ private final T value;
+
+ private Impl(int sc, T val) {
+ statusCode = sc;
+ value = val;
+ }
+
+ @Override
+ public int statusCode() {
+ return statusCode;
+ }
+
+ @Override
+ public T value() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return "[" + statusCode() + "] " + value();
+ }
+ }
+
+ private static final class None extends Response<Object> {
+ private None() {
+ }
+
+ @Override
+ public int statusCode() {
+ return 204;
+ }
+
+ public Object value() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String toString() {
+ return "[204 No Content] None";
+ }
+ }
+}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/NativeString.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/NativeString.java
index 4ea7121..573c5e7 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/NativeString.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/NativeString.java
@@ -33,7 +33,7 @@
return new AsyncCallback<NativeString>() {
@Override
public void onSuccess(NativeString result) {
- cb.onSuccess(result.asString());
+ cb.onSuccess(result != null ? result.asString() : null);
}
@Override
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java
index 4cbeb9a..d3c7000 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java
@@ -40,7 +40,8 @@
/** Makes a REST API call to the server. */
public class RestApi {
private static final int SC_UNAVAILABLE = 2;
- private static final int SC_TRANSPORT = 3;
+ private static final int SC_BAD_TRANSPORT = 3;
+ private static final int SC_BAD_RESPONSE = 4;
private static final String JSON_TYPE = "application/json";
private static final String TEXT_TYPE = "text/plain";
@@ -111,7 +112,33 @@
@Override
public void onResponseReceived(Request req, Response res) {
int status = res.getStatusCode();
- if (status != 200) {
+ if (status == Response.SC_NO_CONTENT) {
+ cb.onSuccess(null);
+ RpcStatus.INSTANCE.onRpcComplete();
+
+ } else if (200 <= status && status < 300) {
+ if (!isJsonBody(res)) {
+ RpcStatus.INSTANCE.onRpcComplete();
+ cb.onFailure(new StatusCodeException(SC_BAD_RESPONSE, "Expected "
+ + JSON_TYPE + "; received Content-Type: "
+ + res.getHeader("Content-Type")));
+ return;
+ }
+
+ T data;
+ try {
+ data = cast(parseJson(res));
+ } catch (JSONException e) {
+ RpcStatus.INSTANCE.onRpcComplete();
+ cb.onFailure(new StatusCodeException(SC_BAD_RESPONSE,
+ "Invalid JSON: " + e.getMessage()));
+ return;
+ }
+
+ cb.onSuccess(data);
+ RpcStatus.INSTANCE.onRpcComplete();
+
+ } else {
String msg;
if (isTextBody(res)) {
msg = res.getText().trim();
@@ -133,29 +160,7 @@
RpcStatus.INSTANCE.onRpcComplete();
cb.onFailure(new StatusCodeException(status, msg));
- return;
}
-
- if (!isJsonBody(res)) {
- RpcStatus.INSTANCE.onRpcComplete();
- cb.onFailure(new StatusCodeException(200, "Expected "
- + JSON_TYPE + "; received Content-Type: "
- + res.getHeader("Content-Type")));
- return;
- }
-
- T data;
- try {
- data = cast(parseJson(res));
- } catch (JSONException e) {
- RpcStatus.INSTANCE.onRpcComplete();
- cb.onFailure(new StatusCodeException(200,
- "Invalid JSON: " + e.getMessage()));
- return;
- }
-
- cb.onSuccess(data);
- RpcStatus.INSTANCE.onRpcComplete();
}
@Override
@@ -166,7 +171,7 @@
SC_UNAVAILABLE,
RpcConstants.C.errorServerUnavailable()));
} else {
- cb.onFailure(new StatusCodeException(SC_TRANSPORT, err.getMessage()));
+ cb.onFailure(new StatusCodeException(SC_BAD_TRANSPORT, err.getMessage()));
}
}
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index ba168a0..8b9fb73 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -43,6 +43,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.PutInput;
@@ -237,11 +238,20 @@
throw new ResourceNotFoundException();
}
+ if (result instanceof Response) {
+ @SuppressWarnings("rawtypes")
+ Response r = (Response) result;
+ status = r.statusCode();
+ }
res.setStatus(status);
- if (result instanceof BinaryResult) {
- replyBinaryResult(req, res, (BinaryResult) result);
- } else {
- replyJson(req, res, config, result);
+
+ if (result != Response.none()) {
+ result = Response.unwrap(result);
+ if (result instanceof BinaryResult) {
+ replyBinaryResult(req, res, (BinaryResult) result);
+ } else {
+ replyJson(req, res, config, result);
+ }
}
} catch (AuthException e) {
replyError(res, SC_FORBIDDEN, e.getMessage());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java
index 74a5af9..cb56627 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java
@@ -18,6 +18,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -26,6 +27,7 @@
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.change.PutDraft.Input;
import com.google.gerrit.server.util.Url;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -45,8 +47,8 @@
}
@Override
- public Object apply(RevisionResource rsrc, Input in) throws AuthException,
- BadRequestException, ResourceConflictException, Exception {
+ public Response<GetDraft.Comment> apply(RevisionResource rsrc, Input in)
+ throws AuthException, BadRequestException, ResourceConflictException, OrmException {
if (Strings.isNullOrEmpty(in.path)) {
throw new BadRequestException("path must be non-empty");
} else if (in.message == null || in.message.trim().isEmpty()) {
@@ -66,6 +68,6 @@
c.setSide(in.side == GetDraft.Side.PARENT ? (short) 0 : (short) 1);
c.setMessage(in.message.trim());
db.get().patchComments().insert(Collections.singleton(c));
- return new GetDraft.Comment(c);
+ return Response.created(new GetDraft.Comment(c));
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java
index af9b846..6573204 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.change;
+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.DeleteDraft.Input;
@@ -42,6 +43,6 @@
@Override
public Object apply(DraftResource rsrc, Input input) throws OrmException {
db.get().patchComments().delete(Collections.singleton(rsrc.getComment()));
- return new Object();
+ return Response.none();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
index c58f78d..9921388 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Change;
@@ -71,7 +72,7 @@
} finally {
db.rollback();
}
- return new Object();
+ return Response.none();
}
private Iterable<PatchSetApproval> approvals(ReviewDb db,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
index 16c4e89..3747f9f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
@@ -18,6 +18,7 @@
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.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Change;
@@ -102,6 +103,8 @@
});
db.changeMessages().insert(Collections.singleton(cmsg));
}
- return Strings.nullToEmpty(newTopicName);
+ return Strings.isNullOrEmpty(newTopicName)
+ ? Response.none()
+ : newTopicName;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/InstallPlugin.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/InstallPlugin.java
index 8f12806..cd64dfd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/InstallPlugin.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/InstallPlugin.java
@@ -16,11 +16,10 @@
import com.google.gerrit.common.data.GlobalCapability;
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.DefaultInput;
import com.google.gerrit.extensions.restapi.PutInput;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.plugins.InstallPlugin.Input;
@@ -44,10 +43,12 @@
private final PluginLoader loader;
private final String name;
+ private final boolean created;
- InstallPlugin(PluginLoader loader, String name) {
+ InstallPlugin(PluginLoader loader, String name, boolean created) {
this.loader = loader;
this.name = name;
+ this.created = created;
}
@Override
@@ -56,9 +57,8 @@
}
@Override
- public Object apply(TopLevelResource resource, Input input)
- throws AuthException, BadRequestException, ResourceConflictException,
- Exception {
+ public Response<ListPlugins.PluginInfo> apply(TopLevelResource resource,
+ Input input) throws BadRequestException, IOException {
try {
InputStream in;
if (input.raw != null) {
@@ -91,7 +91,9 @@
}
throw new BadRequestException(buf.toString());
}
- return new ListPlugins.PluginInfo(loader.get(name));
+
+ ListPlugins.PluginInfo info = new ListPlugins.PluginInfo(loader.get(name));
+ return created ? Response.created(info) : Response.ok(info);
}
@RequiresCapability(GlobalCapability.ADMINISTRATE_SERVER)
@@ -109,10 +111,9 @@
}
@Override
- public Object apply(PluginResource resource, Input input)
- throws AuthException, BadRequestException, ResourceConflictException,
- Exception {
- return new InstallPlugin(loader, resource.getName())
+ public Response<ListPlugins.PluginInfo> apply(PluginResource resource,
+ Input input) throws BadRequestException, IOException {
+ return new InstallPlugin(loader, resource.getName(), false)
.apply(TopLevelResource.INSTANCE, input);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginsCollection.java
index 1127a54..d461eb1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginsCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginsCollection.java
@@ -59,7 +59,7 @@
@Override
public InstallPlugin create(TopLevelResource parent, String id)
throws ResourceNotFoundException {
- return new InstallPlugin(loader, Url.decode(id));
+ return new InstallPlugin(loader, Url.decode(id), true /* created */);
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SetDefaultDashboard.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SetDefaultDashboard.java
index 6f82eca..7fb2986 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SetDefaultDashboard.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SetDefaultDashboard.java
@@ -18,6 +18,7 @@
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.Response;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -112,7 +113,7 @@
info.isDefault = true;
return info;
}
- return new Object();
+ return Response.none();
} finally {
md.close();
}
@@ -147,9 +148,9 @@
Exception {
SetDefaultDashboard set = setDefault.get();
set.inherited = inherited;
- return set.apply(
+ return Response.created(set.apply(
DashboardResource.projectDefault(resource.getControl()),
- input);
+ input));
}
}
}
