Manage removal of refs Implement the apply-object API where the revision data is null, interpreting the call as a ref delete. The approach is taken because of the need of backward compatiblity with existing REST-API on the stable-3.3 branch. This fixes the problem of the lack of replication for delete changes. Bug: Issue 15618 Change-Id: I305ebcae3e997f64c8d2f5f452114f5d771399b5
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java index c4b50b5..7449263 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java
@@ -140,7 +140,11 @@ event.getRefName(), event.getOldObjectId(), event.getNewObjectId()); - fire(event.getProjectName(), ObjectId.fromString(event.getNewObjectId()), event.getRefName()); + fire( + event.getProjectName(), + ObjectId.fromString(event.getNewObjectId()), + event.getRefName(), + event.isDelete()); } } @@ -169,26 +173,32 @@ return !refsFilter.match(refName); } - private void fire(String projectName, ObjectId objectId, String refName) { + private void fire(String projectName, ObjectId objectId, String refName, boolean isDelete) { ReplicationState state = new ReplicationState(new GitUpdateProcessing(dispatcher.get())); - fire(Project.nameKey(projectName), objectId, refName, state); + fire(Project.nameKey(projectName), objectId, refName, isDelete, state); state.markAllFetchTasksScheduled(); } private void fire( - Project.NameKey project, ObjectId objectId, String refName, ReplicationState state) { + Project.NameKey project, + ObjectId objectId, + String refName, + boolean isDelete, + ReplicationState state) { if (!running) { stateLog.warn( "Replication plugin did not finish startup before event, event replication is postponed", state); - beforeStartupEventsQueue.add(ReferenceUpdatedEvent.create(project.get(), refName, objectId)); + beforeStartupEventsQueue.add( + ReferenceUpdatedEvent.create(project.get(), refName, objectId, isDelete)); return; } ForkJoinPool fetchCallsPool = null; try { fetchCallsPool = new ForkJoinPool(sources.get().getAll().size()); - final Consumer<Source> callFunction = callFunction(project, objectId, refName, state); + final Consumer<Source> callFunction = + callFunction(project, objectId, refName, isDelete, state); fetchCallsPool .submit(() -> sources.get().getAll().parallelStream().forEach(callFunction)) .get(fetchCallsTimeout, TimeUnit.MILLISECONDS); @@ -207,8 +217,12 @@ } private Consumer<Source> callFunction( - NameKey project, ObjectId objectId, String refName, ReplicationState state) { - CallFunction call = getCallFunction(project, objectId, refName, state); + NameKey project, + ObjectId objectId, + String refName, + boolean isDelete, + ReplicationState state) { + CallFunction call = getCallFunction(project, objectId, refName, isDelete, state); return (source) -> { try { @@ -220,11 +234,20 @@ } private CallFunction getCallFunction( - NameKey project, ObjectId objectId, String refName, ReplicationState state) { + NameKey project, + ObjectId objectId, + String refName, + boolean isDelete, + ReplicationState state) { + if (isDelete) { + return ((source) -> callSendObject(source, project, refName, isDelete, null, state)); + } + try { Optional<RevisionData> revisionData = revisionReader.read(project, objectId, refName); if (revisionData.isPresent()) { - return ((source) -> callSendObject(source, project, refName, revisionData.get(), state)); + return ((source) -> + callSendObject(source, project, refName, isDelete, revisionData.get(), state)); } } catch (InvalidObjectIdException | IOException e) { stateLog.error( @@ -242,6 +265,7 @@ Source source, Project.NameKey project, String refName, + boolean isDelete, RevisionData revision, ReplicationState state) throws MissingParentObjectException { @@ -251,7 +275,7 @@ URIish uri = new URIish(apiUrl); FetchRestApiClient fetchClient = fetchClientFactory.create(source); - HttpResult result = fetchClient.callSendObject(project, refName, revision, uri); + HttpResult result = fetchClient.callSendObject(project, refName, isDelete, revision, uri); if (isProjectMissing(result, project) && source.isCreateMissingRepositories()) { result = initProject(project, uri, fetchClient, result); } @@ -338,7 +362,7 @@ String eventKey = String.format("%s:%s", event.projectName(), event.refName()); if (!eventsReplayed.contains(eventKey)) { repLog.info("Firing pending task {}", event); - fire(event.projectName(), event.objectId(), event.refName()); + fire(event.projectName(), event.objectId(), event.refName(), event.isDelete()); eventsReplayed.add(eventKey); } } @@ -358,8 +382,10 @@ @AutoValue abstract static class ReferenceUpdatedEvent { - static ReferenceUpdatedEvent create(String projectName, String refName, ObjectId objectId) { - return new AutoValue_ReplicationQueue_ReferenceUpdatedEvent(projectName, refName, objectId); + static ReferenceUpdatedEvent create( + String projectName, String refName, ObjectId objectId, boolean isDelete) { + return new AutoValue_ReplicationQueue_ReferenceUpdatedEvent( + projectName, refName, objectId, isDelete); } public abstract String projectName(); @@ -367,6 +393,8 @@ public abstract String refName(); public abstract ObjectId objectId(); + + public abstract boolean isDelete(); } @FunctionalInterface
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectAction.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectAction.java index f029834..5132f41 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectAction.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectAction.java
@@ -33,11 +33,16 @@ public class ApplyObjectAction implements RestModifyView<ProjectResource, RevisionInput> { private final ApplyObjectCommand command; + private final DeleteRefCommand deleteRefCommand; private final FetchPreconditions preConditions; @Inject - public ApplyObjectAction(ApplyObjectCommand command, FetchPreconditions preConditions) { + public ApplyObjectAction( + ApplyObjectCommand command, + DeleteRefCommand deleteRefCommand, + FetchPreconditions preConditions) { this.command = command; + this.deleteRefCommand = deleteRefCommand; this.preConditions = preConditions; } @@ -56,7 +61,8 @@ } if (Objects.isNull(input.getRevisionData())) { - throw new BadRequestException("Ref-update revision data cannot be null or empty"); + deleteRefCommand.deleteRef(resource.getNameKey(), input.getRefName(), input.getLabel()); + return Response.created(input); } if (Objects.isNull(input.getRevisionData().getCommitObject())
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java new file mode 100644 index 0000000..0c5e016 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java
@@ -0,0 +1,113 @@ +// Copyright (C) 2022 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.googlesource.gerrit.plugins.replication.pull.api; + +import static com.googlesource.gerrit.plugins.replication.pull.PullReplicationLogger.repLog; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.registration.DynamicItem; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.events.EventDispatcher; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.project.ProjectCache; +import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.restapi.project.DeleteRef; +import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.replication.pull.Context; +import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; +import com.googlesource.gerrit.plugins.replication.pull.PullReplicationStateLogger; +import com.googlesource.gerrit.plugins.replication.pull.ReplicationState; +import java.io.IOException; +import java.util.Optional; +import org.eclipse.jgit.lib.RefUpdate; + +public class DeleteRefCommand { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final PullReplicationStateLogger fetchStateLog; + private final DeleteRef deleteRef; + private final DynamicItem<EventDispatcher> eventDispatcher; + private final ProjectCache projectCache; + + @Inject + public DeleteRefCommand( + PullReplicationStateLogger fetchStateLog, + ProjectCache projectCache, + DeleteRef deleteRef, + DynamicItem<EventDispatcher> eventDispatcher) { + this.fetchStateLog = fetchStateLog; + this.projectCache = projectCache; + this.deleteRef = deleteRef; + this.eventDispatcher = eventDispatcher; + } + + public void deleteRef(Project.NameKey name, String refName, String sourceLabel) + throws IOException, RestApiException { + try { + repLog.info("Delete ref from {} for project {}, ref name {}", sourceLabel, name, refName); + Optional<ProjectState> projectState = projectCache.get(name); + if (!projectState.isPresent()) { + throw new ResourceNotFoundException(String.format("Project %s was not found", name)); + } + + try { + Context.setLocalEvent(true); + deleteRef.deleteSingleRef(projectState.get(), refName); + + eventDispatcher + .get() + .postEvent( + new FetchRefReplicatedEvent( + name.get(), + refName, + sourceLabel, + ReplicationState.RefFetchResult.SUCCEEDED, + RefUpdate.Result.FORCED)); + } catch (PermissionBackendException e) { + logger.atSevere().withCause(e).log( + "Unexpected error while trying to delete ref '%s' on project %s and notifying it", + refName, name); + throw RestApiException.wrap(e.getMessage(), e); + } catch (ResourceConflictException e) { + eventDispatcher + .get() + .postEvent( + new FetchRefReplicatedEvent( + name.get(), + refName, + sourceLabel, + ReplicationState.RefFetchResult.FAILED, + RefUpdate.Result.LOCK_FAILURE)); + String message = + String.format( + "RefUpdate lock failure for: sourceLabel=%s, project=%s, refName=%s", + sourceLabel, name, refName); + logger.atSevere().withCause(e).log(message); + fetchStateLog.error(message); + throw e; + } finally { + Context.unsetLocalEvent(); + } + + repLog.info( + "Delete ref from {} for project {}, ref name {} completed", sourceLabel, name, refName); + } catch (PermissionBackendException e) { + throw RestApiException.wrap(e.getMessage(), e); + } + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java index a916a7c..ab3d3c5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java
@@ -21,6 +21,7 @@ import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; import com.google.common.net.MediaType; +import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.restapi.Url; @@ -142,9 +143,19 @@ } public HttpResult callSendObject( - Project.NameKey project, String refName, RevisionData revisionData, URIish targetUri) + Project.NameKey project, + String refName, + boolean isDelete, + @Nullable RevisionData revisionData, + URIish targetUri) throws ClientProtocolException, IOException { + if (!isDelete) { + requireNonNull( + revisionData, "RevisionData MUST not be null when the ref-update is not a DELETE"); + } else { + requireNull(revisionData, "DELETE ref-updates cannot be associated with a RevisionData"); + } RevisionInput input = new RevisionInput(instanceLabel, refName, revisionData); String url = @@ -158,6 +169,12 @@ return httpClientFactory.create(source).execute(post, this, getContext(targetUri)); } + private void requireNull(Object object, String string) { + if (object != null) { + throw new IllegalArgumentException(string); + } + } + @Override public HttpResult handleResponse(HttpResponse response) { Optional<String> responseBody = Optional.empty();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java index d742984..82dc2b8 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java
@@ -16,6 +16,8 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.file.Files.createTempDirectory; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.never; @@ -86,7 +88,7 @@ private Path pluginDataPath; @Before - public void setup() throws IOException, LargeObjectException, RefUpdateException { + public void setup() throws IOException, LargeObjectException { Path sitePath = createTempPath("site"); sitePaths = new SitePaths(sitePath); Path pluginDataPath = createTempPath("data"); @@ -101,7 +103,7 @@ when(rd.get()).thenReturn(sourceCollection); when(revReader.read(any(), any(), anyString())).thenReturn(Optional.of(revisionData)); when(fetchClientFactory.create(any())).thenReturn(fetchRestApiClient); - when(fetchRestApiClient.callSendObject(any(), anyString(), any(), any())) + when(fetchRestApiClient.callSendObject(any(), anyString(), anyBoolean(), any(), any())) .thenReturn(httpResult); when(fetchRestApiClient.callFetch(any(), anyString(), any())).thenReturn(httpResult); when(httpResult.isSuccessful()).thenReturn(true); @@ -117,7 +119,7 @@ objectUnderTest.start(); objectUnderTest.onGitReferenceUpdated(event); - verify(fetchRestApiClient).callSendObject(any(), anyString(), any(), any()); + verify(fetchRestApiClient).callSendObject(any(), anyString(), eq(false), any(), any()); } @Test @@ -152,7 +154,7 @@ objectUnderTest.start(); objectUnderTest.onGitReferenceUpdated(event); - verify(fetchRestApiClient).callSendObject(any(), anyString(), any(), any()); + verify(fetchRestApiClient).callSendObject(any(), anyString(), eq(false), any(), any()); } @Test @@ -189,7 +191,7 @@ when(httpResult.isSuccessful()).thenReturn(false); when(httpResult.isParentObjectMissing()).thenReturn(true); - when(fetchRestApiClient.callSendObject(any(), anyString(), any(), any())) + when(fetchRestApiClient.callSendObject(any(), anyString(), eq(false), any(), any())) .thenReturn(httpResult); objectUnderTest.onGitReferenceUpdated(event); @@ -362,7 +364,7 @@ @Override public String getOldObjectId() { - return null; + return ObjectId.zeroId().getName(); } @Override
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionTest.java index 8738046..56e1429 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionTest.java
@@ -72,6 +72,7 @@ + "Submitted-with: OK: Code-Review: Gerrit User 1000000 <1000000@69ec38f0-350e-4d9c-96d4-bc956f2faaac>"; @Mock ApplyObjectCommand applyObjectCommand; + @Mock DeleteRefCommand deleteRefCommand; @Mock ProjectResource projectResource; @Mock FetchPreconditions preConditions; @@ -79,7 +80,7 @@ public void setup() { when(preConditions.canCallFetchApi()).thenReturn(true); - applyObjectAction = new ApplyObjectAction(applyObjectCommand, preConditions); + applyObjectAction = new ApplyObjectAction(applyObjectCommand, deleteRefCommand, preConditions); } @Test @@ -129,13 +130,6 @@ } @Test(expected = BadRequestException.class) - public void shouldThrowBadRequestExceptionWhenMissingRevisionData() throws Exception { - RevisionInput inputParams = new RevisionInput(label, refName, null); - - applyObjectAction.apply(projectResource, inputParams); - } - - @Test(expected = BadRequestException.class) public void shouldThrowBadRequestExceptionWhenMissingCommitObjectData() throws Exception { RevisionObjectData commitData = new RevisionObjectData(Constants.OBJ_COMMIT, null); RevisionObjectData treeData = new RevisionObjectData(Constants.OBJ_TREE, new byte[] {});
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java new file mode 100644 index 0000000..daf2001 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java
@@ -0,0 +1,77 @@ +// Copyright (C) 2022 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.googlesource.gerrit.plugins.replication.pull.api; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.Project.NameKey; +import com.google.gerrit.extensions.registration.DynamicItem; +import com.google.gerrit.server.events.Event; +import com.google.gerrit.server.events.EventDispatcher; +import com.google.gerrit.server.project.ProjectCache; +import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.restapi.project.DeleteRef; +import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; +import com.googlesource.gerrit.plugins.replication.pull.PullReplicationStateLogger; +import java.util.Optional; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class DeleteRefCommandTest { + private static final String TEST_SOURCE_LABEL = "test-source-label"; + private static final String TEST_REF_NAME = "refs/changes/01/1/1"; + private static final NameKey TEST_PROJECT_NAME = Project.nameKey("test-project"); + + @Mock private PullReplicationStateLogger fetchStateLog; + @Mock private DynamicItem<EventDispatcher> eventDispatcherDataItem; + @Mock private EventDispatcher eventDispatcher; + @Mock private ProjectCache projectCache; + @Mock private DeleteRef deleteRef; + @Mock private ProjectState projectState; + @Captor ArgumentCaptor<Event> eventCaptor; + + private DeleteRefCommand objectUnderTest; + + @Before + public void setup() { + when(eventDispatcherDataItem.get()).thenReturn(eventDispatcher); + when(projectCache.get(any())).thenReturn(Optional.of(projectState)); + + objectUnderTest = + new DeleteRefCommand(fetchStateLog, projectCache, deleteRef, eventDispatcherDataItem); + } + + @Test + public void shouldSendEventWhenDeletingRef() throws Exception { + objectUnderTest.deleteRef(TEST_PROJECT_NAME, TEST_REF_NAME, TEST_SOURCE_LABEL); + + verify(eventDispatcher).postEvent(eventCaptor.capture()); + Event sentEvent = eventCaptor.getValue(); + assertThat(sentEvent).isInstanceOf(FetchRefReplicatedEvent.class); + FetchRefReplicatedEvent fetchEvent = (FetchRefReplicatedEvent) sentEvent; + assertThat(fetchEvent.getProjectNameKey()).isEqualTo(TEST_PROJECT_NAME); + assertThat(fetchEvent.getRefName()).isEqualTo(TEST_REF_NAME); + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java index a778049..394a4a4 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java
@@ -64,6 +64,8 @@ @RunWith(MockitoJUnitRunner.class) public class FetchRestApiClientTest { + private static final boolean IS_REF_UPDATE = false; + @Mock CredentialsProvider credentialProvider; @Mock CredentialsFactory credentials; @Mock HttpClient httpClient; @@ -273,7 +275,11 @@ throws ClientProtocolException, IOException, URISyntaxException { objectUnderTest.callSendObject( - Project.nameKey("test_repo"), refName, createSampleRevisionData(), new URIish(api)); + Project.nameKey("test_repo"), + refName, + IS_REF_UPDATE, + createSampleRevisionData(), + new URIish(api)); verify(httpClient, times(1)).execute(httpPostCaptor.capture(), any(), any()); @@ -288,7 +294,11 @@ throws ClientProtocolException, IOException, URISyntaxException { objectUnderTest.callSendObject( - Project.nameKey("test_repo"), refName, createSampleRevisionData(), new URIish(api)); + Project.nameKey("test_repo"), + refName, + IS_REF_UPDATE, + createSampleRevisionData(), + new URIish(api)); verify(httpClient, times(1)).execute(httpPostCaptor.capture(), any(), any());