Validation: Do not reject imports that are added in the same commit
If a commit is validated that contains a code owner config with a new
import of a code owner config file that is added in the same commit, the
commit was wrongly rejected saying that the imported code owner config
was not resolveable. This was because we looked for the imported code
owner config in the HEAD revision of the branch. Instead we should look
it up from the commit from which also the importing code owner config
was loaded.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I32b0d8b9f3699e012e8c904e8602635e3a92a491
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 234ad45..e659580 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -776,6 +776,7 @@
validateCodeOwnerConfigReference(
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
+ codeOwnerConfig.revision(),
CodeOwnerConfigImportType.GLOBAL,
codeOwnerConfigReference)),
codeOwnerConfig.codeOwnerSets().stream()
@@ -785,6 +786,7 @@
validateCodeOwnerConfigReference(
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
+ codeOwnerConfig.revision(),
CodeOwnerConfigImportType.PER_FILE,
codeOwnerConfigReference)))
.filter(Optional::isPresent)
@@ -797,6 +799,8 @@
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config reference
* @param keyOfImportingCodeOwnerConfig key of the importing code owner config
+ * @param codeOwnerConfigRevision the commit from which the code owner config which contains the
+ * code owner config reference was loaded
* @param importType the type of the import
* @param codeOwnerConfigReference the code owner config reference that should be validated.
* @return a validation message describing the issue with the code owner config reference, {@link
@@ -805,6 +809,7 @@
private Optional<CommitValidationMessage> validateCodeOwnerConfigReference(
Path codeOwnerConfigFilePath,
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
+ ObjectId codeOwnerConfigRevision,
CodeOwnerConfigImportType importType,
CodeOwnerConfigReference codeOwnerConfigReference) {
CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
@@ -832,7 +837,9 @@
projectState.get().getProject().getState().name()));
}
- Optional<ObjectId> revision = getRevision(keyOfImportedCodeOwnerConfig);
+ Optional<ObjectId> revision =
+ getRevision(
+ keyOfImportingCodeOwnerConfig, codeOwnerConfigRevision, keyOfImportedCodeOwnerConfig);
if (!revision.isPresent() || !isBranchReadable(keyOfImportedCodeOwnerConfig)) {
// we intentionally use the same error message for non-existing and non-readable branches so
// that uploaders cannot probe for the existence of branches (e.g. deduce from the error
@@ -916,7 +923,18 @@
}
}
- private Optional<ObjectId> getRevision(CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) {
+ private Optional<ObjectId> getRevision(
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
+ ObjectId codeOwnerConfigRevision,
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) {
+ if (keyOfImportingCodeOwnerConfig
+ .branchNameKey()
+ .equals(keyOfImportedCodeOwnerConfig.branchNameKey())) {
+ // load the imported code owner config from the same revision from which the importing code
+ // owner config was loaded
+ return Optional.of(codeOwnerConfigRevision);
+ }
+
try (Repository repo = repoManager.openRepository(keyOfImportedCodeOwnerConfig.project())) {
return Optional.ofNullable(repo.exactRef(keyOfImportedCodeOwnerConfig.ref()))
.map(Ref::getObjectId);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index 68b4f46..2e5e73b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -22,6 +22,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
@@ -166,6 +167,12 @@
.addCodeOwnerEmail(user.email())
.create();
+ // Fetch the commit that created the imported code owner config into the local repository so
+ // that the commit that creates the importing code owner config becomes a successor of this
+ // commit.
+ GitUtil.fetch(testRepo, "refs/*:refs/*");
+ testRepo.reset(projectOperations.project(project).getHead("master"));
+
CodeOwnerConfigReference codeOwnerConfigReference =
CodeOwnerConfigReference.create(
CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
@@ -282,6 +289,47 @@
}
@Test
+ public void canUploadConfigWithoutIssues_withImportOfConfigThatIsAddedInSameCommit()
+ throws Exception {
+ // imports are not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig = createCodeOwnerConfigKey("/foo/");
+
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportedCodeOwnerConfig).getFilePath());
+
+ // Create a code owner config with import and without issues.
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ ImmutableMap.of(
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addImport(codeOwnerConfigReference)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addPathExpression("foo")
+ .addImport(codeOwnerConfigReference)
+ .build())
+ .build()),
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportedCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(keyOfImportedCodeOwnerConfig, TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder().addCodeOwnerEmail(user.email()).build())
+ .build())));
+ assertOkWithHints(r, "code owner config files validated, no issues found");
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.backend", value = "non-existing-backend")
public void canUploadNonParseableConfigIfCodeOwnersPluginConfigurationIsInvalid()
throws Exception {
