Merge "CommentJsonMigrator: add test for dryRun flag"
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
index aed372c..0134ce3 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
@@ -23,9 +23,7 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
-import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -38,17 +36,12 @@
public class DeleteBranch implements RestModifyView<BranchResource, Input> {
private final Provider<InternalChangeQuery> queryProvider;
- private final DeleteRef.Factory deleteRefFactory;
- private final PermissionBackend permissionBackend;
+ private final DeleteRef deleteRef;
@Inject
- DeleteBranch(
- Provider<InternalChangeQuery> queryProvider,
- DeleteRef.Factory deleteRefFactory,
- PermissionBackend permissionBackend) {
+ DeleteBranch(Provider<InternalChangeQuery> queryProvider, DeleteRef deleteRef) {
this.queryProvider = queryProvider;
- this.deleteRefFactory = deleteRefFactory;
- this.permissionBackend = permissionBackend;
+ this.deleteRef = deleteRef;
}
@Override
@@ -60,14 +53,11 @@
"not allowed to delete branch " + rsrc.getBranchKey().get());
}
- permissionBackend.currentUser().ref(rsrc.getBranchKey()).check(RefPermission.DELETE);
- rsrc.getProjectState().checkStatePermitsWrite();
-
if (!queryProvider.get().setLimit(1).byBranchOpen(rsrc.getBranchKey()).isEmpty()) {
throw new ResourceConflictException("branch " + rsrc.getBranchKey() + " has open changes");
}
- deleteRefFactory.create(rsrc).ref(rsrc.getRef()).prefix(R_HEADS).delete();
+ deleteRef.deleteSingleRef(rsrc.getProjectState(), rsrc.getRef(), R_HEADS);
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java
index d8166e1..6e60193 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java
@@ -16,6 +16,7 @@
import static org.eclipse.jgit.lib.Constants.R_HEADS;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,11 +31,11 @@
@Singleton
public class DeleteBranches implements RestModifyView<ProjectResource, DeleteBranchesInput> {
- private final DeleteRef.Factory deleteRefFactory;
+ private final DeleteRef deleteRef;
@Inject
- DeleteBranches(DeleteRef.Factory deleteRefFactory) {
- this.deleteRefFactory = deleteRefFactory;
+ DeleteBranches(DeleteRef deleteRef) {
+ this.deleteRef = deleteRef;
}
@Override
@@ -43,7 +44,8 @@
if (input == null || input.branches == null || input.branches.isEmpty()) {
throw new BadRequestException("branches must be specified");
}
- deleteRefFactory.create(project).refs(input.branches).prefix(R_HEADS).delete();
+ deleteRef.deleteMultipleRefs(
+ project.getProjectState(), ImmutableSet.copyOf(input.branches), R_HEADS);
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteRef.java b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
index 769eaf8..0016354 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteRef.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
@@ -14,33 +14,35 @@
package com.google.gerrit.server.restapi.project;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef;
import static java.lang.String.format;
-import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_REFS;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.DELETE;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
-import com.google.gerrit.server.project.ProjectResource;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefValidationHelper;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import com.google.inject.assistedinject.Assisted;
+import com.google.inject.Singleton;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
import org.eclipse.jgit.errors.LockFailedException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -52,6 +54,7 @@
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.transport.ReceiveCommand.Result;
+@Singleton
public class DeleteRef {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -64,13 +67,6 @@
private final GitReferenceUpdated referenceUpdated;
private final RefValidationHelper refDeletionValidator;
private final Provider<InternalChangeQuery> queryProvider;
- private final ProjectResource resource;
- private final List<String> refsToDelete;
- private String prefix;
-
- public interface Factory {
- DeleteRef create(ProjectResource r);
- }
@Inject
DeleteRef(
@@ -79,135 +75,164 @@
GitRepositoryManager repoManager,
GitReferenceUpdated referenceUpdated,
RefValidationHelper.Factory refDeletionValidatorFactory,
- Provider<InternalChangeQuery> queryProvider,
- @Assisted ProjectResource resource) {
+ Provider<InternalChangeQuery> queryProvider) {
this.identifiedUser = identifiedUser;
this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.referenceUpdated = referenceUpdated;
this.refDeletionValidator = refDeletionValidatorFactory.create(DELETE);
this.queryProvider = queryProvider;
- this.resource = resource;
- this.refsToDelete = new ArrayList<>();
}
- public DeleteRef ref(String ref) {
- this.refsToDelete.add(ref);
- return this;
+ /**
+ * Deletes a single ref from the repository.
+ *
+ * @param projectState the {@code ProjectState} of the project containing the target ref.
+ * @param ref the ref to be deleted.
+ * @throws IOException
+ * @throws ResourceConflictException
+ */
+ public void deleteSingleRef(ProjectState projectState, String ref)
+ throws IOException, ResourceConflictException, AuthException, PermissionBackendException {
+ deleteSingleRef(projectState, ref, null);
}
- public DeleteRef refs(List<String> refs) {
- this.refsToDelete.addAll(refs);
- return this;
- }
-
- public DeleteRef prefix(String prefix) {
- this.prefix = prefix;
- return this;
- }
-
- public void delete()
- throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
- if (!refsToDelete.isEmpty()) {
- try (Repository r = repoManager.openRepository(resource.getNameKey())) {
- if (refsToDelete.size() == 1) {
- deleteSingleRef(r);
- } else {
- deleteMultipleRefs(r);
- }
- }
- }
- }
-
- private void deleteSingleRef(Repository r) throws IOException, ResourceConflictException {
- String ref = refsToDelete.get(0);
+ /**
+ * Deletes a single ref from the repository.
+ *
+ * @param projectState the {@code ProjectState} of the project containing the target ref.
+ * @param ref the ref to be deleted.
+ * @param prefix the prefix of the ref.
+ * @throws IOException
+ * @throws ResourceConflictException
+ */
+ public void deleteSingleRef(ProjectState projectState, String ref, @Nullable String prefix)
+ throws IOException, ResourceConflictException, AuthException, PermissionBackendException {
if (prefix != null && !ref.startsWith(R_REFS)) {
ref = prefix + ref;
}
- RefUpdate.Result result;
- RefUpdate u = r.updateRef(ref);
- u.setExpectedOldObjectId(r.exactRef(ref).getObjectId());
- u.setNewObjectId(ObjectId.zeroId());
- u.setForceUpdate(true);
- refDeletionValidator.validateRefOperation(resource.getName(), identifiedUser.get(), u);
- int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
- for (; ; ) {
- try {
- result = u.delete();
- } catch (LockFailedException e) {
- result = RefUpdate.Result.LOCK_FAILURE;
- } catch (IOException e) {
- logger.atSevere().withCause(e).log("Cannot delete %s", ref);
- throw e;
- }
- if (result == RefUpdate.Result.LOCK_FAILURE && --remainingLockFailureCalls > 0) {
+
+ projectState.checkStatePermitsWrite();
+ permissionBackend
+ .currentUser()
+ .project(projectState.getNameKey())
+ .ref(ref)
+ .check(RefPermission.DELETE);
+
+ try (Repository repository = repoManager.openRepository(projectState.getNameKey())) {
+ RefUpdate.Result result;
+ RefUpdate u = repository.updateRef(ref);
+ u.setExpectedOldObjectId(repository.exactRef(ref).getObjectId());
+ u.setNewObjectId(ObjectId.zeroId());
+ u.setForceUpdate(true);
+ refDeletionValidator.validateRefOperation(projectState.getName(), identifiedUser.get(), u);
+ int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
+ for (; ; ) {
try {
- Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
- } catch (InterruptedException ie) {
- // ignore
+ result = u.delete();
+ } catch (LockFailedException e) {
+ result = RefUpdate.Result.LOCK_FAILURE;
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Cannot delete %s", ref);
+ throw e;
}
- } else {
- break;
+ if (result == RefUpdate.Result.LOCK_FAILURE && --remainingLockFailureCalls > 0) {
+ try {
+ Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
+ } catch (InterruptedException ie) {
+ // ignore
+ }
+ } else {
+ break;
+ }
}
- }
- switch (result) {
- case NEW:
- case NO_CHANGE:
- case FAST_FORWARD:
- case FORCED:
- referenceUpdated.fire(
- resource.getNameKey(), u, ReceiveCommand.Type.DELETE, identifiedUser.get().state());
- break;
+ switch (result) {
+ case NEW:
+ case NO_CHANGE:
+ case FAST_FORWARD:
+ case FORCED:
+ referenceUpdated.fire(
+ projectState.getNameKey(),
+ u,
+ ReceiveCommand.Type.DELETE,
+ identifiedUser.get().state());
+ break;
- case REJECTED_CURRENT_BRANCH:
- logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
- throw new ResourceConflictException("cannot delete current branch");
+ case REJECTED_CURRENT_BRANCH:
+ logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
+ throw new ResourceConflictException("cannot delete current branch");
- case IO_FAILURE:
- case LOCK_FAILURE:
- case NOT_ATTEMPTED:
- case REJECTED:
- case RENAMED:
- case REJECTED_MISSING_OBJECT:
- case REJECTED_OTHER_REASON:
- default:
- logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
- throw new ResourceConflictException("cannot delete: " + result.name());
+ case IO_FAILURE:
+ case LOCK_FAILURE:
+ case NOT_ATTEMPTED:
+ case REJECTED:
+ case RENAMED:
+ case REJECTED_MISSING_OBJECT:
+ case REJECTED_OTHER_REASON:
+ default:
+ logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
+ throw new ResourceConflictException("cannot delete: " + result.name());
+ }
}
}
- private void deleteMultipleRefs(Repository r)
- throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
- BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate();
- batchUpdate.setAtomic(false);
- List<String> refs =
- prefix == null
- ? refsToDelete
- : refsToDelete
- .stream()
- .map(ref -> ref.startsWith(R_REFS) ? ref : prefix + ref)
- .collect(toList());
- for (String ref : refs) {
- batchUpdate.addCommand(createDeleteCommand(resource, r, ref));
+ /**
+ * Deletes a set of refs from the repository.
+ *
+ * @param projectState the {@code ProjectState} of the project whose refs are to be deleted.
+ * @param refsToDelete the refs to be deleted.
+ * @param prefix the prefix of the refs.
+ * @throws OrmException
+ * @throws IOException
+ * @throws ResourceConflictException
+ * @throws PermissionBackendException
+ */
+ public void deleteMultipleRefs(
+ ProjectState projectState, ImmutableSet<String> refsToDelete, String prefix)
+ throws OrmException, IOException, ResourceConflictException, PermissionBackendException,
+ AuthException {
+ if (refsToDelete.isEmpty()) {
+ return;
}
- try (RevWalk rw = new RevWalk(r)) {
- batchUpdate.execute(rw, NullProgressMonitor.INSTANCE);
+
+ if (refsToDelete.size() == 1) {
+ deleteSingleRef(projectState, Iterables.getOnlyElement(refsToDelete), prefix);
+ return;
}
- StringBuilder errorMessages = new StringBuilder();
- for (ReceiveCommand command : batchUpdate.getCommands()) {
- if (command.getResult() == Result.OK) {
- postDeletion(resource, command);
- } else {
- appendAndLogErrorMessage(errorMessages, command);
+
+ try (Repository repository = repoManager.openRepository(projectState.getNameKey())) {
+ BatchRefUpdate batchUpdate = repository.getRefDatabase().newBatchUpdate();
+ batchUpdate.setAtomic(false);
+ ImmutableSet<String> refs =
+ prefix == null
+ ? refsToDelete
+ : refsToDelete
+ .stream()
+ .map(ref -> ref.startsWith(R_REFS) ? ref : prefix + ref)
+ .collect(toImmutableSet());
+ for (String ref : refs) {
+ batchUpdate.addCommand(createDeleteCommand(projectState, repository, ref));
}
- }
- if (errorMessages.length() > 0) {
- throw new ResourceConflictException(errorMessages.toString());
+ try (RevWalk rw = new RevWalk(repository)) {
+ batchUpdate.execute(rw, NullProgressMonitor.INSTANCE);
+ }
+ StringBuilder errorMessages = new StringBuilder();
+ for (ReceiveCommand command : batchUpdate.getCommands()) {
+ if (command.getResult() == Result.OK) {
+ postDeletion(projectState.getNameKey(), command);
+ } else {
+ appendAndLogErrorMessage(errorMessages, command);
+ }
+ }
+ if (errorMessages.length() > 0) {
+ throw new ResourceConflictException(errorMessages.toString());
+ }
}
}
- private ReceiveCommand createDeleteCommand(ProjectResource project, Repository r, String refName)
+ private ReceiveCommand createDeleteCommand(
+ ProjectState projectState, Repository r, String refName)
throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
Ref ref = r.getRefDatabase().getRef(refName);
ReceiveCommand command;
@@ -227,7 +252,7 @@
try {
permissionBackend
.currentUser()
- .project(project.getNameKey())
+ .project(projectState.getNameKey())
.ref(refName)
.check(RefPermission.DELETE);
} catch (AuthException denied) {
@@ -237,12 +262,12 @@
}
}
- if (!project.getProjectState().statePermitsWrite()) {
+ if (!projectState.statePermitsWrite()) {
command.setResult(Result.REJECTED_OTHER_REASON, "project state does not permit write");
}
if (!refName.startsWith(R_TAGS)) {
- Branch.NameKey branchKey = new Branch.NameKey(project.getNameKey(), ref.getName());
+ Branch.NameKey branchKey = new Branch.NameKey(projectState.getNameKey(), ref.getName());
if (!queryProvider.get().setLimit(1).byBranchOpen(branchKey).isEmpty()) {
command.setResult(Result.REJECTED_OTHER_REASON, "it has open changes");
}
@@ -252,7 +277,7 @@
u.setForceUpdate(true);
u.setExpectedOldObjectId(r.exactRef(refName).getObjectId());
u.setNewObjectId(ObjectId.zeroId());
- refDeletionValidator.validateRefOperation(project.getName(), identifiedUser.get(), u);
+ refDeletionValidator.validateRefOperation(projectState.getName(), identifiedUser.get(), u);
return command;
}
@@ -281,7 +306,7 @@
errorMessages.append("\n");
}
- private void postDeletion(ProjectResource project, ReceiveCommand cmd) {
- referenceUpdated.fire(project.getNameKey(), cmd, identifiedUser.get().state());
+ private void postDeletion(Project.NameKey project, ReceiveCommand cmd) {
+ referenceUpdated.fire(project, cmd, identifiedUser.get().state());
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteTag.java b/java/com/google/gerrit/server/restapi/project/DeleteTag.java
index bd5f444..f7cce11 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteTag.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteTag.java
@@ -21,9 +21,7 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
-import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.RefUtil;
import com.google.gerrit.server.project.TagResource;
import com.google.gwtorm.server.OrmException;
@@ -34,13 +32,11 @@
@Singleton
public class DeleteTag implements RestModifyView<TagResource, Input> {
- private final PermissionBackend permissionBackend;
- private final DeleteRef.Factory deleteRefFactory;
+ private final DeleteRef deleteRef;
@Inject
- DeleteTag(PermissionBackend permissionBackend, DeleteRef.Factory deleteRefFactory) {
- this.permissionBackend = permissionBackend;
- this.deleteRefFactory = deleteRefFactory;
+ DeleteTag(DeleteRef deleteRef) {
+ this.deleteRef = deleteRef;
}
@Override
@@ -53,13 +49,7 @@
throw new MethodNotAllowedException("not allowed to delete " + tag);
}
- permissionBackend
- .currentUser()
- .project(resource.getNameKey())
- .ref(tag)
- .check(RefPermission.DELETE);
- resource.getProjectState().checkStatePermitsWrite();
- deleteRefFactory.create(resource).ref(tag).delete();
+ deleteRef.deleteSingleRef(resource.getProjectState(), tag);
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteTags.java b/java/com/google/gerrit/server/restapi/project/DeleteTags.java
index 83fa1ea..bf2c524 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteTags.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteTags.java
@@ -16,6 +16,7 @@
import static org.eclipse.jgit.lib.Constants.R_TAGS;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.api.projects.DeleteTagsInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,11 +31,11 @@
@Singleton
public class DeleteTags implements RestModifyView<ProjectResource, DeleteTagsInput> {
- private final DeleteRef.Factory deleteRefFactory;
+ private final DeleteRef deleteRef;
@Inject
- DeleteTags(DeleteRef.Factory deleteRefFactory) {
- this.deleteRefFactory = deleteRefFactory;
+ DeleteTags(DeleteRef deleteRef) {
+ this.deleteRef = deleteRef;
}
@Override
@@ -43,7 +44,8 @@
if (input == null || input.tags == null || input.tags.isEmpty()) {
throw new BadRequestException("tags must be specified");
}
- deleteRefFactory.create(project).refs(input.tags).prefix(R_TAGS).delete();
+ deleteRef.deleteMultipleRefs(
+ project.getProjectState(), ImmutableSet.copyOf(input.tags), R_TAGS);
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/Module.java b/java/com/google/gerrit/server/restapi/project/Module.java
index f2047e0..a57438e 100644
--- a/java/com/google/gerrit/server/restapi/project/Module.java
+++ b/java/com/google/gerrit/server/restapi/project/Module.java
@@ -104,7 +104,6 @@
put(PROJECT_KIND, "config").to(PutConfig.class);
post(COMMIT_KIND, "cherrypick").to(CherryPickCommit.class);
- factory(DeleteRef.Factory.class);
factory(ProjectNode.Factory.class);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java
index 008997b..330f2b8 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.api.projects.ProjectApi;
+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.reviewdb.client.RefNames;
@@ -64,7 +65,24 @@
}
@Test
- public void deleteBranchesForbidden() throws Exception {
+ public void deleteOneBranchWithoutPermissionForbidden() throws Exception {
+ ImmutableList<String> branchToDelete = ImmutableList.of("refs/heads/test-1");
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = branchToDelete;
+ setApiUser(user);
+ try {
+ project().deleteBranches(input);
+ fail("Expected AuthException");
+ } catch (AuthException e) {
+ assertThat(e).hasMessageThat().isEqualTo("delete not permitted for refs/heads/test-1");
+ }
+ setApiUser(admin);
+ assertBranches(BRANCHES);
+ }
+
+ @Test
+ public void deleteMultiBranchesWithoutPermissionForbidden() throws Exception {
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = BRANCHES;
setApiUser(user);
