DeleteRef: expose separate methods for deleting single/multi refs
This commit removes DeleteRef#delete and expose two methods
for deleting one single ref and multiple refs respectively.
This makes the intention of the caller more clear and thus
easier to read.
The existing code doesn't check whether the user has DELETE
permission when the user tries to delete a single ref through
"Delete Branches" REST API. "Delete Branches" assumes
permissions being checked in DeleteRef#delete but when there is
only one ref, the deletion is delegated to
DeleteRef#deleteSingleRef which doesn't perform any permission
check. This will be fixed in the child commit.
Change-Id: I3d0aacb0bc7a9adc9b753b9756ca0333c95c7e76
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
index cd0142e..49dda2c 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java
@@ -17,7 +17,6 @@
import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef;
import static org.eclipse.jgit.lib.Constants.R_HEADS;
-import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -68,7 +67,7 @@
throw new ResourceConflictException("branch " + rsrc.getBranchKey() + " has open changes");
}
- deleteRef.delete(rsrc.getProjectState(), ImmutableSet.of(rsrc.getRef()), R_HEADS);
+ 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 01f63ef..6e60193 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java
@@ -44,7 +44,8 @@
if (input == null || input.branches == null || input.branches.isEmpty()) {
throw new BadRequestException("branches must be specified");
}
- deleteRef.delete(project.getProjectState(), ImmutableSet.copyOf(input.branches), R_HEADS);
+ 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 38e033b..043417e 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteRef.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
@@ -22,7 +22,6 @@
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;
@@ -84,111 +83,137 @@
this.queryProvider = queryProvider;
}
- public void delete(ProjectState projectState, ImmutableSet<String> refsToDelete)
- throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
- delete(projectState, refsToDelete, null);
+ /**
+ * 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 {
+ deleteSingleRef(projectState, ref, null);
}
- public void delete(
- ProjectState projectState, ImmutableSet<String> refsToDelete, @Nullable String prefix)
- throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
- if (!refsToDelete.isEmpty()) {
- try (Repository r = repoManager.openRepository(projectState.getNameKey())) {
- if (refsToDelete.size() == 1) {
- deleteSingleRef(
- r, projectState.getNameKey(), Iterables.getOnlyElement(refsToDelete), prefix);
- } else {
- deleteMultipleRefs(r, projectState, refsToDelete, prefix);
- }
- }
- }
- }
-
- private void deleteSingleRef(Repository r, Project.NameKey project, String ref, String prefix)
+ /**
+ * 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 {
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(project.get(), 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) {
+
+ 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(project, 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, ProjectState projectState, ImmutableSet<String> refsToDelete, String prefix)
+ /**
+ * 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 {
- BatchRefUpdate batchUpdate = r.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, r, ref));
+ if (refsToDelete.isEmpty()) {
+ return;
}
- try (RevWalk rw = new RevWalk(r)) {
- 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);
+
+ 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());
+ }
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteTag.java b/java/com/google/gerrit/server/restapi/project/DeleteTag.java
index 079fed4..f4eced7 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteTag.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteTag.java
@@ -16,7 +16,6 @@
import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef;
-import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.Response;
@@ -60,7 +59,7 @@
.ref(tag)
.check(RefPermission.DELETE);
resource.getProjectState().checkStatePermitsWrite();
- deleteRef.delete(resource.getProjectState(), ImmutableSet.of(tag));
+ 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 5f10898..bf2c524 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteTags.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteTags.java
@@ -44,7 +44,8 @@
if (input == null || input.tags == null || input.tags.isEmpty()) {
throw new BadRequestException("tags must be specified");
}
- deleteRef.delete(project.getProjectState(), ImmutableSet.copyOf(input.tags), R_TAGS);
+ deleteRef.deleteMultipleRefs(
+ project.getProjectState(), ImmutableSet.copyOf(input.tags), R_TAGS);
return Response.none();
}
}
