Fix MissingObjectException when validating OWNERS imports on create change
When a new merge change is created through the Create Change REST
endpoint the code owner config validator can see the newly created merge
commit only through the provided RevWalk. Using another (new) RevWalk
instance does not see this revision yet and trying to access it fails
with a MissingObjectException. For validating imports of code owner
configs we always created a new RevWalk to check if the imported code
owner config exists. If a code owner config was imported from the same
project, and thus from the newly created merge commit, it failed with
a MissingObjectException. Fix this by using the provided RevWalk
instance in this case. The RevWalk instance is specific to the repo for
which it was created, hence for loading code owner configs from other
projects, we still need to create a new RevWalk instance (which happens
when we call CodeOwnerBackend#getCodeOwnerConfig(CodeOwnerConfig.Key,
ObjectId)).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Icd74c3393e1b4e5fd00a495d290109ede74649a7
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();
