Add REST endpoint to validate the code owner config files of a project This REST endpoint allows project owners to validate the code owner config files in their projects and fix the detected issues. This is important since code owner config files can become invalid after they have been submitted, e.g. because a referenced code owner config file has been deleted. No validation is done for branches for which the code owner functionality is disabled. In the ProjectCodeOwners API class we have an inner class to represent the request to check code owner config files so that we can add parameters for this REST endpoint later without making an incompatible API change. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I3cfa1b846a6dc223e849b6b9c57a7c9ea03abddc
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java index 8138287..4d3785b 100644 --- a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java +++ b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java
@@ -14,7 +14,10 @@ package com.google.gerrit.plugins.codeowners.api; +import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.extensions.restapi.RestApiException; +import java.util.List; +import java.util.Map; /** * Project-level Java API of the code-owners plugin. @@ -25,6 +28,19 @@ /** Returns the code owner project configuration. */ CodeOwnerProjectConfigInfo getConfig() throws RestApiException; + /** Create a request to check the code owner config files in the project. */ + CheckCodeOwnerConfigFilesRequest checkCodeOwnerConfigFiles() throws RestApiException; + + /** Request to check code owner config files. */ + abstract class CheckCodeOwnerConfigFilesRequest { + /** + * Executes the request to check the code owner config files and retrieves the result of the + * validation. + */ + public abstract Map<String, Map<String, List<ConsistencyProblemInfo>>> check() + throws RestApiException; + } + /** Returns the branch-level code owners API for the given branch. */ BranchCodeOwners branch(String branchName) throws RestApiException; }
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java index 0454dc0..813f6a8 100644 --- a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java +++ b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java
@@ -16,14 +16,19 @@ import static com.google.gerrit.server.api.ApiUtil.asRestApiException; +import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; +import com.google.gerrit.extensions.common.Input; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles; import com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnerProjectConfig; import com.google.gerrit.server.project.BranchResource; import com.google.gerrit.server.project.ProjectResource; import com.google.gerrit.server.restapi.project.BranchesCollection; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; +import java.util.List; +import java.util.Map; /** Implementation of the {@link ProjectCodeOwners} API. */ public class ProjectCodeOwnersImpl implements ProjectCodeOwners { @@ -34,6 +39,7 @@ private final BranchesCollection branchesCollection; private final BranchCodeOwnersImpl.Factory branchCodeOwnersApi; private final GetCodeOwnerProjectConfig getCodeOwnerProjectConfig; + private final CheckCodeOwnerConfigFiles checkCodeOwnerConfigFiles; private final ProjectResource projectResource; @Inject @@ -41,10 +47,12 @@ BranchesCollection branchesCollection, BranchCodeOwnersImpl.Factory branchCodeOwnersApi, GetCodeOwnerProjectConfig getCodeOwnerProjectConfig, + CheckCodeOwnerConfigFiles checkCodeOwnerConfigFiles, @Assisted ProjectResource projectResource) { this.branchesCollection = branchesCollection; this.branchCodeOwnersApi = branchCodeOwnersApi; this.getCodeOwnerProjectConfig = getCodeOwnerProjectConfig; + this.checkCodeOwnerConfigFiles = checkCodeOwnerConfigFiles; this.projectResource = projectResource; } @@ -67,4 +75,19 @@ throw asRestApiException("Cannot get code owner project config", e); } } + + @Override + public CheckCodeOwnerConfigFilesRequest checkCodeOwnerConfigFiles() throws RestApiException { + return new CheckCodeOwnerConfigFilesRequest() { + @Override + public Map<String, Map<String, List<ConsistencyProblemInfo>>> check() + throws RestApiException { + try { + return checkCodeOwnerConfigFiles.apply(projectResource, new Input()).value(); + } catch (Exception e) { + throw asRestApiException("Cannot check code owner config files", e); + } + } + }; + } }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java index b898919..6585278 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -130,7 +130,8 @@ } // The code owner config is invalid and cannot be parsed. - invalidCodeOwnerConfigCallback.onInvalidCodeOwnerConfig(folderPath.resolve(fileName)); + invalidCodeOwnerConfigCallback.onInvalidCodeOwnerConfig( + folderPath.resolve(fileName), configInvalidException.get()); continue; } @@ -181,11 +182,7 @@ * config files. */ public static InvalidCodeOwnerConfigCallback ignoreInvalidCodeOwnerConfigFiles() { - return new InvalidCodeOwnerConfigCallback() { - @Override - public void onInvalidCodeOwnerConfig(Path codeOwnerConfigFilePath) { + return (codeOwnerConfigFilePath, configInvalidException) -> logger.atFine().log("ignoring invalid code owner config file %s", codeOwnerConfigFilePath); - } - }; } }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java index 0da9cb7..fed838a 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/InvalidCodeOwnerConfigCallback.java
@@ -15,6 +15,7 @@ package com.google.gerrit.plugins.codeowners.backend; import java.nio.file.Path; +import org.eclipse.jgit.errors.ConfigInvalidException; /** Callback interface to let callers handle invalid code owner config files. */ public interface InvalidCodeOwnerConfigCallback { @@ -22,6 +23,8 @@ * Invoked when an invalid code owner config file is found. * * @param codeOwnerConfigFilePath the path of the invalid code owner config file + * @param configInvalidException the parsing exception */ - void onInvalidCodeOwnerConfig(Path codeOwnerConfigFilePath); + void onInvalidCodeOwnerConfig( + Path codeOwnerConfigFilePath, ConfigInvalidException configInvalidException); }
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java new file mode 100644 index 0000000..7dc9d62 --- /dev/null +++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -0,0 +1,170 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.restapi; + +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimaps; +import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; +import com.google.gerrit.extensions.common.Input; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigScanner; +import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; +import com.google.gerrit.plugins.codeowners.validation.CodeOwnerConfigValidator; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.git.validators.CommitValidationMessage; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.ProjectPermission; +import com.google.gerrit.server.project.ProjectResource; +import com.google.gerrit.server.restapi.project.ListBranches; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Stream; + +/** + * REST endpoint that checks/validates the code owner config files in a project. + * + * <p>This REST endpoint handles {@code POST + * /projects/<project-name>/branches/<branch-name>/code_owners.check_config} requests. + * + * <p>The implementation of this REST endpoint iterates over all code owner config files in the + * project. This means the expected performance of this REST endpoint is rather low and it should + * not be used in any critical path where performance matters. + */ +@Singleton +public class CheckCodeOwnerConfigFiles implements RestModifyView<ProjectResource, Input> { + private final CurrentUser currentUser; + private final PermissionBackend permissionBackend; + private final Provider<ListBranches> listBranches; + private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; + private final CodeOwnerConfigScanner codeOwnerConfigScanner; + private final CodeOwnerConfigValidator codeOwnerConfigValidator; + + @Inject + public CheckCodeOwnerConfigFiles( + CurrentUser currentUser, + PermissionBackend permissionBackend, + Provider<ListBranches> listBranches, + CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, + CodeOwnerConfigScanner codeOwnerConfigScanner, + CodeOwnerConfigValidator codeOwnerConfigValidator) { + this.currentUser = currentUser; + this.permissionBackend = permissionBackend; + this.listBranches = listBranches; + this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; + this.codeOwnerConfigScanner = codeOwnerConfigScanner; + this.codeOwnerConfigValidator = codeOwnerConfigValidator; + } + + @Override + public Response<Map<String, Map<String, List<ConsistencyProblemInfo>>>> apply( + ProjectResource projectResource, Input input) + throws RestApiException, PermissionBackendException, IOException { + if (!currentUser.isIdentifiedUser()) { + throw new AuthException("Authentication required"); + } + + // This REST endpoint requires the caller to be a project owner. + permissionBackend + .currentUser() + .project(projectResource.getNameKey()) + .check(ProjectPermission.WRITE_CONFIG); + + ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>> resultsByBranchBuilder = + ImmutableMap.builder(); + branches(projectResource) + .filter(branchNameKey -> !codeOwnersPluginConfiguration.isDisabled(branchNameKey)) + .forEach( + branchNameKey -> + resultsByBranchBuilder.put(branchNameKey.branch(), checkBranch(branchNameKey))); + return Response.ok(resultsByBranchBuilder.build()); + } + + private Stream<BranchNameKey> branches(ProjectResource projectResource) + throws RestApiException, IOException, PermissionBackendException { + return listBranches.get().apply(projectResource).value().stream() + .filter(branchInfo -> !"HEAD".equals(branchInfo.ref)) + .map(branchInfo -> BranchNameKey.create(projectResource.getNameKey(), branchInfo.ref)); + } + + private Map<String, List<ConsistencyProblemInfo>> checkBranch(BranchNameKey branchNameKey) { + ListMultimap<String, ConsistencyProblemInfo> problemsByPath = LinkedListMultimap.create(); + CodeOwnerBackend codeOwnerBackend = codeOwnersPluginConfiguration.getBackend(branchNameKey); + codeOwnerConfigScanner.visit( + branchNameKey, + codeOwnerConfig -> { + problemsByPath.putAll( + codeOwnerBackend.getFilePath(codeOwnerConfig.key()).toString(), + checkCodeOwnerConfig(codeOwnerBackend, codeOwnerConfig)); + return true; + }, + (codeOwnerConfigFilePath, configInvalidException) -> { + problemsByPath.put( + codeOwnerConfigFilePath.toString(), + new ConsistencyProblemInfo( + ConsistencyProblemInfo.Status.ERROR, configInvalidException.getMessage())); + }); + + return Multimaps.asMap(problemsByPath); + } + + private ImmutableList<ConsistencyProblemInfo> checkCodeOwnerConfig( + CodeOwnerBackend codeOwnerBackend, CodeOwnerConfig codeOwnerConfig) { + return codeOwnerConfigValidator + .validateCodeOwnerConfig(currentUser.asIdentifiedUser(), codeOwnerBackend, codeOwnerConfig) + .map(this::createConsistencyProblemInfo) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(toImmutableList()); + } + + private Optional<ConsistencyProblemInfo> createConsistencyProblemInfo( + CommitValidationMessage commitValidationMessage) { + switch (commitValidationMessage.getType()) { + case ERROR: + return Optional.of( + new ConsistencyProblemInfo( + ConsistencyProblemInfo.Status.ERROR, commitValidationMessage.getMessage())); + case WARNING: + return Optional.of( + new ConsistencyProblemInfo( + ConsistencyProblemInfo.Status.WARNING, commitValidationMessage.getMessage())); + case HINT: + case OTHER: + return Optional.empty(); + } + + throw new IllegalStateException( + String.format( + "unknown message type %s for message %s", + commitValidationMessage.getType(), commitValidationMessage.getMessage())); + } +}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/RestApiModule.java b/java/com/google/gerrit/plugins/codeowners/restapi/RestApiModule.java index daf66ed..8599f39 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/RestApiModule.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/RestApiModule.java
@@ -43,5 +43,6 @@ get(CHANGE_KIND, "code_owners.status").to(GetCodeOwnerStatus.class); get(PROJECT_KIND, "code_owners.project_config").to(GetCodeOwnerProjectConfig.class); + post(PROJECT_KIND, "code_owners.check_config").to(CheckCodeOwnerConfigFiles.class); } }
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java index 98e9690..54f8f09 100644 --- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java +++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -563,7 +563,7 @@ * @return a stream of validation messages that describe issues with the code owner config, an * empty stream if there are no issues */ - private Stream<CommitValidationMessage> validateCodeOwnerConfig( + public Stream<CommitValidationMessage> validateCodeOwnerConfig( IdentifiedUser user, CodeOwnerBackend codeOwnerBackend, CodeOwnerConfig codeOwnerConfig) { requireNonNull(codeOwnerConfig, "codeOwnerConfig"); return Streams.concat(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java new file mode 100644 index 0000000..47d78fa --- /dev/null +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -0,0 +1,237 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.acceptance.api; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.plugins.codeowners.JgitPath; +import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference; +import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend; +import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend; +import com.google.gerrit.plugins.codeowners.config.BackendConfig; +import com.google.gerrit.plugins.codeowners.config.StatusConfig; +import com.google.inject.Inject; +import java.util.List; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; + +/** + * Acceptance test for the {@link + * com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles} REST endpoint. + * + * <p>Further tests for the {@link + * com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles} REST endpoint that + * require using the REST API are implemented in {@link + * com.google.gerrit.plugins.codeowners.acceptance.restapi.CheckCodeOwnerConfigFilesRestIT}. + */ +public class CheckCodeOwnerConfigFilesIT extends AbstractCodeOwnersIT { + @Inject private RequestScopeOperations requestScopeOperations; + + private BackendConfig backendConfig; + + @Before + public void setUpCodeOwnersPlugin() throws Exception { + backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class); + } + + @Test + public void requiresCallerToBeProjectOwner() throws Exception { + requestScopeOperations.setApiUser(user.id()); + AuthException authException = + assertThrows(AuthException.class, () -> checkCodeOwnerConfigFilesIn(project)); + assertThat(authException).hasMessageThat().isEqualTo("write refs/meta/config not permitted"); + } + + @Test + public void noCodeOwnerConfigFile() throws Exception { + assertThat(checkCodeOwnerConfigFilesIn(project)) + .containsExactly( + "refs/heads/master", ImmutableMap.of(), + "refs/meta/config", ImmutableMap.of()); + } + + @Test + @GerritConfig(name = "plugin.code-owners.disabledBranch", value = "refs/meta/config") + public void disabledBranchesAreSkipped() throws Exception { + assertThat(checkCodeOwnerConfigFilesIn(project)) + .containsExactly("refs/heads/master", ImmutableMap.of()); + } + + @Test + public void noIssuesInCodeOwnerConfigFile() throws Exception { + // Create some code owner config files. + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/bar/") + .addCodeOwnerEmail(user.email()) + .create(); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/baz/") + .addCodeOwnerEmail(admin.email()) + .create(); + + assertThat(checkCodeOwnerConfigFilesIn(project)) + .containsExactly( + "refs/heads/master", ImmutableMap.of(), + "refs/meta/config", ImmutableMap.of()); + } + + @Test + public void nonParseableCodeOwnerConfigFile() throws Exception { + String codeOwnerConfigPath = "/" + getCodeOwnerConfigFileName(); + createInvalidCodeOwnerConfig(codeOwnerConfigPath); + + assertThat(checkCodeOwnerConfigFilesIn(project)) + .containsExactly( + "refs/heads/master", + ImmutableMap.of( + codeOwnerConfigPath, + ImmutableList.of( + error( + String.format( + "invalid code owner config file '%s':\n %s", + codeOwnerConfigPath, + getParsingErrorMessage( + ImmutableMap.of( + FindOwnersBackend.class, + "invalid line: INVALID", + ProtoBackend.class, + "1:8: Expected \"{\".")))))), + "refs/meta/config", ImmutableMap.of()); + } + + @Test + public void issuesInCodeOwnerConfigFile() throws Exception { + // imports are not supported for the proto backend + assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class); + + // Create some code owner config files with issues. + CodeOwnerConfig.Key keyOfInvalidConfig1 = + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addImport( + CodeOwnerConfigReference.create( + CodeOwnerConfigImportMode.ALL, "/not-a-code-owner-config")) + .create(); + + CodeOwnerConfig.Key keyOfInvalidConfig2 = + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/bar/") + .addCodeOwnerEmail("unknown1@example.com") + .addCodeOwnerEmail("unknown2@example.com") + .create(); + + // Also create a code owner config files without issues. + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/baz") + .addCodeOwnerEmail(user.email()) + .create(); + + assertThat(checkCodeOwnerConfigFilesIn(project)) + .containsExactly( + "refs/heads/master", + ImmutableMap.of( + getCodeOwnerConfigFilePath(keyOfInvalidConfig1), + ImmutableList.of( + error( + String.format( + "invalid global import in '%s': '/not-a-code-owner-config' is" + + " not a code owner config file", + getCodeOwnerConfigFilePath(keyOfInvalidConfig1)))), + getCodeOwnerConfigFilePath(keyOfInvalidConfig2), + ImmutableList.of( + error( + String.format( + "code owner email 'unknown1@example.com' in '%s' cannot be" + + " resolved for admin", + getCodeOwnerConfigFilePath(keyOfInvalidConfig2))), + error( + String.format( + "code owner email 'unknown2@example.com' in '%s' cannot be" + + " resolved for admin", + getCodeOwnerConfigFilePath(keyOfInvalidConfig2))))), + "refs/meta/config", ImmutableMap.of()); + } + + private ConsistencyProblemInfo error(String message) { + return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message); + } + + private Map<String, Map<String, List<ConsistencyProblemInfo>>> checkCodeOwnerConfigFilesIn( + Project.NameKey projectName) throws RestApiException { + return projectCodeOwnersApiFactory.project(projectName).checkCodeOwnerConfigFiles().check(); + } + + private String getCodeOwnerConfigFilePath(CodeOwnerConfig.Key codeOwnerConfigKey) { + return backendConfig.getDefaultBackend().getFilePath(codeOwnerConfigKey).toString(); + } + + private String getCodeOwnerConfigFileName() { + CodeOwnerBackend backend = backendConfig.getDefaultBackend(); + if (backend instanceof FindOwnersBackend) { + return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME; + } else if (backend instanceof ProtoBackend) { + return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME; + } + throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName()); + } + + private void createInvalidCodeOwnerConfig(String path) throws Exception { + disableCodeOwnersForProject(project); + String changeId = + createChange("Add invalid code owners file", JgitPath.of(path).get(), "INVALID") + .getChangeId(); + approve(changeId); + gApi.changes().id(changeId).current().submit(); + setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED, "false"); + } + + private String getParsingErrorMessage( + ImmutableMap<Class<? extends CodeOwnerBackend>, String> messagesByBackend) { + CodeOwnerBackend codeOwnerBackend = backendConfig.getDefaultBackend(); + assertThat(messagesByBackend).containsKey(codeOwnerBackend.getClass()); + return messagesByBackend.get(codeOwnerBackend.getClass()); + } +}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CheckCodeOwnerConfigFilesRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CheckCodeOwnerConfigFilesRestIT.java new file mode 100644 index 0000000..eaf3ef1 --- /dev/null +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CheckCodeOwnerConfigFilesRestIT.java
@@ -0,0 +1,64 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.acceptance.restapi; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.acceptance.RestResponse; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.extensions.restapi.IdString; +import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; +import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest; +import org.junit.Test; + +/** + * Acceptance test for the {@link + * com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles} REST endpoint that + * require using REST. + * + * <p>Acceptance test for the {@link + * com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles} REST endpoint that can + * use the Java API are implemented in {@link + * com.google.gerrit.plugins.codeowners.acceptance.api.CheckCodeOwnerConfigFilesIT}. + * + * <p>The tests in this class do not depend on the used code owner backend, hence we do not need to + * extend {@link AbstractCodeOwnersIT}. + */ +public class CheckCodeOwnerConfigFilesRestIT extends AbstractCodeOwnersTest { + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = "non-existing-backend") + public void cannotCheckCodeOwnerConfigFilesIfPluginConfigurationIsInvalid() throws Exception { + RestResponse r = + adminRestSession.post( + String.format( + "/projects/%s/code_owners.check_config", IdString.fromDecoded(project.get()))); + r.assertConflict(); + assertThat(r.getEntityContent()) + .contains( + "Invalid configuration of the code-owners plugin. Code owner backend" + + " 'non-existing-backend' that is configured in gerrit.config (parameter" + + " plugin.code-owners.backend) not found."); + } + + @Test + public void cannotCheckCodeOwnerConfigFilesAnonymously() throws Exception { + RestResponse r = + anonymousRestSession.post( + String.format( + "/projects/%s/code_owners.check_config", IdString.fromDecoded(project.get()))); + r.assertForbidden(); + assertThat(r.getEntityContent()).contains("Authentication required"); + } +}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CodeOwnersRestApiBindingsIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CodeOwnersRestApiBindingsIT.java index ae1aee8..7d90176 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CodeOwnersRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/CodeOwnersRestApiBindingsIT.java
@@ -41,7 +41,9 @@ ImmutableList.of(RestCall.get("/changes/%s/code-owners~code_owners.status")); private static final ImmutableList<RestCall> PROJECT_ENDPOINTS = - ImmutableList.of(RestCall.get("/projects/%s/code-owners~code_owners.project_config")); + ImmutableList.of( + RestCall.get("/projects/%s/code-owners~code_owners.project_config"), + RestCall.post("/projects/%s/code_owners.check_config")); private static final ImmutableList<RestCall> BRANCH_ENDPOINTS = ImmutableList.of(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java index d7ebafc..ea0eab7 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
@@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; @@ -28,6 +29,7 @@ import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations; import com.google.gerrit.plugins.codeowners.config.StatusConfig; import java.nio.file.Paths; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -144,7 +146,8 @@ verifyZeroInteractions(visitor); // Verify that we received the expected callbacks for the invalid code onwer config. - Mockito.verify(invalidCodeOwnerConfigCallback).onInvalidCodeOwnerConfig(Paths.get("/OWNERS")); + Mockito.verify(invalidCodeOwnerConfigCallback) + .onInvalidCodeOwnerConfig(eq(Paths.get("/OWNERS")), any(ConfigInvalidException.class)); verifyNoMoreInteractions(invalidCodeOwnerConfigCallback); } @@ -173,7 +176,8 @@ verifyNoMoreInteractions(visitor); // Verify that we received the expected callbacks for the invalid code onwer config. - Mockito.verify(invalidCodeOwnerConfigCallback).onInvalidCodeOwnerConfig(Paths.get("/OWNERS")); + Mockito.verify(invalidCodeOwnerConfigCallback) + .onInvalidCodeOwnerConfig(eq(Paths.get("/OWNERS")), any(ConfigInvalidException.class)); verifyNoMoreInteractions(invalidCodeOwnerConfigCallback); }
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 2abe5e3..242c362 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -56,6 +56,68 @@ } ``` +### <a id="check-code-owner-config-files">Check Code Owner Config Files +_'POST /projects/[\{project-name\}](../../../Documentation/rest-api-projects.html#project-name)/code_owners.check_config'_ + +Checks/validates the code owner config files in a project. + +Requires that the caller is an owner of the project. + +No validation is done for branches for which the code owner functionality is +[disabled](config.html#codeOwnersDisabledBranch). + +As a response a map is returned that maps a branch name to a map that maps an +owner configuration file path to a list of +[ConsistencyProblemInfo](../../../Documentation/rest-api-config.html#consistency-problem-info) +entities. + +Code owner config files that have no issues are omitted from the response. + +#### Request + +``` + POST /projects/foo%2Fbar/code_owners.check_config HTTP/1.0 +``` + +#### Response + +``` + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + "refs/heads/master": { + "/OWNERS": [ + { + "status": "ERROR", + "message": "code owner email 'foo@example.com' in '/OWNERS' cannot be resolved for John Doe" + }, + { + "status": "ERROR", + "message": "code owner email 'bar@example.com' in '/OWNERS' cannot be resolved for John Doe" + } + ], + "/foo/OWNERS": [ + { + "status": "ERROR", + "message": "invalid global import in '/foo/OWNERS': '/not-a-code-owner-config' is not a code owner config file" + } + ] + }, + "refs/heads/stable-1.0" {}, + "refs/heads/stable-1.1" { + "/foo/OWNERS": [ + { + "status": "ERROR", + "message": "invalid global import in '/foo/OWNERS': '/not-a-code-owner-config' is not a code owner config file" + } + ] + } + } +``` + ## <a id="branch-endpoints">Branch Endpoints ### <a id="list-code-owner-config-files">List Code Owner Config Files
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md index 62cd076..507f50c 100644 --- a/resources/Documentation/setup-guide.md +++ b/resources/Documentation/setup-guide.md
@@ -349,6 +349,11 @@ submitting an invalid code owner config that may block the submission of all changes (e.g. if it is not parseable). +**NOTE** If the repository contains pre-existing code owner config files, it is +recommended to validate them via the [Check code owners files REST +endpoint](rest-api.html#check-code-owner-config-files) and fix the reported +issues. + ### <a id="faq">FAQ's ##### <a id="updateCodeOwnersConfig">How to update the code-owners.config file for a project
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md index 4d3165d..9966f09 100644 --- a/resources/Documentation/validation.md +++ b/resources/Documentation/validation.md
@@ -9,7 +9,9 @@ configuration of the `@PLUGIN@` plugin To reduce the risk that these files become invalid, they are validated when -they are modified and invalid modifications are rejected. +they are modified and invalid modifications are rejected. In addition code owner +config files in a repository can be validated on demand by the [Check code +owners files REST endpoint](rest-api.html#check-code-owner-config-files). **NOTE:** Most configuration issues are gracefully handled and do not break the code owners functionality (e.g. non-resolveable code owners or non-resolveable