Merge "Return code-owners~skip-validation option only if OWNERS files are touched"
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java index 2917a90..316da14 100644 --- a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java +++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -20,13 +20,19 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.plugins.codeowners.backend.ChangedFiles; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; +import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy; import com.google.gerrit.server.PluginPushOption; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.patch.DiffNotAvailableException; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.inject.Inject; import com.google.inject.Singleton; +import java.io.IOException; +import java.nio.file.Path; /** Push option that allows to skip the code owner config validation. */ @Singleton @@ -38,17 +44,20 @@ private final PermissionBackend permissionBackend; private final SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability; private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; + private final ChangedFiles changedFiles; @Inject SkipCodeOwnerConfigValidationPushOption( @PluginName String pluginName, PermissionBackend permissionBackend, SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability, - CodeOwnersPluginConfiguration codeOwnersPluginConfiguration) { + CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, + ChangedFiles changedFiles) { this.pluginName = pluginName; this.permissionBackend = permissionBackend; this.skipCodeOwnerConfigValidationCapability = skipCodeOwnerConfigValidationCapability; this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; + this.changedFiles = changedFiles; } @Override @@ -66,7 +75,39 @@ return !codeOwnersPluginConfiguration .getProjectConfig(changeNotes.getProjectName()) .isDisabled(changeNotes.getChange().getDest().branch()) - && canSkipCodeOwnerConfigValidation(); + && canSkipCodeOwnerConfigValidation() + && hasModifiedCodeOwnerConfigFiles(changeNotes); + } + + private boolean hasModifiedCodeOwnerConfigFiles(ChangeNotes changeNotes) { + CodeOwnerBackend codeOwnerBackend = + codeOwnersPluginConfiguration + .getProjectConfig(changeNotes.getProjectName()) + .getBackend(changeNotes.getChange().getDest().branch()); + + // For merge commits, always do the comparison against the destination branch + // (MergeCommitStrategy.ALL_CHANGED_FILES) as this is what CodeOwnerConfigValidator does. + try { + return changedFiles + .getFromDiffCache( + changeNotes.getProjectName(), + changeNotes.getCurrentPatchSet().commitId(), + MergeCommitStrategy.ALL_CHANGED_FILES) + .stream() + // filter out deletions (files without new path) + .filter(changedFile -> changedFile.newPath().isPresent()) + .anyMatch( + changedFile -> + codeOwnerBackend.isCodeOwnerConfigFile( + changeNotes.getProjectName(), + Path.of(changedFile.newPath().get().toString()).getFileName().toString())); + } catch (IOException | DiffNotAvailableException e) { + throw newInternalServerError( + String.format( + "Failed to check changed files of change %s in project %s", + changeNotes.getChangeId(), changeNotes.getProjectName()), + e); + } } /**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java index 6c74375..dd2cf9d 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java
@@ -21,30 +21,56 @@ import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.change.ChangeOperations; +import com.google.gerrit.acceptance.testsuite.change.TestChangeCreation; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.common.ValidationOptionInfo; import com.google.gerrit.extensions.common.ValidationOptionInfos; import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet; +import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig; +import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend; +import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersCodeOwnerConfigParser; +import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend; +import com.google.gerrit.plugins.codeowners.backend.proto.ProtoCodeOwnerConfigParser; import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationCapability; import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationPushOption; import com.google.inject.Inject; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Before; import org.junit.Test; public class SkipCodeOwnerConfigValidationPushOptionIT extends AbstractCodeOwnersIT { + private static final ObjectId TEST_REVISION = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + @Inject private ChangeOperations changeOperations; @Inject private ProjectOperations projectOperations; @Inject private RequestScopeOperations requestScopeOperations; + private FindOwnersCodeOwnerConfigParser findOwnersCodeOwnerConfigParser; + private ProtoCodeOwnerConfigParser protoCodeOwnerConfigParser; + + @Before + public void setUpCodeOwnersPlugin() throws Exception { + backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class); + findOwnersCodeOwnerConfigParser = + plugin.getSysInjector().getInstance(FindOwnersCodeOwnerConfigParser.class); + protoCodeOwnerConfigParser = + plugin.getSysInjector().getInstance(ProtoCodeOwnerConfigParser.class); + } + @Test public void getCodeOwnersSkipOptionAsAdmin() throws Exception { // Use the admin user that has the SkipCodeOwnerConfigValidationCapability global capability // implicitly assigned. requestScopeOperations.setApiUser(admin.id()); - Change.Id changeId = changeOperations.newChange().project(project).create(); - + Change.Id changeId = createChangeWithCodeOwnerConfigFile(project); ValidationOptionInfos validationOptionsInfos = gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); assertThat(validationOptionsInfos.validationOptions) @@ -66,9 +92,7 @@ .update(); requestScopeOperations.setApiUser(user.id()); - - Change.Id changeId = changeOperations.newChange().project(project).create(); - + Change.Id changeId = createChangeWithCodeOwnerConfigFile(project); ValidationOptionInfos validationOptionsInfos = gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); assertThat(validationOptionsInfos.validationOptions) @@ -86,7 +110,7 @@ // capability. requestScopeOperations.setApiUser(user.id()); - Change.Id changeId = changeOperations.newChange().project(project).create(); + Change.Id changeId = createChangeWithCodeOwnerConfigFile(project); ValidationOptionInfos validationOptionsInfos = gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); assertThat(validationOptionsInfos.validationOptions).isEmpty(); @@ -96,7 +120,7 @@ @Test public void codeOwnersSkipOptionIsOmittedIfCodeOwnersFunctionalityIsDisabledForProject() throws Exception { - Change.Id changeId = changeOperations.newChange().project(project).create(); + Change.Id changeId = createChangeWithCodeOwnerConfigFile(project); ValidationOptionInfos validationOptionsInfos = gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); assertThat(validationOptionsInfos.validationOptions).isEmpty(); @@ -106,9 +130,57 @@ @Test public void codeOwnersSkipOptionIsOmittedIfCodeOwnersFunctionalityIsDisabledForBranch() throws Exception { - Change.Id changeId = changeOperations.newChange().project(project).branch("master").create(); + Change.Id changeId = createChangeWithCodeOwnerConfigFile(project, "master"); ValidationOptionInfos validationOptionsInfos = gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); assertThat(validationOptionsInfos.validationOptions).isEmpty(); } + + @Test + public void codeOwnersSkipOptionIsOmittedIfChangeDoesNotTouchCodeOwnerConfigs() throws Exception { + requestScopeOperations.setApiUser(admin.id()); + Change.Id changeId = changeOperations.newChange().project(project).create(); + ValidationOptionInfos validationOptionsInfos = + gApi.changes().id(project.get(), changeId.get()).getValidationOptions(); + assertThat(validationOptionsInfos.validationOptions).isEmpty(); + } + + private Change.Id createChangeWithCodeOwnerConfigFile(Project.NameKey project) throws Exception { + return createChangeWithCodeOwnerConfigFile(project, /* branch= */ null); + } + + private Change.Id createChangeWithCodeOwnerConfigFile( + Project.NameKey project, @Nullable String branch) throws Exception { + CodeOwnerConfig.Key codeOwnerConfigKey = CodeOwnerConfig.Key.create(project, "master", "/"); + String codeOwnerConfigFile = + codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(); + + TestChangeCreation.Builder testChangeCreationBuilder = + changeOperations.newChange().project(project); + if (branch != null) { + testChangeCreationBuilder.branch(branch); + } + + return testChangeCreationBuilder + .file(codeOwnerConfigFile) + .content( + format( + CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION) + .addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(admin.email())) + .build())) + .create(); + } + + private String format(CodeOwnerConfig codeOwnerConfig) throws Exception { + if (backendConfig.getDefaultBackend() instanceof FindOwnersBackend) { + return findOwnersCodeOwnerConfigParser.formatAsString(codeOwnerConfig); + } else if (backendConfig.getDefaultBackend() instanceof ProtoBackend) { + return protoCodeOwnerConfigParser.formatAsString(codeOwnerConfig); + } + + throw new IllegalStateException( + String.format( + "unknown code owner backend: %s", + backendConfig.getDefaultBackend().getClass().getName())); + } }