From c7c287be3a8eb38aebe97281a90c67579289f67c Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 19 Jul 2018 10:29:14 +0200 Subject: [PATCH] Replace REST collections for rebase/publish of change edits with REST views Currently rebase and publish of change edits are implemented as REST collections that only support POST on the collection, but no listing/parsing of members. This adds unneeded complexity. It's cleaner to just bind normal REST views to handle these POST requests. ChangesRestApiBindingsIT.changeEditEndpoints() verifies that the /changes//edit:rebase and /changes//edit:publish REST endpoints are still reachable. In addition ChangeEditIT has specific tests for the change edit REST API, in particular rebaseEditRest and publishEditRest, that ensure that these REST endpoints are still working. Change-Id: I5918f72b2478b0e0f1353e57c900c613a20b32d9 Signed-off-by: Edwin Kempin --- .../server/api/changes/ChangeEditApiImpl.java | 8 +- .../server/change/ChangeEditResource.java | 16 --- .../gerrit/server/restapi/change/Module.java | 10 +- .../restapi/change/PublishChangeEdit.java | 102 ++++++------------ .../restapi/change/RebaseChangeEdit.java | 77 ++++--------- 5 files changed, 59 insertions(+), 154 deletions(-) diff --git a/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java index 92aae03832..73f6740e02 100644 --- a/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java +++ b/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java @@ -47,8 +47,8 @@ public class ChangeEditApiImpl implements ChangeEditApi { private final ChangeEdits.Detail editDetail; private final ChangeEdits.Post changeEditsPost; private final DeleteChangeEdit deleteChangeEdit; - private final RebaseChangeEdit.Rebase rebaseChangeEdit; - private final PublishChangeEdit.Publish publishChangeEdit; + private final RebaseChangeEdit rebaseChangeEdit; + private final PublishChangeEdit publishChangeEdit; private final ChangeEdits.Get changeEditsGet; private final ChangeEdits.Put changeEditsPut; private final ChangeEdits.DeleteContent changeEditDeleteContent; @@ -62,8 +62,8 @@ public class ChangeEditApiImpl implements ChangeEditApi { ChangeEdits.Detail editDetail, ChangeEdits.Post changeEditsPost, DeleteChangeEdit deleteChangeEdit, - RebaseChangeEdit.Rebase rebaseChangeEdit, - PublishChangeEdit.Publish publishChangeEdit, + RebaseChangeEdit rebaseChangeEdit, + PublishChangeEdit publishChangeEdit, ChangeEdits.Get changeEditsGet, ChangeEdits.Put changeEditsPut, ChangeEdits.DeleteContent changeEditDeleteContent, diff --git a/java/com/google/gerrit/server/change/ChangeEditResource.java b/java/com/google/gerrit/server/change/ChangeEditResource.java index 5419ee996e..392709e536 100644 --- a/java/com/google/gerrit/server/change/ChangeEditResource.java +++ b/java/com/google/gerrit/server/change/ChangeEditResource.java @@ -32,10 +32,6 @@ import com.google.inject.TypeLiteral; public class ChangeEditResource implements RestResource { public static final TypeLiteral> CHANGE_EDIT_KIND = new TypeLiteral>() {}; - public static final TypeLiteral> CHANGE_EDIT_PUBLISH_KIND = - new TypeLiteral>() {}; - public static final TypeLiteral> CHANGE_EDIT_REBASE_KIND = - new TypeLiteral>() {}; private final ChangeResource change; private final ChangeEdit edit; @@ -64,16 +60,4 @@ public class ChangeEditResource implements RestResource { public String getPath() { return path; } - - public static class Publish extends ChangeEditResource { - public Publish(ChangeResource change, ChangeEdit edit, String path) { - super(change, edit, path); - } - } - - public static class Rebase extends ChangeEditResource { - public Rebase(ChangeResource change, ChangeEdit edit, String path) { - super(change, edit, path); - } - } } diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java index cbc6e6cc04..5b28971409 100644 --- a/java/com/google/gerrit/server/restapi/change/Module.java +++ b/java/com/google/gerrit/server/restapi/change/Module.java @@ -15,8 +15,6 @@ package com.google.gerrit.server.restapi.change; import static com.google.gerrit.server.change.ChangeEditResource.CHANGE_EDIT_KIND; -import static com.google.gerrit.server.change.ChangeEditResource.CHANGE_EDIT_PUBLISH_KIND; -import static com.google.gerrit.server.change.ChangeEditResource.CHANGE_EDIT_REBASE_KIND; import static com.google.gerrit.server.change.ChangeMessageResource.CHANGE_MESSAGE_KIND; import static com.google.gerrit.server.change.ChangeResource.CHANGE_KIND; import static com.google.gerrit.server.change.CommentResource.COMMENT_KIND; @@ -67,8 +65,6 @@ public class Module extends RestApiModule { DynamicMap.mapOf(binder(), REVIEWER_KIND); DynamicMap.mapOf(binder(), REVISION_KIND); DynamicMap.mapOf(binder(), CHANGE_EDIT_KIND); - DynamicMap.mapOf(binder(), CHANGE_EDIT_PUBLISH_KIND); - DynamicMap.mapOf(binder(), CHANGE_EDIT_REBASE_KIND); DynamicMap.mapOf(binder(), VOTE_KIND); DynamicMap.mapOf(binder(), CHANGE_MESSAGE_KIND); @@ -174,10 +170,8 @@ public class Module extends RestApiModule { deleteMissing(CHANGE_EDIT_KIND).to(ChangeEdits.DeleteFile.class); postOnCollection(CHANGE_EDIT_KIND).to(ChangeEdits.Post.class); deleteOnCollection(CHANGE_EDIT_KIND).to(DeleteChangeEdit.class); - child(CHANGE_KIND, "edit:publish").to(PublishChangeEdit.class); - postOnCollection(CHANGE_EDIT_PUBLISH_KIND).to(PublishChangeEdit.Publish.class); - child(CHANGE_KIND, "edit:rebase").to(RebaseChangeEdit.class); - postOnCollection(CHANGE_EDIT_REBASE_KIND).to(RebaseChangeEdit.Rebase.class); + post(CHANGE_KIND, "edit:publish").to(PublishChangeEdit.class); + post(CHANGE_KIND, "edit:rebase").to(RebaseChangeEdit.class); put(CHANGE_KIND, "edit:message").to(ChangeEdits.EditMessage.class); get(CHANGE_KIND, "edit:message").to(ChangeEdits.GetMessage.class); put(CHANGE_EDIT_KIND, "/").to(ChangeEdits.Put.class); diff --git a/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java b/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java index 3633e99e0d..3d401c498a 100644 --- a/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java +++ b/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java @@ -15,15 +15,9 @@ package com.google.gerrit.server.restapi.change; import com.google.gerrit.extensions.api.changes.PublishChangeEditInput; -import com.google.gerrit.extensions.registration.DynamicMap; -import com.google.gerrit.extensions.restapi.ChildCollection; -import com.google.gerrit.extensions.restapi.IdString; 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.RestView; -import com.google.gerrit.server.change.ChangeEditResource; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.NotifyUtil; import com.google.gerrit.server.edit.ChangeEdit; @@ -32,7 +26,7 @@ import com.google.gerrit.server.project.ContributorAgreementsChecker; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryingRestCollectionModifyView; +import com.google.gerrit.server.update.RetryingRestModifyView; import com.google.gerrit.server.update.UpdateException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -43,74 +37,44 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PublishChangeEdit - implements ChildCollection { - - private final DynamicMap> views; + extends RetryingRestModifyView> { + private final ChangeEditUtil editUtil; + private final NotifyUtil notifyUtil; + private final ContributorAgreementsChecker contributorAgreementsChecker; @Inject - PublishChangeEdit(DynamicMap> views) { - this.views = views; + PublishChangeEdit( + RetryHelper retryHelper, + ChangeEditUtil editUtil, + NotifyUtil notifyUtil, + ContributorAgreementsChecker contributorAgreementsChecker) { + super(retryHelper); + this.editUtil = editUtil; + this.notifyUtil = notifyUtil; + this.contributorAgreementsChecker = contributorAgreementsChecker; } @Override - public DynamicMap> views() { - return views; - } - - @Override - public RestView list() throws ResourceNotFoundException { - throw new ResourceNotFoundException(); - } - - @Override - public ChangeEditResource.Publish parse(ChangeResource parent, IdString id) - throws ResourceNotFoundException { - throw new ResourceNotFoundException(); - } - - @Singleton - public static class Publish - extends RetryingRestCollectionModifyView< - ChangeResource, ChangeEditResource.Publish, PublishChangeEditInput, Response> { - - private final ChangeEditUtil editUtil; - private final NotifyUtil notifyUtil; - private final ContributorAgreementsChecker contributorAgreementsChecker; - - @Inject - Publish( - RetryHelper retryHelper, - ChangeEditUtil editUtil, - NotifyUtil notifyUtil, - ContributorAgreementsChecker contributorAgreementsChecker) { - super(retryHelper); - this.editUtil = editUtil; - this.notifyUtil = notifyUtil; - this.contributorAgreementsChecker = contributorAgreementsChecker; + protected Response applyImpl( + BatchUpdate.Factory updateFactory, ChangeResource rsrc, PublishChangeEditInput in) + throws IOException, OrmException, RestApiException, UpdateException, ConfigInvalidException, + NoSuchProjectException { + contributorAgreementsChecker.check(rsrc.getProject(), rsrc.getUser()); + Optional edit = editUtil.byChange(rsrc.getNotes(), rsrc.getUser()); + if (!edit.isPresent()) { + throw new ResourceConflictException( + String.format("no edit exists for change %s", rsrc.getChange().getChangeId())); } - - @Override - protected Response applyImpl( - BatchUpdate.Factory updateFactory, ChangeResource rsrc, PublishChangeEditInput in) - throws IOException, OrmException, RestApiException, UpdateException, ConfigInvalidException, - NoSuchProjectException { - contributorAgreementsChecker.check(rsrc.getProject(), rsrc.getUser()); - Optional edit = editUtil.byChange(rsrc.getNotes(), rsrc.getUser()); - if (!edit.isPresent()) { - throw new ResourceConflictException( - String.format("no edit exists for change %s", rsrc.getChange().getChangeId())); - } - if (in == null) { - in = new PublishChangeEditInput(); - } - editUtil.publish( - updateFactory, - rsrc.getNotes(), - rsrc.getUser(), - edit.get(), - in.notify, - notifyUtil.resolveAccounts(in.notifyDetails)); - return Response.none(); + if (in == null) { + in = new PublishChangeEditInput(); } + editUtil.publish( + updateFactory, + rsrc.getNotes(), + rsrc.getUser(), + edit.get(), + in.notify, + notifyUtil.resolveAccounts(in.notifyDetails)); + return Response.none(); } } diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChangeEdit.java b/java/com/google/gerrit/server/restapi/change/RebaseChangeEdit.java index e054672844..6020e957ba 100644 --- a/java/com/google/gerrit/server/restapi/change/RebaseChangeEdit.java +++ b/java/com/google/gerrit/server/restapi/change/RebaseChangeEdit.java @@ -15,16 +15,10 @@ package com.google.gerrit.server.restapi.change; import com.google.gerrit.extensions.common.Input; -import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AuthException; -import com.google.gerrit.extensions.restapi.ChildCollection; -import com.google.gerrit.extensions.restapi.IdString; 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.RestView; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.server.change.ChangeEditResource; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.edit.ChangeEditModifier; import com.google.gerrit.server.git.GitRepositoryManager; @@ -32,7 +26,7 @@ import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryingRestCollectionModifyView; +import com.google.gerrit.server.update.RetryingRestModifyView; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -40,61 +34,30 @@ import java.io.IOException; import org.eclipse.jgit.lib.Repository; @Singleton -public class RebaseChangeEdit - implements ChildCollection { - - private final DynamicMap> views; +public class RebaseChangeEdit extends RetryingRestModifyView> { + private final GitRepositoryManager repositoryManager; + private final ChangeEditModifier editModifier; @Inject - RebaseChangeEdit(DynamicMap> views) { - this.views = views; + RebaseChangeEdit( + RetryHelper retryHelper, + GitRepositoryManager repositoryManager, + ChangeEditModifier editModifier) { + super(retryHelper); + this.repositoryManager = repositoryManager; + this.editModifier = editModifier; } @Override - public DynamicMap> views() { - return views; - } - - @Override - public RestView list() throws ResourceNotFoundException { - throw new ResourceNotFoundException(); - } - - @Override - public ChangeEditResource.Rebase parse(ChangeResource parent, IdString id) - throws ResourceNotFoundException { - throw new ResourceNotFoundException(); - } - - @Singleton - public static class Rebase - extends RetryingRestCollectionModifyView< - ChangeResource, ChangeEditResource.Rebase, Input, Response> { - private final GitRepositoryManager repositoryManager; - private final ChangeEditModifier editModifier; - - @Inject - Rebase( - RetryHelper retryHelper, - GitRepositoryManager repositoryManager, - ChangeEditModifier editModifier) { - super(retryHelper); - this.repositoryManager = repositoryManager; - this.editModifier = editModifier; - } - - @Override - protected Response applyImpl( - BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input in) - throws AuthException, ResourceConflictException, IOException, OrmException, - PermissionBackendException { - Project.NameKey project = rsrc.getProject(); - try (Repository repository = repositoryManager.openRepository(project)) { - editModifier.rebaseEdit(repository, rsrc.getNotes()); - } catch (InvalidChangeOperationException e) { - throw new ResourceConflictException(e.getMessage()); - } - return Response.none(); + protected Response applyImpl(BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input in) + throws AuthException, ResourceConflictException, IOException, OrmException, + PermissionBackendException { + Project.NameKey project = rsrc.getProject(); + try (Repository repository = repositoryManager.openRepository(project)) { + editModifier.rebaseEdit(repository, rsrc.getNotes()); + } catch (InvalidChangeOperationException e) { + throw new ResourceConflictException(e.getMessage()); } + return Response.none(); } }