Fail apply-object on change /meta when missing patch-set The execution of apply-object of changes /meta ref has no guarantee to be executed after the successful replication of the associated patch-set. Updating the repository with the change /meta pointing to a missing patch-set, albeit legal from a repo's perspective, it would cause errors in Gerrit at multiple levels: - the change screen would not render and produce 500s - the reindex may fail - the comments may succeed on repo but fail to be reindexed It is way better to fail the apply-object and let the replication to fallback to async fetch rather than putting the change in an inconsistent state. Return an HTTP status 412 (PRECONDITION FAILED) with the patch-set ref that was a precondition for the apply-object of the /meta ref to be executed. Change-Id: I607cc62af96dc2aae3972673ebd94f4a7fee603d
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 72f0266..381e560 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
@@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.PreconditionFailedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; @@ -27,6 +28,7 @@ import com.google.gerrit.server.project.ProjectResource; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionInput; +import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingLatestPatchSetException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.RefUpdateException; import java.io.IOException; @@ -130,6 +132,15 @@ input.getRevisionData(), e); throw new UnprocessableEntityException(e.getMessage()); + } catch (MissingLatestPatchSetException e) { + repLog.error( + "Apply object API *FAILED* from {} for {}:{} - {}", + input.getLabel(), + resource.getNameKey(), + input.getRefName(), + input.getRevisionData(), + e); + throw new PreconditionFailedException(e.getMessage()); } } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectCommand.java index 968a03c..5fef17d 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectCommand.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectCommand.java
@@ -34,6 +34,7 @@ import com.googlesource.gerrit.plugins.replication.pull.SourcesCollection; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionData; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionObjectData; +import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingLatestPatchSetException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.RefUpdateException; import com.googlesource.gerrit.plugins.replication.pull.fetch.ApplyObject; @@ -84,7 +85,8 @@ RevisionData revisionsData, String sourceLabel, long eventCreatedOn) - throws IOException, RefUpdateException, MissingParentObjectException { + throws IOException, RefUpdateException, MissingParentObjectException, + MissingLatestPatchSetException { applyObjects(name, refName, new RevisionData[] {revisionsData}, sourceLabel, eventCreatedOn); } @@ -94,7 +96,8 @@ RevisionData[] revisionsData, String sourceLabel, long eventCreatedOn) - throws IOException, RefUpdateException, MissingParentObjectException { + throws IOException, RefUpdateException, MissingParentObjectException, + MissingLatestPatchSetException { repLog.info( "Apply object from {} for {}:{} - {}",
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 817ea00..c4c4e83 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
@@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.PreconditionFailedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; @@ -27,6 +28,7 @@ import com.google.gerrit.server.project.ProjectResource; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionsInput; +import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingLatestPatchSetException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.RefUpdateException; import java.io.IOException; @@ -130,6 +132,15 @@ Arrays.toString(input.getRevisionsData()), e); throw new UnprocessableEntityException(e.getMessage()); + } catch (MissingLatestPatchSetException e) { + repLog.error( + "Apply object API *FAILED* from {} for {}:{} - {}", + input.getLabel(), + resource.getNameKey(), + input.getRefName(), + Arrays.toString(input.getRevisionsData()), + e); + throw new PreconditionFailedException(e.getMessage()); } } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/MissingLatestPatchSetException.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/MissingLatestPatchSetException.java new file mode 100644 index 0000000..458c74a --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/MissingLatestPatchSetException.java
@@ -0,0 +1,26 @@ +// 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.exception; + +import com.google.gerrit.entities.Project; + +public class MissingLatestPatchSetException extends Exception { + private static final long serialVersionUID = 1L; + + public MissingLatestPatchSetException( + Project.NameKey project, String refName, String errorMessage) { + super(String.format("%s for project %s ref name: %s", errorMessage, project.get(), refName)); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObject.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObject.java index 36356e9..6bb1373 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObject.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObject.java
@@ -20,6 +20,7 @@ import com.googlesource.gerrit.plugins.replication.pull.LocalGitRepositoryManagerProvider; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionData; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionObjectData; +import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingLatestPatchSetException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import java.io.IOException; import org.eclipse.jgit.lib.ObjectId; @@ -43,7 +44,7 @@ } public RefUpdateState apply(Project.NameKey name, RefSpec refSpec, RevisionData[] revisionsData) - throws MissingParentObjectException, IOException { + throws MissingParentObjectException, IOException, MissingLatestPatchSetException { try (Repository git = gitManager.openRepository(name)) { ObjectId refHead = null; @@ -61,6 +62,13 @@ throw new MissingParentObjectException(name, refSpec.getSource(), parent.getId()); } } + + StringBuffer error = new StringBuffer(); + if (!ChangeMetaCommitValidator.isValid( + git, refSpec.getSource(), commit, error::append)) { + throw new MissingLatestPatchSetException(name, refSpec.getSource(), error.toString()); + } + refHead = newObjectID = oi.insert(commitObject.getType(), commitObject.getContent()); RevisionObjectData treeObject = revisionData.getTreeObject();
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ChangeMetaCommitValidator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ChangeMetaCommitValidator.java new file mode 100644 index 0000000..a5fec55 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ChangeMetaCommitValidator.java
@@ -0,0 +1,64 @@ +// Copyright (C) 2020 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.fetch; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.RefNames; +import java.io.IOException; +import java.util.List; +import java.util.Optional; +import java.util.OptionalInt; +import java.util.function.Consumer; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.FooterKey; +import org.eclipse.jgit.revwalk.RevCommit; + +class ChangeMetaCommitValidator { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private static final FooterKey FOOTER_CHANGE_META_PATCH_SET = new FooterKey("Patch-set"); + + public static boolean isValid( + Repository repo, String refName, RevCommit commit, Consumer<String> errorCallback) + throws IOException { + if (!refName.startsWith(RefNames.REFS_CHANGES) || !refName.endsWith(RefNames.META_SUFFIX)) { + return true; + } + + List<String> patchSetFooter = commit.getFooterLines(FOOTER_CHANGE_META_PATCH_SET); + OptionalInt latestPatchSet = patchSetFooter.stream().mapToInt(Integer::parseInt).max(); + + if (latestPatchSet.isEmpty()) { + return true; + } + + String patchSetRef = refName.replace(RefNames.META_SUFFIX, "/" + latestPatchSet.getAsInt()); + Optional<ObjectId> patchSetObjectId = + Optional.ofNullable(repo.exactRef(patchSetRef)).map(Ref::getObjectId); + + if (patchSetObjectId.isEmpty()) { + errorCallback.accept("Unable to find latest patch-set ref " + patchSetRef); + return false; + } + + RevCommit patchSetCommit = repo.parseCommit(patchSetObjectId.get()); + logger.atFine().log( + "Change ref %s has latest patch-set %d and is successfully resolved to %s with commit %s", + refName, latestPatchSet.getAsInt(), patchSetObjectId.get().getName(), patchSetCommit); + + return true; + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java index a46846e..2d94a96 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java
@@ -229,4 +229,9 @@ secureConfig.setString("remote", remoteName, "password", password); secureConfig.save(); } + + protected String firstPatchSetForChangeMetaRef(String metaRefName) { + String patchSetRefName = metaRefName.replace(RefNames.META_SUFFIX, "/1"); + return patchSetRefName; + } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java index 652daed..edbb85f 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ApplyObjectActionIT.java
@@ -35,7 +35,7 @@ + TEST_REPLICATION_REMOTE + "\",\"ref_name\":\"%s\",\"revision_data\":{\"commit_object\":{\"sha1\":\"%s\",\"type\":1,\"content\":\"%s\"},\"tree_object\":{\"type\":2,\"content\":\"%s\"},\"blobs\":[]}, \"async\":true}"; - String refName = createRef(); + String refName = firstPatchSetForChangeMetaRef(createRef()); Optional<RevisionData> revisionDataOption = createRevisionData(refName); assertThat(revisionDataOption.isPresent()).isTrue(); @@ -57,7 +57,7 @@ + TEST_REPLICATION_REMOTE + "\",\"ref_name\":\"%s\",\"revision_data\":{\"commit_object\":{\"sha1\":\"%s\",\"type\":1,\"content\":\"%s\"},\"tree_object\":{\"type\":2,\"content\":\"%s\"},\"blobs\":[]}}"; - String refName = createRef(); + String refName = firstPatchSetForChangeMetaRef(createRef()); Optional<RevisionData> revisionDataOption = createRevisionData(refName); assertThat(revisionDataOption.isPresent()).isTrue(); @@ -225,7 +225,7 @@ + TEST_REPLICATION_REMOTE + "\",\"ref_name\":\"%s\",\"revision_data\":{\"commit_object\":{\"sha1\":\"%s\",\"type\":1,\"content\":\"%s\"},\"tree_object\":{\"type\":2,\"content\":\"%s\"},\"blobs\":[]}}"; - String refName = createRef(); + String refName = firstPatchSetForChangeMetaRef(createRef()); Optional<RevisionData> revisionDataOption = createRevisionData(refName); assertThat(revisionDataOption.isPresent()).isTrue();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObjectIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObjectIT.java index c75d32a..b562933 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObjectIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/fetch/ApplyObjectIT.java
@@ -28,6 +28,7 @@ import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Patch; +import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.Project.NameKey; import com.google.gerrit.entities.RefNames; @@ -42,6 +43,7 @@ import com.googlesource.gerrit.plugins.replication.pull.RevisionReader; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionData; import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionObjectData; +import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingLatestPatchSetException; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import java.util.List; import java.util.Optional; @@ -75,7 +77,9 @@ testRepo = cloneProject(createTestProject(testRepoProjectName)); Result pushResult = createChange(); - String refName = RefNames.changeMetaRef(pushResult.getChange().getId()); + Change.Id changeId = pushResult.getChange().getId(); + String refName = RefNames.changeMetaRef(changeId); + String patchSetRefName = RefNames.patchSetRef(PatchSet.id(changeId, 1)); RefSpec refSpec = new RefSpec(refName); Optional<RevisionData> revisionData; @@ -83,6 +87,7 @@ try (Repository repo = repoManager.openRepository(testRepoKey)) { revisionData = reader.read(testRepoKey, repo.exactRef(refName).getObjectId(), refName, 0); + objectUnderTest.apply(project, new RefSpec(patchSetRefName), toArray(revisionData)); objectUnderTest.apply(project, refSpec, toArray(revisionData)); } @@ -124,6 +129,7 @@ Result pushResult = createChange(); Change.Id changeId = pushResult.getChange().getId(); + String patchSetRefname = RefNames.patchSetRef(PatchSet.id(changeId, 1)); String refName = RefNames.changeMetaRef(changeId); RefSpec refSpec = new RefSpec(refName); @@ -131,6 +137,7 @@ try (Repository repo = repoManager.openRepository(testRepoKey)) { Optional<RevisionData> revisionData = reader.read(testRepoKey, repo.exactRef(refName).getObjectId(), refName, 0); + objectUnderTest.apply(project, new RefSpec(patchSetRefname), toArray(revisionData)); objectUnderTest.apply(project, refSpec, toArray(revisionData)); } @@ -181,6 +188,27 @@ } } + @Test + public void shouldThrowExceptionWhenPatchSetIsMissing() throws Exception { + String testRepoProjectName = project + TEST_REPLICATION_SUFFIX; + NameKey createTestProject = createTestProject(testRepoProjectName); + try (Repository repo = repoManager.openRepository(createTestProject)) { + testRepo = cloneProject(createTestProject); + + Result pushResult = createChange(); + Change.Id changeId = pushResult.getChange().getId(); + String refName = RefNames.changeMetaRef(changeId); + + Optional<RevisionData> revisionData = + reader.read(createTestProject, repo.exactRef(refName).getObjectId(), refName, 0); + + RefSpec refSpec = new RefSpec(refName); + assertThrows( + MissingLatestPatchSetException.class, + () -> objectUnderTest.apply(project, refSpec, toArray(revisionData))); + } + } + private void compareObjects(RevisionData expected, Optional<RevisionData> actualOption) { assertThat(actualOption.isPresent()).isTrue(); RevisionData actual = actualOption.get();