Fix validation of code-owner.config files
The validation may be invoked for a commit which is not present in the
repository yet and trying to load it via a new RevWalk fails with
MissingObjectException. Such commits are only visible through the
RevWalk that is creating the commit. This RevWalk is passed in together
with the CommitReceivedEvent. We must use this RevWalk instance to load
the commit from the event.
For example the validation of code-owner.config files failed with
MissingObjectException when a change for refs/meta/config was rebased.
It failed regardless of whether the change actually touched a
code-owners.config file (because we need to load the commit to know if
the code-owners.config file was touched).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I5fc34f0302fbf77908fba56da5d94924a0d05bd8
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
index ababe6f..85933df 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -16,13 +16,12 @@
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.plugins.codeowners.backend.ChangedFiles;
+import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.events.CommitReceivedEvent;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
@@ -39,8 +38,6 @@
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
/** Validates modifications to the {@code code-owners.config} file in {@code refs/meta/config}. */
@Singleton
@@ -48,7 +45,6 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final String pluginName;
- private final GitRepositoryManager repoManager;
private final ProjectConfig.Factory projectConfigFactory;
private final ProjectState.Factory projectStateFactory;
private final ChangedFiles changedFiles;
@@ -61,7 +57,6 @@
@Inject
CodeOwnersPluginConfigValidator(
@PluginName String pluginName,
- GitRepositoryManager repoManager,
ProjectConfig.Factory projectConfigFactory,
ProjectState.Factory projectStateFactory,
ChangedFiles changedFiles,
@@ -71,7 +66,6 @@
RequiredApprovalConfig requiredApprovalConfig,
OverrideApprovalConfig overrideApprovalConfig) {
this.pluginName = pluginName;
- this.repoManager = repoManager;
this.projectConfigFactory = projectConfigFactory;
this.projectStateFactory = projectStateFactory;
this.changedFiles = changedFiles;
@@ -86,18 +80,17 @@
public ImmutableList<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
String fileName = pluginName + ".config";
- Project.NameKey project = receiveEvent.project.getNameKey();
try {
if (!receiveEvent.refName.equals(RefNames.REFS_CONFIG)
- || !isFileChanged(project, receiveEvent.commit, fileName)) {
+ || !isFileChanged(receiveEvent, fileName)) {
// the code-owners.config file in refs/meta/config was not modified, hence we do not need to
// validate it
return ImmutableList.of();
}
- ProjectState projectState = getProjectState(project, receiveEvent.commit);
- ProjectLevelConfig.Bare cfg = loadConfig(project, fileName, receiveEvent.commit);
+ ProjectState projectState = getProjectState(receiveEvent);
+ ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, fileName);
ImmutableList<CommitValidationMessage> validationMessages =
validateConfig(projectState, fileName, cfg.getConfig());
if (!validationMessages.isEmpty()) {
@@ -109,52 +102,57 @@
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
- fileName, receiveEvent.commit.getName(), RefNames.REFS_CONFIG, project);
+ fileName,
+ receiveEvent.commit.getName(),
+ RefNames.REFS_CONFIG,
+ receiveEvent.project.getNameKey());
logger.atSevere().withCause(e).log(errorMessage);
throw new CommitValidationException(errorMessage, e);
}
}
- private ProjectState getProjectState(Project.NameKey projectName, RevCommit commit)
+ private ProjectState getProjectState(CommitReceivedEvent receiveEvent)
throws IOException, ConfigInvalidException {
- try (Repository repo = repoManager.openRepository(projectName)) {
- ProjectConfig projectConfig = projectConfigFactory.create(projectName);
- projectConfig.load(repo, commit);
- return projectStateFactory.create(projectConfig.getCacheable());
- }
+ ProjectConfig projectConfig = projectConfigFactory.create(receiveEvent.project.getNameKey());
+ projectConfig.load(receiveEvent.revWalk, receiveEvent.commit);
+ return projectStateFactory.create(projectConfig.getCacheable());
}
/**
* Whether the given file was changed in the given revision.
*
- * @param project the name of the project
- * @param revision the revision
+ * @param receiveEvent the receive event
* @param fileName the name of the file
*/
- private boolean isFileChanged(Project.NameKey project, ObjectId revision, String fileName)
+ private boolean isFileChanged(CommitReceivedEvent receiveEvent, String fileName)
throws IOException, PatchListNotAvailableException {
- return changedFiles.compute(project, revision).stream()
+ return changedFiles
+ .compute(
+ receiveEvent.project.getNameKey(),
+ receiveEvent.repoConfig,
+ receiveEvent.revWalk,
+ receiveEvent.commit,
+ MergeCommitStrategy.ALL_CHANGED_FILES)
+ .stream()
.anyMatch(changedFile -> changedFile.hasNewPath(JgitPath.of(fileName).getAsAbsolutePath()));
}
/**
* Loads the configuration from the file and revision.
*
- * @param project the project name
+ * @param receiveEvent the receive event
* @param fileName the name of the config file
- * @param revision the revision from which the configuration should be loaded
* @return the loaded configuration
* @throws CommitValidationException thrown if the configuration is invalid and cannot be parsed
*/
- private ProjectLevelConfig.Bare loadConfig(
- Project.NameKey project, String fileName, ObjectId revision)
+ private ProjectLevelConfig.Bare loadConfig(CommitReceivedEvent receiveEvent, String fileName)
throws CommitValidationException, IOException {
ProjectLevelConfig.Bare cfg = new ProjectLevelConfig.Bare(fileName);
- try (Repository git = repoManager.openRepository(project)) {
- cfg.load(project, git, revision);
+ try {
+ cfg.load(receiveEvent.project.getNameKey(), receiveEvent.revWalk, receiveEvent.commit);
} catch (ConfigInvalidException e) {
throw new CommitValidationException(
- exceptionMessage(fileName, revision),
+ exceptionMessage(fileName, receiveEvent.commit),
new CommitValidationMessage(e.getMessage(), ValidationMessage.Type.ERROR));
}
return cfg;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
index 7f6e660..9edbece 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -17,12 +17,19 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.fetch;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.GitUtil;
+import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.RebaseInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
@@ -532,6 +539,128 @@
+ " code-owners.config (parameter codeOwners.maxPathsInChangeMessages) is invalid.");
}
+ @Test
+ public void validationDoesntFailOnRebaseChange_unrelatedChange() throws Exception {
+ // Create two changes for refs/meta/config both with the same parent.
+ GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+ testRepo.reset("config");
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change 1", "a.txt", "content");
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId1 = r.getChangeId();
+
+ testRepo.reset("config");
+ push = pushFactory.create(admin.newIdent(), testRepo, "Change 2", "b.txt", "content");
+ r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId2 = r.getChangeId();
+
+ // Approve and submit the first change
+ approve(changeId1);
+ gApi.changes().id(changeId1).current().submit();
+
+ // Rebase the second change, throws an exception if the code owner plugin config validation
+ // fails.
+ gApi.changes().id(changeId2).rebase();
+
+ // Second change should have 2 patch sets now.
+ ChangeInfo changeInfo = gApi.changes().id(changeId2).get(CURRENT_REVISION);
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision)._number).isEqualTo(2);
+ }
+
+ @Test
+ public void validationDoesntFailOnRebaseChange_changeThatUpdatesTheCodeOwnersConfig()
+ throws Exception {
+ // Create two changes for refs/meta/config both with the same parent.
+ GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+ testRepo.reset("config");
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change 1", "a,txt", "content");
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId1 = r.getChangeId();
+
+ testRepo.reset("config");
+ Config cfg = new Config();
+ cfg.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ push =
+ pushFactory.create(
+ admin.newIdent(), testRepo, "Change 2", "code-owners.config", cfg.toText());
+ r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId2 = r.getChangeId();
+
+ // Approve and submit the first change
+ approve(changeId1);
+ gApi.changes().id(changeId1).current().submit();
+
+ // Rebase the second change, throws an exception if the code owner plugin config validation
+ // fails.
+ gApi.changes().id(changeId2).rebase();
+
+ // Second change should have 2 patch sets now.
+ ChangeInfo changeInfo = gApi.changes().id(changeId2).get(CURRENT_REVISION);
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision)._number).isEqualTo(2);
+ }
+
+ @Test
+ public void validationFailsOnRebaseChange_changeThatCreatesInvalidCodeOwnerConfig()
+ throws Exception {
+ // Create two changes for refs/meta/config both with the same parent.
+ GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+ testRepo.reset("config");
+ Config cfg = new Config();
+ cfg.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.NONE);
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(), testRepo, "Change 1", "code-owners.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId1 = r.getChangeId();
+
+ testRepo.reset("config");
+ cfg = new Config();
+ cfg.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ push =
+ pushFactory.create(
+ admin.newIdent(), testRepo, "Change 2", "code-owners.config", cfg.toText());
+ r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ String changeId2 = r.getChangeId();
+
+ // Approve and submit the first change
+ approve(changeId1);
+ gApi.changes().id(changeId1).current().submit();
+
+ // Rebase the second change with allowing conflicts. This results in a code-owners.config that
+ // contains conflict markers and hence is rejected as invalid.
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.allowConflicts = true;
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId2).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "Invalid config file code-owners.config in project %s in branch %s",
+ project, RefNames.REFS_CONFIG));
+ }
+
private void fetchRefsMetaConfig() throws Exception {
fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
testRepo.reset(RefNames.REFS_CONFIG);
