Ban synchronous ref-delete via apply-object(s) Ref deletion is a non-fast-forward operation and it may have different outcome with other forced operations when not executed in the correct order. Ban the use of ref-delete via apply-object returning a 400 and force the client to run an asynchronous fetch instead which would also go through global-refdb validation. For example, a forced update and a delete cannot be swapped. Bug: Issue 367651566 Change-Id: I978c3079fbb9db4ea5b4fcbb63df711430ad0ce8
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 d9b47d3..b4c6ead 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
@@ -367,7 +367,7 @@ .map(ref -> toBatchApplyObject(project, ref, state)) .collect(Collectors.toList()); - if (!containsLargeRef(refsBatch)) { + if (!containsLargeOrDeletedRefs(refsBatch)) { return ((source) -> callBatchSendObject(source, project, refsBatch, eventCreatedOn, state)); } } catch (UncheckedIOException e) { @@ -378,14 +378,10 @@ private BatchApplyObjectData toBatchApplyObject( NameKey project, ReferenceUpdatedEvent event, ReplicationState state) { - if (event.isDelete()) { - Optional<RevisionData> noRevisionData = Optional.empty(); - return BatchApplyObjectData.create(event.refName(), noRevisionData, event.isDelete()); - } try { Optional<RevisionData> maybeRevisionData = revReaderProvider.get().read(project, event.objectId(), event.refName(), 0); - return BatchApplyObjectData.create(event.refName(), maybeRevisionData, event.isDelete()); + return BatchApplyObjectData.create(event.refName(), maybeRevisionData); } catch (IOException e) { stateLog.error( String.format( @@ -397,8 +393,8 @@ } } - private boolean containsLargeRef(List<BatchApplyObjectData> batchApplyObjectData) { - return batchApplyObjectData.stream().anyMatch(e -> e.revisionData().isEmpty() && !e.isDelete()); + private boolean containsLargeOrDeletedRefs(List<BatchApplyObjectData> batchApplyObjectData) { + return batchApplyObjectData.stream().anyMatch(e -> e.revisionData().isEmpty()); } private Optional<HttpResult> callSendObject( @@ -408,7 +404,6 @@ NameKey project, String refName, long eventCreatedOn, - boolean isDelete, List<RevisionData> revision) throws IOException { String revisionDataStr = @@ -423,9 +418,7 @@ revisionDataStr); Context<String> apiTimer = applyObjectMetrics.startEnd2End(remoteName); HttpResult result = - isDelete - ? fetchClient.callSendObject(project, refName, eventCreatedOn, isDelete, null, uri) - : fetchClient.callSendObjects(project, refName, eventCreatedOn, revision, uri); + fetchClient.callSendObjects(project, refName, eventCreatedOn, revision, uri); repLog.info( "Pull replication REST API apply object to {} COMPLETED for {}:{} - {}, HTTP Result:" + " {} - time:{} ms", @@ -500,7 +493,6 @@ project, batchApplyObject.refName(), eventCreatedOn, - batchApplyObject.isDelete(), batchApplyObject.revisionData().map(ImmutableList::of).orElse(null)); resultSuccessful = HttpResultUtils.isSuccessful(result); @@ -533,14 +525,7 @@ Optional<HttpResult> sendObjectResult = callSendObject( - fetchClient, - remoteName, - uri, - project, - refName, - eventCreatedOn, - batchApplyObject.isDelete(), - allRevisions); + fetchClient, remoteName, uri, project, refName, eventCreatedOn, allRevisions); resultSuccessful = HttpResultUtils.isSuccessful(sendObjectResult); if (!resultSuccessful) { break;
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 01d32fb..402336d 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
@@ -34,22 +34,17 @@ import com.googlesource.gerrit.plugins.replication.pull.api.exception.RefUpdateException; import java.io.IOException; import java.util.Objects; -import javax.servlet.http.HttpServletResponse; @Singleton public class ApplyObjectAction implements RestModifyView<ProjectResource, RevisionInput> { private final ApplyObjectCommand applyObjectCommand; - private final DeleteRefCommand deleteRefCommand; private final FetchPreconditions preConditions; @Inject public ApplyObjectAction( - ApplyObjectCommand applyObjectCommand, - DeleteRefCommand deleteRefCommand, - FetchPreconditions preConditions) { + ApplyObjectCommand applyObjectCommand, FetchPreconditions preConditions) { this.applyObjectCommand = applyObjectCommand; - this.deleteRefCommand = deleteRefCommand; this.preConditions = preConditions; } @@ -65,6 +60,9 @@ if (Strings.isNullOrEmpty(input.getRefName())) { throw new BadRequestException("Ref-update refname cannot be null or empty"); } + if (Objects.isNull(input.getRevisionData())) { + throw new BadRequestException("Revision data cannot be null"); + } try { repLog.info( @@ -74,17 +72,6 @@ input.getRefName(), input.getRevisionData()); - if (Objects.isNull(input.getRevisionData())) { - deleteRefCommand.deleteRef(resource.getNameKey(), input.getRefName(), input.getLabel()); - repLog.info( - "Apply object API - REF DELETED - from {} for {}:{} - {}", - input.getLabel(), - resource.getNameKey(), - input.getRefName(), - input.getRevisionData()); - return Response.withStatusCode(HttpServletResponse.SC_NO_CONTENT, ""); - } - try { input.validate(); } catch (IllegalArgumentException e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectsAction.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectsAction.java index 1088e40..811cb27 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectsAction.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectsAction.java
@@ -34,21 +34,15 @@ import java.io.IOException; import java.util.Arrays; import java.util.Objects; -import javax.servlet.http.HttpServletResponse; public class ApplyObjectsAction implements RestModifyView<ProjectResource, RevisionsInput> { private final ApplyObjectCommand command; - private final DeleteRefCommand deleteRefCommand; private final FetchPreconditions preConditions; @Inject - public ApplyObjectsAction( - ApplyObjectCommand command, - DeleteRefCommand deleteRefCommand, - FetchPreconditions preConditions) { + public ApplyObjectsAction(ApplyObjectCommand command, FetchPreconditions preConditions) { this.command = command; - this.deleteRefCommand = deleteRefCommand; this.preConditions = preConditions; } @@ -65,6 +59,9 @@ if (Strings.isNullOrEmpty(input.getRefName())) { throw new BadRequestException("Ref-update refname cannot be null or empty"); } + if (Objects.isNull(input.getRevisionsData())) { + throw new BadRequestException("Revision data cannot be null"); + } repLog.info( "Apply object API from {} for {}:{} - {}", @@ -73,16 +70,6 @@ input.getRefName(), Arrays.toString(input.getRevisionsData())); - if (Objects.isNull(input.getRevisionsData())) { - deleteRefCommand.deleteRef(resource.getNameKey(), input.getRefName(), input.getLabel()); - repLog.info( - "Apply object API - REF DELETED - from {} for {}:{}", - input.getLabel(), - resource.getNameKey(), - input.getRefName()); - return Response.withStatusCode(HttpServletResponse.SC_NO_CONTENT, ""); - } - try { input.validate(); } catch (IllegalArgumentException e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java index 7a613c0..5b17002 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java
@@ -20,14 +20,9 @@ @AutoValue public abstract class BatchApplyObjectData { - public static BatchApplyObjectData create( - String refName, Optional<RevisionData> revisionData, boolean isDelete) + public static BatchApplyObjectData create(String refName, Optional<RevisionData> revisionData) throws IllegalArgumentException { - if (isDelete && revisionData.isPresent()) { - throw new IllegalArgumentException( - "DELETE ref-updates cannot be associated with a RevisionData"); - } - return new AutoValue_BatchApplyObjectData(refName, revisionData, isDelete); + return new AutoValue_BatchApplyObjectData(refName, revisionData, false); } public abstract String refName();
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 a4fc41b..1d3acc6 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
@@ -83,7 +83,6 @@ + "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; @@ -91,7 +90,7 @@ public void setup() throws Exception { when(preConditions.canCallFetchApi()).thenReturn(true); - applyObjectAction = new ApplyObjectAction(applyObjectCommand, deleteRefCommand, preConditions); + applyObjectAction = new ApplyObjectAction(applyObjectCommand, preConditions); } @Test
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectDataTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectDataTest.java deleted file mode 100644 index bf74b56..0000000 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectDataTest.java +++ /dev/null
@@ -1,32 +0,0 @@ -// Copyright (C) 2023 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.data; - -import java.util.Optional; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; - -@RunWith(MockitoJUnitRunner.class) -public class BatchApplyObjectDataTest { - - @Mock private RevisionData revisionData; - - @Test(expected = IllegalArgumentException.class) - public void shouldFailIfRevisionDataIsPresentForADelete() { - BatchApplyObjectData.create("foo", Optional.of(revisionData), true); - } -}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java index fa5a349..595fb8f 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java
@@ -623,7 +623,7 @@ List<BatchApplyObjectData> batchApplyObjects = new ArrayList<>(); batchApplyObjects.add( - BatchApplyObjectData.create(refName, Optional.of(createSampleRevisionData("a")), false)); + BatchApplyObjectData.create(refName, Optional.of(createSampleRevisionData("a")))); objectUnderTest.callBatchSendObject( Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api)); @@ -646,8 +646,8 @@ RevisionData revisionA = createSampleRevisionData("a"); RevisionData revisionB = createSampleRevisionData("b"); String refNameB = "refs/heads/b"; - batchApplyObjects.add(BatchApplyObjectData.create(refName, Optional.of(revisionA), false)); - batchApplyObjects.add(BatchApplyObjectData.create(refNameB, Optional.of(revisionB), false)); + batchApplyObjects.add(BatchApplyObjectData.create(refName, Optional.of(revisionA))); + batchApplyObjects.add(BatchApplyObjectData.create(refNameB, Optional.of(revisionB))); objectUnderTest.callBatchSendObject( Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api)); @@ -682,38 +682,6 @@ assertThat(readPayload(httpPost)).isEqualTo(expectedSendObjectsPayload); } - @Test - public void shouldCallBatchApplyObjectEndpointWithNoRevisionDataForDeletes() throws Exception { - List<BatchApplyObjectData> batchApplyObjects = new ArrayList<>(); - batchApplyObjects.add(BatchApplyObjectData.create(refName, Optional.empty(), true)); - - objectUnderTest.callBatchSendObject( - Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api)); - - verify(httpClient, times(1)).execute(httpPostCaptor.capture(), any()); - - HttpPost httpPost = httpPostCaptor.getValue(); - - String expectedSendObjectsPayload = - "[{\"label\":\"Replication\",\"ref_name\":\"" - + refName - + "\",\"event_created_on\":" - + eventCreatedOn - + "}]"; - assertThat(readPayload(httpPost)).isEqualTo(expectedSendObjectsPayload); - } - - @Test(expected = IllegalArgumentException.class) - public void shouldThrowExceptionIfDeleteFlagIsSetButRevisionDataIsPresentForBatchSendEndpoint() - throws Exception { - List<BatchApplyObjectData> batchApplyObjects = new ArrayList<>(); - batchApplyObjects.add( - BatchApplyObjectData.create(refName, Optional.of(createSampleRevisionData()), true)); - - objectUnderTest.callBatchSendObject( - Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api)); - } - public String readPayload(HttpPost entity) throws Exception { ByteBuffer buf = IO.readWholeStream(entity.getEntity().getContent(), 1024); return RawParseUtils.decode(buf.array(), buf.arrayOffset(), buf.limit()).trim();