RestApiModule: Support binding a RestView to delete a missing resource
A change edit can be created by deleting a file (that exists in the
change) from the non-existing change edit:
DELETE /changes/<change-id>/edit/<file-path>
The same request is also used to delete the content of the specified
file from an existing change edit.
Technically what happens is that:
1. The ChangeEdits RestCollection is asked to parse the <file-path>
2. If the change edit and the file in the change exist, a
ChangeEditResource(*) is returned, if the change edit doesn't exist
the 'parse' method throws a ResourceNotFoundException (indicating
that the member wasn't found)
3a. If the ChangeEditResource was returned, we find the RestView that is
bound for DELETE on ChangeEditResources and let it handle the
request.
3b. If a ResourceNotFoundException was thrown, we catch the exception
and check if the RestCollection implements AcceptsDelete and if yes,
we invoke the 'delete' method to *create* the change edit and to
delete the member from it.
(*) Note that ChangeEditResource represents 2 kind of resources
(according to its JavaDoc), the change edit itself and a file within
the change edit.
I find 3b. pretty confusing and would have preferred that this use case
would have required two calls, one to create the change edit, and one to
delete the file in the *existing* change edit, but now it's hard to
change without breaking anything.
To make this logic easier to understand RestApiModule now offers to bind
a RestDeleteMissingView for DELETE requests that should be handled on
missing collection members (similar to how a RestCreateView can be bound
for resource creation by PUT and POST, see change I5cd61f77a).
This is the first step to remove the AcceptsDelete interface. The other
use case for the AcceptsDelete interface is to support DELETE on the
RestCollection itself. Gerrit core doesn't use this functionality but
plugins may rely on it. Hence for now AcceptsDelete is kept for this
purpose. It's planned to support this use case by REST bindings too, but
this will be implemented by a follow-up change (similar to how POST on
RestCollection is supported now, see change Iea3b8e9800).
This change improves code readability since by reading the Module class
we can now directly see which REST collections support DELETE on missing
members.
The ChangesRestApiBindingsIT.changeEditCreateEndpoints test verifies
that the REST request for creating a change edit by DELETE is correctly
resolved to a REST endpoint implementation.
Change-Id: If64224d502bbfe578133d1255b42902dd4fbe4fb
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/extensions/restapi/AcceptsDelete.java b/java/com/google/gerrit/extensions/restapi/AcceptsDelete.java
index e1c4b3b..774e0a3 100644
--- a/java/com/google/gerrit/extensions/restapi/AcceptsDelete.java
+++ b/java/com/google/gerrit/extensions/restapi/AcceptsDelete.java
@@ -17,31 +17,17 @@
/**
* Optional interface for {@link RestCollection}.
*
- * <p>This interface is used for 2 purposes:
- *
- * <ul>
- * <li>to support {@code DELETE} directly on the collection itself
- * <li>to support {@code DELETE} on a non-existing member of the collection (in order to create
- * that member)
- * </ul>
+ * <p>This interface is used to support {@code DELETE} directly on the collection itself.
*
* <p>This interface is not supported for root collections.
*/
public interface AcceptsDelete<P extends RestResource> {
/**
- * Handle
- *
- * <ul>
- * <li>{@code DELETE} directly on the collection itself (in this case id is {@code null})
- * <li>{@code DELETE} on a non-existing member of the collection (in this case id is not {@code
- * null})
- * </ul>
+ * Handle {@code DELETE} directly on the collection itself.
*
* @param parent the collection
- * @param id id of the non-existing collection member for which the {@code DELETE} request is
- * done, {@code null} if the {@code DELETE} request is done on the collection itself
* @return a view to handle the {@code DELETE} request
* @throws RestApiException the view cannot be constructed
*/
- RestModifyView<P, ?> delete(P parent, IdString id) throws RestApiException;
+ RestModifyView<P, ?> delete(P parent) throws RestApiException;
}
diff --git a/java/com/google/gerrit/extensions/restapi/RestApiModule.java b/java/com/google/gerrit/extensions/restapi/RestApiModule.java
index d47b094..c1bb95e 100644
--- a/java/com/google/gerrit/extensions/restapi/RestApiModule.java
+++ b/java/com/google/gerrit/extensions/restapi/RestApiModule.java
@@ -29,6 +29,7 @@
protected static final String DELETE = "DELETE";
protected static final String POST = "POST";
protected static final String CREATE = "CREATE";
+ protected static final String DELETE_MISSING = "DELETE_MISSING";
protected static final String POST_ON_COLLECTION = "POST_ON_COLLECTION";
protected <R extends RestResource> ReadViewBinder<R> get(TypeLiteral<RestView<R>> viewType) {
@@ -57,6 +58,11 @@
return new CreateViewBinder<>(bind(viewType).annotatedWith(export(CREATE, "/")));
}
+ protected <R extends RestResource> DeleteViewBinder<R> deleteMissing(
+ TypeLiteral<RestView<R>> viewType) {
+ return new DeleteViewBinder<>(bind(viewType).annotatedWith(export(DELETE_MISSING, "/")));
+ }
+
protected <R extends RestResource> ReadViewBinder<R> get(
TypeLiteral<RestView<R>> viewType, String name) {
return new ReadViewBinder<>(view(viewType, GET, name));
@@ -203,6 +209,34 @@
}
}
+ public static class DeleteViewBinder<C extends RestResource> {
+ private final LinkedBindingBuilder<RestView<C>> binder;
+
+ private DeleteViewBinder(LinkedBindingBuilder<RestView<C>> binder) {
+ this.binder = binder;
+ }
+
+ public <P extends RestResource, T extends RestDeleteMissingView<P, C, ?>>
+ ScopedBindingBuilder to(Class<T> impl) {
+ return binder.to(impl);
+ }
+
+ public <P extends RestResource, T extends RestDeleteMissingView<P, C, ?>> void toInstance(
+ T impl) {
+ binder.toInstance(impl);
+ }
+
+ public <P extends RestResource, T extends RestDeleteMissingView<P, C, ?>>
+ ScopedBindingBuilder toProvider(Class<? extends Provider<? extends T>> providerType) {
+ return binder.toProvider(providerType);
+ }
+
+ public <P extends RestResource, T extends RestDeleteMissingView<P, C, ?>>
+ ScopedBindingBuilder toProvider(Provider<? extends T> provider) {
+ return binder.toProvider(provider);
+ }
+ }
+
public static class ChildCollectionBinder<P extends RestResource> {
private final LinkedBindingBuilder<RestView<P>> binder;
diff --git a/java/com/google/gerrit/extensions/restapi/RestDeleteMissingView.java b/java/com/google/gerrit/extensions/restapi/RestDeleteMissingView.java
new file mode 100644
index 0000000..1d58c80
--- /dev/null
+++ b/java/com/google/gerrit/extensions/restapi/RestDeleteMissingView.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2018 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;
+
+/**
+ * RestView that supports accepting input and deleting a resource that is missing.
+ *
+ * <p>The RestDeleteMissingView solely exists to support a special case for creating a change edit
+ * by deleting a path in the non-existing change edit. This interface should not be used for new
+ * REST API's.
+ *
+ * <p>The input must be supplied as JSON as the body of the HTTP request. Delete views can be
+ * invoked by the HTTP method {@code DELETE}.
+ *
+ * <p>The RestDeleteMissingView is only invoked when the parse method of the {@code RestCollection}
+ * throws {@link ResourceNotFoundException}, and hence the resource doesn't exist yet.
+ *
+ * @param <P> type of the parent resource
+ * @param <C> type of the child resource that id deleted
+ * @param <I> type of input the JSON parser will parse the input into.
+ */
+public interface RestDeleteMissingView<P extends RestResource, C extends RestResource, I>
+ extends RestView<C> {
+
+ /**
+ * Process the view operation by deleting the resource.
+ *
+ * @param parentResource parent resource of the resource that should be deleted
+ * @param input input after parsing from request.
+ * @return result to return to the client. Use {@link BinaryResult} to avoid automatic conversion
+ * to JSON.
+ * @throws RestApiException if the resource creation is rejected
+ * @throws Exception the implementation of the view failed. The exception will be logged and HTTP
+ * 500 Internal Server Error will be returned to the client.
+ */
+ Object apply(P parentResource, IdString id, I input) throws Exception;
+}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 599e98f..70296a3 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -88,6 +88,7 @@
import com.google.gerrit.extensions.restapi.RestCollection;
import com.google.gerrit.extensions.restapi.RestCollectionView;
import com.google.gerrit.extensions.restapi.RestCreateView;
+import com.google.gerrit.extensions.restapi.RestDeleteMissingView;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.RestResource;
@@ -325,15 +326,28 @@
checkPreconditions(req);
}
} catch (ResourceNotFoundException e) {
- if (!path.isEmpty() || (!isPost(req) && !isPut(req))) {
+ if (!path.isEmpty()) {
throw e;
}
- RestView<RestResource> createView = rc.views().get("gerrit", "CREATE./");
- if (createView != null) {
- viewData = new ViewData(null, createView);
- status = SC_CREATED;
- path.add(id);
+ if (isPost(req) || isPut(req)) {
+ RestView<RestResource> createView = rc.views().get("gerrit", "CREATE./");
+ if (createView != null) {
+ viewData = new ViewData(null, createView);
+ status = SC_CREATED;
+ path.add(id);
+ } else {
+ throw e;
+ }
+ } else if (isDelete(req)) {
+ RestView<RestResource> deleteView = rc.views().get("gerrit", "DELETE_MISSING./");
+ if (deleteView != null) {
+ viewData = new ViewData(null, deleteView);
+ status = SC_NO_CONTENT;
+ path.add(id);
+ } else {
+ throw e;
+ }
} else {
throw e;
}
@@ -363,7 +377,7 @@
} else if (c instanceof AcceptsDelete && isDelete(req)) {
@SuppressWarnings("unchecked")
AcceptsDelete<RestResource> ac = (AcceptsDelete<RestResource>) c;
- viewData = new ViewData(null, ac.delete(rsrc, null));
+ viewData = new ViewData(null, ac.delete(rsrc));
} else {
throw new MethodNotAllowedException();
}
@@ -388,11 +402,15 @@
} else {
throw e;
}
- } else if (c instanceof AcceptsDelete && isDelete(req)) {
- @SuppressWarnings("unchecked")
- AcceptsDelete<RestResource> ac = (AcceptsDelete<RestResource>) c;
- viewData = new ViewData(viewData.pluginName, ac.delete(rsrc, id));
- status = SC_NO_CONTENT;
+ } else if (isDelete(req)) {
+ RestView<RestResource> deleteView = c.views().get("gerrit", "DELETE_MISSING./");
+ if (deleteView != null) {
+ viewData = new ViewData(null, deleteView);
+ status = SC_NO_CONTENT;
+ path.add(id);
+ } else {
+ throw e;
+ }
} else {
throw e;
}
@@ -440,6 +458,19 @@
ServletUtils.consumeRequestBody(is);
}
}
+ } else if (viewData.view instanceof RestDeleteMissingView<?, ?, ?>) {
+ @SuppressWarnings("unchecked")
+ RestDeleteMissingView<RestResource, RestResource, Object> m =
+ (RestDeleteMissingView<RestResource, RestResource, Object>) viewData.view;
+
+ Type type = inputType(m);
+ inputRequestBody = parseRequest(req, type);
+ result = m.apply(rsrc, path.get(0), inputRequestBody);
+ if (inputRequestBody instanceof RawInput) {
+ try (InputStream is = req.getInputStream()) {
+ ServletUtils.consumeRequestBody(is);
+ }
+ }
} else if (viewData.view instanceof RestCollectionView<?, ?, ?>) {
@SuppressWarnings("unchecked")
RestCollectionView<RestResource, RestResource, Object> m =
@@ -797,6 +828,24 @@
return ((ParameterizedType) supertype).getActualTypeArguments()[2];
}
+ private static Type inputType(RestDeleteMissingView<RestResource, RestResource, Object> m) {
+ // MyCreateView implements RestDeleteMissingView<SomeResource, SomeResource, MyInput>
+ TypeLiteral<?> typeLiteral = TypeLiteral.get(m.getClass());
+
+ // RestDeleteMissingView<SomeResource, SomeResource, MyInput>
+ // This is smart enough to resolve even when there are intervening subclasses, even if they have
+ // reordered type arguments.
+ TypeLiteral<?> supertypeLiteral = typeLiteral.getSupertype(RestDeleteMissingView.class);
+
+ Type supertype = supertypeLiteral.getType();
+ checkState(
+ supertype instanceof ParameterizedType,
+ "supertype of %s is not parameterized: %s",
+ typeLiteral,
+ supertypeLiteral);
+ return ((ParameterizedType) supertype).getActualTypeArguments()[2];
+ }
+
private static Type inputType(RestCollectionView<RestResource, RestResource, Object> m) {
// MyCollectionView implements RestCollectionView<SomeResource, SomeResource, MyInput>
TypeLiteral<?> typeLiteral = TypeLiteral.get(m.getClass());
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
index efc2712..70eb3e5 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
@@ -19,7 +19,6 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.extensions.restapi.AcceptsDelete;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
@@ -30,9 +29,9 @@
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.RestCollectionView;
import com.google.gerrit.extensions.restapi.RestCreateView;
+import com.google.gerrit.extensions.restapi.RestDeleteMissingView;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.RestView;
@@ -59,7 +58,6 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
-import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.List;
import java.util.Optional;
@@ -70,10 +68,8 @@
import org.kohsuke.args4j.Option;
@Singleton
-public class ChangeEdits
- implements ChildCollection<ChangeResource, ChangeEditResource>, AcceptsDelete<ChangeResource> {
+public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditResource> {
private final DynamicMap<RestView<ChangeEditResource>> views;
- private final DeleteFile.Factory deleteFileFactory;
private final Provider<Detail> detail;
private final ChangeEditUtil editUtil;
@@ -81,12 +77,10 @@
ChangeEdits(
DynamicMap<RestView<ChangeEditResource>> views,
Provider<Detail> detail,
- ChangeEditUtil editUtil,
- DeleteFile.Factory deleteFileFactory) {
+ ChangeEditUtil editUtil) {
this.views = views;
this.detail = detail;
this.editUtil = editUtil;
- this.deleteFileFactory = deleteFileFactory;
}
@Override
@@ -110,20 +104,6 @@
}
/**
- * This method is invoked if a DELETE request on a non-existing member is done. For change edits
- * this is the case if a DELETE request for a file in a change edit is done and the change edit
- * doesn't exist yet (and hence the parse method returned ResourceNotFoundException). In this case
- * we want to create the change edit on the fly and delete the file with the given id in it.
- */
- @Override
- public DeleteFile delete(ChangeResource parent, IdString id) throws RestApiException {
- // It's safe to assume that id can never be null, because
- // otherwise we would end up in dedicated endpoint for
- // deleting of change edits and not a file in change edit
- return deleteFileFactory.create(id.get());
- }
-
- /**
* Create handler that is activated when collection element is accessed but doesn't exist, e. g.
* PUT request with a path was called but change edit wasn't created yet. Change edit is created
* and PUT handler is called.
@@ -146,26 +126,20 @@
}
}
- public static class DeleteFile implements RestModifyView<ChangeResource, Input> {
-
- public interface Factory {
- DeleteFile create(String path);
- }
-
+ public static class DeleteFile
+ implements RestDeleteMissingView<ChangeResource, ChangeEditResource, Input> {
private final DeleteContent deleteContent;
- private final String path;
@Inject
- DeleteFile(DeleteContent deleteContent, @Assisted String path) {
+ DeleteFile(DeleteContent deleteContent) {
this.deleteContent = deleteContent;
- this.path = path;
}
@Override
- public Response<?> apply(ChangeResource rsrc, Input in)
+ public Response<?> apply(ChangeResource rsrc, IdString id, Input in)
throws IOException, AuthException, ResourceConflictException, OrmException,
PermissionBackendException {
- return deleteContent.apply(rsrc, path);
+ return deleteContent.apply(rsrc, id.get());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java
index dca64fd..5c13303 100644
--- a/java/com/google/gerrit/server/restapi/change/Module.java
+++ b/java/com/google/gerrit/server/restapi/change/Module.java
@@ -171,6 +171,7 @@
child(CHANGE_KIND, "edit").to(ChangeEdits.class);
create(CHANGE_EDIT_KIND).to(ChangeEdits.Create.class);
+ deleteMissing(CHANGE_EDIT_KIND).to(ChangeEdits.DeleteFile.class);
postOnCollection(CHANGE_EDIT_KIND).to(ChangeEdits.Post.class);
delete(CHANGE_KIND, "edit").to(DeleteChangeEdit.class);
child(CHANGE_KIND, "edit:publish").to(PublishChangeEdit.class);
@@ -189,7 +190,6 @@
get(CHANGE_MESSAGE_KIND).to(GetChangeMessage.class);
factory(AccountLoader.Factory.class);
- factory(ChangeEdits.DeleteFile.Factory.class);
factory(ChangeInserter.Factory.class);
factory(ChangeResource.Factory.class);
factory(DeleteReviewerByEmailOp.Factory.class);
