Merge "Fix MissingObjectException when validating OWNERS imports on create change"
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index b0da9f7..ae55e82 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -41,6 +41,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.validation.CodeOwnerConfigValidator;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -54,6 +55,8 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
/**
* REST endpoint that checks/validates the code owner config files in a project.
@@ -72,6 +75,7 @@
private final Provider<CurrentUser> currentUser;
private final PermissionBackend permissionBackend;
+ private final GitRepositoryManager repoManager;
private final Provider<ListBranches> listBranches;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerConfigScanner.Factory codeOwnerConfigScannerFactory;
@@ -81,12 +85,14 @@
public CheckCodeOwnerConfigFiles(
Provider<CurrentUser> currentUser,
PermissionBackend permissionBackend,
+ GitRepositoryManager repoManager,
Provider<ListBranches> listBranches,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigScanner.Factory codeOwnerConfigScannerFactory,
CodeOwnerConfigValidator codeOwnerConfigValidator) {
this.currentUser = currentUser;
this.permissionBackend = permissionBackend;
+ this.repoManager = repoManager;
this.listBranches = listBranches;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerConfigScannerFactory = codeOwnerConfigScannerFactory;
@@ -120,20 +126,23 @@
validateInput(projectResource.getNameKey(), branches, input);
- ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>> resultsByBranchBuilder =
- ImmutableMap.builder();
- branches.stream()
- .filter(branchNameKey -> shouldValidateBranch(input, branchNameKey))
- .filter(
- branchNameKey ->
- validateDisabledBranches(input)
- || !codeOwnersPluginConfiguration.isDisabled(branchNameKey))
- .forEach(
- branchNameKey ->
- resultsByBranchBuilder.put(
- branchNameKey.branch(),
- checkBranch(input.path, branchNameKey, input.verbosity)));
- return Response.ok(resultsByBranchBuilder.build());
+ try (Repository repo = repoManager.openRepository(projectResource.getNameKey());
+ RevWalk revWalk = new RevWalk(repo)) {
+ ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>>
+ resultsByBranchBuilder = ImmutableMap.builder();
+ branches.stream()
+ .filter(branchNameKey -> shouldValidateBranch(input, branchNameKey))
+ .filter(
+ branchNameKey ->
+ validateDisabledBranches(input)
+ || !codeOwnersPluginConfiguration.isDisabled(branchNameKey))
+ .forEach(
+ branchNameKey ->
+ resultsByBranchBuilder.put(
+ branchNameKey.branch(),
+ checkBranch(revWalk, input.path, branchNameKey, input.verbosity)));
+ return Response.ok(resultsByBranchBuilder.build());
+ }
}
private ImmutableSet<BranchNameKey> branches(ProjectResource projectResource)
@@ -145,6 +154,7 @@
}
private Map<String, List<ConsistencyProblemInfo>> checkBranch(
+ RevWalk revWalk,
String pathGlob,
BranchNameKey branchNameKey,
@Nullable ConsistencyProblemInfo.Status verbosity) {
@@ -162,7 +172,11 @@
problemsByPath.putAll(
codeOwnerBackend.getFilePath(codeOwnerConfig.key()).toString(),
checkCodeOwnerConfig(
- branchNameKey.project(), codeOwnerBackend, codeOwnerConfig, verbosity));
+ branchNameKey.project(),
+ revWalk,
+ codeOwnerBackend,
+ codeOwnerConfig,
+ verbosity));
return true;
},
(codeOwnerConfigFilePath, configInvalidException) -> {
@@ -178,12 +192,17 @@
private ImmutableList<ConsistencyProblemInfo> checkCodeOwnerConfig(
Project.NameKey project,
+ RevWalk revWalk,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@Nullable ConsistencyProblemInfo.Status verbosity) {
return codeOwnerConfigValidator
.validateCodeOwnerConfig(
- project, currentUser.get().asIdentifiedUser(), codeOwnerBackend, codeOwnerConfig)
+ project,
+ revWalk,
+ currentUser.get().asIdentifiedUser(),
+ codeOwnerBackend,
+ codeOwnerConfig)
.map(
commitValidationMessage ->
createConsistencyProblemInfo(commitValidationMessage, verbosity))
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index cef6d52..2b8d479 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -488,7 +488,7 @@
// issues. Hence in this case we downgrade all validation errors in the new version to
// warnings so that the update is not blocked.
return validateCodeOwnerConfig(
- branchNameKey.project(), user, codeOwnerBackend, codeOwnerConfig)
+ branchNameKey.project(), revWalk, user, codeOwnerBackend, codeOwnerConfig)
.map(CodeOwnerConfigValidator::downgradeErrorToWarning);
}
@@ -500,13 +500,14 @@
if (baseCodeOwnerConfig.isPresent()) {
return validateCodeOwnerConfig(
branchNameKey.project(),
+ revWalk,
user,
codeOwnerBackend,
codeOwnerConfig,
baseCodeOwnerConfig.get());
}
return validateCodeOwnerConfig(
- branchNameKey.project(), user, codeOwnerBackend, codeOwnerConfig);
+ branchNameKey.project(), revWalk, user, codeOwnerBackend, codeOwnerConfig);
}
/**
@@ -672,6 +673,7 @@
* they are not newly introduced by the given code owner config).
*
* @param project the name of the project
+ * @param revWalk rev walk that should be used to load the code owner configs
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner configs were loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -681,6 +683,7 @@
*/
private Stream<CommitValidationMessage> validateCodeOwnerConfig(
Project.NameKey project,
+ RevWalk revWalk,
IdentifiedUser user,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@@ -689,9 +692,9 @@
requireNonNull(baseCodeOwnerConfig, "baseCodeOwnerConfig");
ImmutableSet<CommitValidationMessage> issuesInBaseVersion =
- validateCodeOwnerConfig(project, user, codeOwnerBackend, baseCodeOwnerConfig)
+ validateCodeOwnerConfig(project, revWalk, user, codeOwnerBackend, baseCodeOwnerConfig)
.collect(toImmutableSet());
- return validateCodeOwnerConfig(project, user, codeOwnerBackend, codeOwnerConfig)
+ return validateCodeOwnerConfig(project, revWalk, user, codeOwnerBackend, codeOwnerConfig)
.map(
commitValidationMessage ->
issuesInBaseVersion.contains(commitValidationMessage)
@@ -703,6 +706,7 @@
* Validates the given code owner config and returns validation issues as stream.
*
* @param project the name of the project
+ * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner config was loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -711,6 +715,7 @@
*/
public Stream<CommitValidationMessage> validateCodeOwnerConfig(
Project.NameKey project,
+ RevWalk revWalk,
IdentifiedUser user,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig) {
@@ -719,7 +724,10 @@
validateCodeOwnerReferences(
project, user, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig),
validateImports(
- project, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig));
+ project,
+ revWalk,
+ codeOwnerBackend.getFilePath(codeOwnerConfig.key()),
+ codeOwnerConfig));
}
/**
@@ -796,6 +804,7 @@
* Validates the imports of the given code owner config.
*
* @param project the name of the project
+ * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config
* @param codeOwnerConfig the code owner config for which the imports should be validated
@@ -803,13 +812,17 @@
* if there are no issues
*/
private Stream<CommitValidationMessage> validateImports(
- Project.NameKey project, Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) {
+ Project.NameKey project,
+ RevWalk revWalk,
+ Path codeOwnerConfigFilePath,
+ CodeOwnerConfig codeOwnerConfig) {
return Streams.concat(
codeOwnerConfig.imports().stream()
.map(
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
project,
+ revWalk,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfig.revision(),
@@ -821,6 +834,7 @@
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
project,
+ revWalk,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfig.revision(),
@@ -834,6 +848,7 @@
* Validates a code owner config reference.
*
* @param project the name of the project
+ * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @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
@@ -846,6 +861,7 @@
*/
private Optional<CommitValidationMessage> validateCodeOwnerConfigReference(
Project.NameKey project,
+ RevWalk revWalk,
Path codeOwnerConfigFilePath,
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
ObjectId codeOwnerConfigRevision,
@@ -916,9 +932,15 @@
}
try {
- if (!codeOwnerBackend
- .getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get())
- .isPresent()) {
+ // If a code owner config is imported from the same project, we must use the provided rev
+ // walk, otherwise the revision may not be visible yet and trying to load a code owner config
+ // from it could fail with MissingObjectException.
+ Optional<CodeOwnerConfig> importedCodeOwnerConfig =
+ keyOfImportedCodeOwnerConfig.project().equals(project)
+ ? codeOwnerBackend.getCodeOwnerConfig(
+ keyOfImportedCodeOwnerConfig, revWalk, revision.get())
+ : codeOwnerBackend.getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get());
+ if (!importedCodeOwnerConfig.isPresent()) {
return nonResolvableImport(
project,
importType,
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 0b55946..8d3c3cf 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -1643,16 +1643,34 @@
}
private void testValidateMergeCommitCreatedViaTheCreateChangeRestApi() throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
// Create another branch.
String branchName = "stable";
createBranch(BranchNameKey.create(project, branchName));
- // Create a code owner config file in the other branch.
+ // Create a code owner config file in the other branch that can be imported.
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(branchName)
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportedCodeOwnerConfig).getFilePath());
+
+ // Create a code owner config file in the other branch that contains an import.
codeOwnerConfigOperations
.newCodeOwnerConfig()
.project(project)
.branch(branchName)
.folderPath("/")
+ .addImport(codeOwnerConfigReference)
.addCodeOwnerEmail(user.email())
.create();
