Merge "Use List Code Owners for Path in change REST endpoint in frontend"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
new file mode 100644
index 0000000..6222b7d
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
@@ -0,0 +1,209 @@
+// 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.backend;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.git.RefUpdateUtil;
+import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.dircache.DirCache;
+import org.eclipse.jgit.dircache.DirCacheEditor;
+import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit;
+import org.eclipse.jgit.dircache.DirCacheEntry;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.FileMode;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+/**
+ * Class to scan a branch for code owner config files and update them.
+ *
+ * <p>Doesn't parse the code owner config files but provides the raw content to the callback.
+ *
+ * <p>All updates to the code owner config files are done atomically with a single commit.
+ */
+@Singleton
+public class CodeOwnerConfigFileUpdateScanner {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final GitRepositoryManager repoManager;
+ private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final Provider<PersonIdent> serverIdentProvider;
+ private final Provider<IdentifiedUser> identifiedUser;
+
+ @Inject
+ CodeOwnerConfigFileUpdateScanner(
+ GitRepositoryManager repoManager,
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
+ Provider<IdentifiedUser> identifiedUser) {
+ this.repoManager = repoManager;
+ this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.serverIdentProvider = serverIdentProvider;
+ this.identifiedUser = identifiedUser;
+ }
+
+ /**
+ * Visits and updates all code owner config files in the given project and branch.
+ *
+ * <p>All updates are done in a single commit. If none of the code owner config files is updated,
+ * no new commit is created.
+ *
+ * @param branchNameKey the project and branch for which the code owner config files should be
+ * updated
+ * @param commitMessage commit message for the new commit if an update is performed
+ * @param codeOwnerConfigFileUpdater the callback that is invoked for each code owner config file
+ * @return the commit that renamed the email if any update was performed
+ */
+ public Optional<RevCommit> update(
+ BranchNameKey branchNameKey,
+ String commitMessage,
+ CodeOwnerConfigFileUpdater codeOwnerConfigFileUpdater) {
+ requireNonNull(branchNameKey, "branchNameKey");
+ requireNonNull(commitMessage, "commitMessage");
+ requireNonNull(codeOwnerConfigFileUpdater, "codeOwnerConfigFileUpdater");
+
+ CodeOwnerBackend codeOwnerBackend = codeOwnersPluginConfiguration.getBackend(branchNameKey);
+ logger.atFine().log(
+ "updating code owner files in branch %s of project %s",
+ branchNameKey.branch(), branchNameKey.project());
+
+ try (Repository repository = repoManager.openRepository(branchNameKey.project());
+ RevWalk rw = new RevWalk(repository);
+ ObjectInserter oi = repository.newObjectInserter();
+ CodeOwnerConfigTreeWalk treeWalk =
+ new CodeOwnerConfigTreeWalk(
+ codeOwnerBackend,
+ branchNameKey,
+ repository,
+ rw,
+ /** pathGlob */
+ null)) {
+ RevCommit revision = treeWalk.getRevision();
+ DirCache newTree = DirCache.newInCore();
+ DirCacheEditor editor = newTree.editor();
+
+ boolean dirty = false;
+ while (treeWalk.next()) {
+ Optional<String> updatedContent =
+ codeOwnerConfigFileUpdater.update(treeWalk.getFilePath(), treeWalk.getFileContent());
+ if (updatedContent.isPresent()) {
+ dirty = true;
+
+ // insert blob with new file content
+ ObjectId blobId = oi.insert(Constants.OBJ_BLOB, updatedContent.get().getBytes(UTF_8));
+
+ // append edit command to set the new blob for the code owner config file
+ editor.add(createEditCommand(treeWalk.getPathString(), blobId));
+ }
+ }
+
+ if (!dirty) {
+ return Optional.empty();
+ }
+
+ editor.finish();
+ ObjectId treeId = newTree.writeTree(oi);
+ ObjectId commitId = createCommit(oi, commitMessage, revision, treeId);
+ updateBranch(branchNameKey.branch(), repository, revision, commitId);
+ return Optional.of(rw.parseCommit(commitId));
+ } catch (IOException e) {
+ throw new StorageException(
+ String.format(
+ "Failed to scan for code owner configs in branch %s of project %s",
+ branchNameKey.branch(), branchNameKey.project()),
+ e);
+ }
+ }
+
+ /**
+ * Creates an edit command that sets the given blob for the given path
+ *
+ * @param jgitFilePath path of the file for which the blob should be set, as jgit path (not
+ * starting with '/')
+ * @param blobId the ID of the blob that should be set for the file path
+ * @return the edit command
+ */
+ private PathEdit createEditCommand(String jgitFilePath, ObjectId blobId) {
+ return new PathEdit(jgitFilePath) {
+ @Override
+ public void apply(DirCacheEntry entry) {
+ entry.setFileMode(FileMode.REGULAR_FILE);
+ entry.setObjectId(blobId);
+ }
+ };
+ }
+
+ /**
+ * Creates a new commit.
+ *
+ * @param objectInserter object inserter that should be used to insert the new commit
+ * @param commitMessage the commit message that should be used for the new commit
+ * @param parentCommit the commit that should be set as parent commit of the new commit
+ * @param treeId the tree of the new commit
+ * @return the commit ID
+ */
+ private ObjectId createCommit(
+ ObjectInserter objectInserter, String commitMessage, ObjectId parentCommit, ObjectId treeId)
+ throws IOException {
+ PersonIdent serverIdent = serverIdentProvider.get();
+ CommitBuilder cb = new CommitBuilder();
+ cb.setParentId(parentCommit);
+ cb.setTreeId(treeId);
+ cb.setCommitter(serverIdent);
+ cb.setAuthor(
+ identifiedUser.get().newCommitterIdent(serverIdent.getWhen(), serverIdent.getTimeZone()));
+ cb.setMessage(commitMessage);
+ ObjectId id = objectInserter.insert(cb);
+ objectInserter.flush();
+ return id;
+ }
+
+ /**
+ * Update the given branch.
+ *
+ * @param branchName the name of the branch that should be updated
+ * @param repository the repository in which the branch should be updated
+ * @param oldObjectId the expected old object ID of the branch
+ * @param newObjectId the new object ID that should be set for the branch
+ */
+ private void updateBranch(
+ String branchName, Repository repository, ObjectId oldObjectId, ObjectId newObjectId)
+ throws IOException {
+ RefUpdate ru = repository.updateRef(branchName);
+ ru.setExpectedOldObjectId(oldObjectId);
+ ru.setNewObjectId(newObjectId);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdater.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdater.java
new file mode 100644
index 0000000..1c38737
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdater.java
@@ -0,0 +1,32 @@
+// 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.backend;
+
+import java.nio.file.Path;
+import java.util.Optional;
+
+/** Callback interface to update a code owner config file. */
+public interface CodeOwnerConfigFileUpdater {
+ /**
+ * Callback for a code owner config file.
+ *
+ * @param codeOwnerConfigFilePath absolute path of the code owner config file
+ * @param codeOwnerConfigFileContent the content of the code owner config, can be also the content
+ * of a non-parseable code owner config
+ * @return the updated content of the code owner config file, {@link Optional#empty()} if no
+ * update should be performed
+ */
+ Optional<String> update(Path codeOwnerConfigFilePath, String codeOwnerConfigFileContent);
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
index 06bd4ef..f1e8a3b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -14,33 +14,23 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidConfigCause;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
-import java.nio.file.FileSystems;
-import java.nio.file.Path;
-import java.nio.file.Paths;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.treewalk.TreeWalk;
-import org.eclipse.jgit.treewalk.filter.TreeFilter;
/** Class to scan a branch for code owner config files. */
@Singleton
@@ -120,33 +110,13 @@
try (Repository repository = repoManager.openRepository(branchNameKey.project());
RevWalk rw = new RevWalk(repository);
- TreeWalk treeWalk = new TreeWalk(repository)) {
- Ref ref = repository.exactRef(branchNameKey.branch());
- checkState(
- ref != null,
- "branch %s of project %s not found",
- branchNameKey.branch(),
- branchNameKey.project());
-
- RevCommit revision = rw.parseCommit(ref.getObjectId());
- treeWalk.addTree(revision.getTree());
- treeWalk.setRecursive(true);
- treeWalk.setFilter(
- createCodeOwnerConfigFilter(codeOwnerBackend, branchNameKey.project(), pathGlob));
-
+ CodeOwnerConfigTreeWalk treeWalk =
+ new CodeOwnerConfigTreeWalk(
+ codeOwnerBackend, branchNameKey, repository, rw, pathGlob)) {
while (treeWalk.next()) {
- Path filePath = Paths.get(treeWalk.getPathString());
- Path folderPath =
- filePath.getParent() != null
- ? JgitPath.of(filePath.getParent()).getAsAbsolutePath()
- : Paths.get("/");
- String fileName = filePath.getFileName().toString();
- CodeOwnerConfig.Key codeOwnerConfigKey =
- CodeOwnerConfig.Key.create(branchNameKey, folderPath, fileName);
- Optional<CodeOwnerConfig> codeOwnerConfig;
-
+ CodeOwnerConfig codeOwnerConfig;
try {
- codeOwnerConfig = codeOwnerBackend.getCodeOwnerConfig(codeOwnerConfigKey, revision);
+ codeOwnerConfig = treeWalk.getCodeOwnerConfig();
} catch (StorageException storageException) {
Optional<ConfigInvalidException> configInvalidException =
getInvalidConfigCause(storageException);
@@ -157,13 +127,11 @@
// The code owner config is invalid and cannot be parsed.
invalidCodeOwnerConfigCallback.onInvalidCodeOwnerConfig(
- folderPath.resolve(fileName), configInvalidException.get());
+ treeWalk.getFilePath(), configInvalidException.get());
continue;
}
- checkState(codeOwnerConfig.isPresent(), "code owner config %s not found", codeOwnerConfig);
- boolean visitFurtherCodeOwnerConfigFiles =
- codeOwnerConfigVisitor.visit(codeOwnerConfig.get());
+ boolean visitFurtherCodeOwnerConfigFiles = codeOwnerConfigVisitor.visit(codeOwnerConfig);
if (!visitFurtherCodeOwnerConfigFiles) {
break;
}
@@ -178,47 +146,6 @@
}
/**
- * Creates a {@link TreeFilter} that matches code owner config files in the given project.
- *
- * @param codeOwnerBackend the code owner backend that is being used
- * @param project the name of the project in which code owner config files should be matched
- * @param pathGlob optional Java NIO glob that the paths of code owner config files must match
- * @return the created {@link TreeFilter}
- */
- private static TreeFilter createCodeOwnerConfigFilter(
- CodeOwnerBackend codeOwnerBackend, Project.NameKey project, @Nullable String pathGlob) {
- return new TreeFilter() {
- @Override
- public boolean shouldBeRecursive() {
- return true;
- }
-
- @Override
- public boolean include(TreeWalk walker) throws IOException {
- if (walker.isSubtree()) {
- walker.enterSubtree();
- return false;
- }
- if (pathGlob != null
- && !FileSystems.getDefault()
- .getPathMatcher("glob:" + pathGlob)
- .matches(JgitPath.of(walker.getPathString()).getAsAbsolutePath())) {
- logger.atFine().log(
- "%s filtered out because it doesn't match the path glob", walker.getPathString());
- return false;
- }
- String fileName = Paths.get(walker.getPathString()).getFileName().toString();
- return codeOwnerBackend.isCodeOwnerConfigFile(project, fileName);
- }
-
- @Override
- public TreeFilter clone() {
- return this;
- }
- };
- }
-
- /**
* Returns an {@link InvalidCodeOwnerConfigCallback} instance that ignores invalid code owner
* config files.
*/
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigTreeWalk.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigTreeWalk.java
new file mode 100644
index 0000000..1ba8f74
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigTreeWalk.java
@@ -0,0 +1,175 @@
+// 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.backend;
+
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.plugins.codeowners.JgitPath;
+import java.io.IOException;
+import java.nio.file.FileSystems;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.treewalk.filter.TreeFilter;
+import org.eclipse.jgit.util.RawParseUtils;
+
+/** {@link TreeWalk} that filters for code owner config files in the tree. */
+public class CodeOwnerConfigTreeWalk extends TreeWalk {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final CodeOwnerBackend codeOwnerBackend;
+ private final BranchNameKey branchNameKey;
+ private final RevCommit revision;
+
+ public CodeOwnerConfigTreeWalk(
+ CodeOwnerBackend codeOwnerBackend,
+ BranchNameKey branchNameKey,
+ Repository repository,
+ RevWalk revWalk,
+ @Nullable String pathGlob)
+ throws IOException {
+ super(repository);
+
+ this.codeOwnerBackend = requireNonNull(codeOwnerBackend, "codeOwnerBackend");
+ this.branchNameKey = requireNonNull(branchNameKey, "branchNameKey");
+ this.revision =
+ getRevision(
+ branchNameKey,
+ requireNonNull(repository, "repository"),
+ requireNonNull(revWalk, "revWalk"));
+
+ addTree(revision.getTree());
+ setRecursive(true);
+ setFilter(createCodeOwnerConfigFilter(codeOwnerBackend, branchNameKey.project(), pathGlob));
+ }
+
+ /**
+ * Returns the revision from which the tree was loaded.
+ *
+ * @return the revision ID
+ */
+ public RevCommit getRevision() {
+ return revision;
+ }
+
+ /** Returns the absolute file path of the current entry. */
+ public Path getFilePath() {
+ return JgitPath.of(getPathString()).getAsAbsolutePath();
+ }
+
+ /** Returns the file content of the current entry. */
+ public String getFileContent() throws IOException {
+ ObjectLoader obj = getObjectReader().open(getObjectId(0), Constants.OBJ_BLOB);
+ byte[] raw = obj.getCachedBytes(Integer.MAX_VALUE);
+ return raw.length != 0 ? RawParseUtils.decode(raw) : "";
+ }
+
+ /** Returns the code owner config key of the current entry. */
+ public CodeOwnerConfig.Key getCodeOwnerConfigKey() {
+ Path filePath = getFilePath();
+ Path folderPath =
+ filePath.getParent() != null
+ ? JgitPath.of(filePath.getParent()).getAsAbsolutePath()
+ : Paths.get("/");
+ String fileName = Paths.get(getPathString()).getFileName().toString();
+ return CodeOwnerConfig.Key.create(branchNameKey, folderPath, fileName);
+ }
+
+ /**
+ * Loads the code owner config file at the current entry's path.
+ *
+ * @return the loaded code owner config
+ */
+ public CodeOwnerConfig getCodeOwnerConfig() {
+ CodeOwnerConfig.Key codeOwnerConfigKey = getCodeOwnerConfigKey();
+ return codeOwnerBackend
+ .getCodeOwnerConfig(codeOwnerConfigKey, revision)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format("code owner config %s not found", codeOwnerConfigKey)));
+ }
+
+ /**
+ * Looks up the current revision of the branch.
+ *
+ * @param branchNameKey the project and branch for which the current revision should be loaded
+ * @param repository the repository from which the branch revision should be loaded
+ * @return the current revision of the branch
+ */
+ private static RevCommit getRevision(
+ BranchNameKey branchNameKey, Repository repository, RevWalk revWalk) throws IOException {
+ Ref ref = repository.exactRef(branchNameKey.branch());
+ checkState(
+ ref != null,
+ "branch %s of project %s not found",
+ branchNameKey.branch(),
+ branchNameKey.project());
+
+ return revWalk.parseCommit(ref.getObjectId());
+ }
+
+ /**
+ * Creates a {@link TreeFilter} that matches code owner config files in the given project.
+ *
+ * @param codeOwnerBackend the code owner backend that is being used
+ * @param project the name of the project in which code owner config files should be matched
+ * @param pathGlob optional Java NIO glob that the paths of code owner config files must match
+ * @return the created {@link TreeFilter}
+ */
+ private static TreeFilter createCodeOwnerConfigFilter(
+ CodeOwnerBackend codeOwnerBackend, Project.NameKey project, @Nullable String pathGlob) {
+ return new TreeFilter() {
+ @Override
+ public boolean shouldBeRecursive() {
+ return true;
+ }
+
+ @Override
+ public boolean include(TreeWalk walker) throws IOException {
+ if (walker.isSubtree()) {
+ walker.enterSubtree();
+ return false;
+ }
+ if (pathGlob != null
+ && !FileSystems.getDefault()
+ .getPathMatcher("glob:" + pathGlob)
+ .matches(JgitPath.of(walker.getPathString()).getAsAbsolutePath())) {
+ logger.atFine().log(
+ "%s filtered out because it doesn't match the path glob", walker.getPathString());
+ return false;
+ }
+ String fileName = Paths.get(walker.getPathString()).getFileName().toString();
+ return codeOwnerBackend.isCodeOwnerConfigFile(project, fileName);
+ }
+
+ @Override
+ public TreeFilter clone() {
+ return this;
+ }
+ };
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index e18ddb3..07788ca 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -154,7 +154,7 @@
problemsByPath.put(
codeOwnerConfigFilePath.toString(),
new ConsistencyProblemInfo(
- ConsistencyProblemInfo.Status.ERROR, configInvalidException.getMessage()));
+ ConsistencyProblemInfo.Status.FATAL, configInvalidException.getMessage()));
},
pathGlob);
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 7fbda44..85cfca4 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -517,11 +517,11 @@
}
/**
- * Returns the {@link com.google.gerrit.server.git.validators.ValidationMessage.Type} (ERROR or
+ * Returns the {@link com.google.gerrit.server.git.validators.ValidationMessage.Type} (FATAL or
* WARNING) that should be used for a parsing error of the code owner config file (specified as
* {@code changedFile}).
*
- * <p>If {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#ERROR} is returned
+ * <p>If {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#FATAL} is returned
* the upload will be blocked, if {@link
* com.google.gerrit.server.git.validators.ValidationMessage.Type#WARNING} is returned the upload
* can succeed and the parsing error will only be shown as warning.
@@ -531,7 +531,7 @@
* it is not making anything worse. Hence in this case the parsing error should be returned as
* {@link com.google.gerrit.server.git.validators.ValidationMessage.Type#WARNING}, whereas a new
* parsing error should be returned as {@link
- * com.google.gerrit.server.git.validators.ValidationMessage.Type#ERROR}.
+ * com.google.gerrit.server.git.validators.ValidationMessage.Type#FATAL}.
*
* @param codeOwnerBackend the code owner backend from which the code owner config can be loaded
* @param branchNameKey the project and branch of the code owner config
@@ -567,8 +567,8 @@
codeOwnerBackend.getCodeOwnerConfig(baseCodeOwnerConfigKey, revWalk, parentRevision);
// The code owner config at the parent revision is parseable. This means the parsing error
// is introduced by the new commit and we should block uploading it, which we achieve by
- // setting the validation message type to error.
- return ValidationMessage.Type.ERROR;
+ // setting the validation message type to fatal.
+ return ValidationMessage.Type.FATAL;
} catch (StorageException storageException) {
// Loading the base code owner config has failed.
if (getInvalidConfigCause(storageException).isPresent()) {
@@ -584,8 +584,8 @@
// The code owner config is newly created. Hence the parsing error comes from the commit
// that is being pushed and we want to block it from uploading. To do this we set the
- // validation message type to error.
- return ValidationMessage.Type.ERROR;
+ // validation message type to fatal.
+ return ValidationMessage.Type.FATAL;
}
/**
@@ -608,12 +608,13 @@
/**
* Returns a copy of the given commit validation message with type warning if the type the given
- * commit validation message is error. Otherwise it returns the given commit validation message
- * unchanged.
+ * commit validation message is fatal or error. Otherwise it returns the given commit validation
+ * message unchanged.
*/
private static CommitValidationMessage downgradeErrorToWarning(
CommitValidationMessage commitValidationMessage) {
- if (CommitValidationMessage.Type.ERROR.equals(commitValidationMessage.getType())) {
+ if (CommitValidationMessage.Type.FATAL.equals(commitValidationMessage.getType())
+ || CommitValidationMessage.Type.ERROR.equals(commitValidationMessage.getType())) {
return new CommitValidationMessage(
commitValidationMessage.getMessage(), ValidationMessage.Type.WARNING);
}
@@ -977,7 +978,8 @@
return validationMessages().stream()
.anyMatch(
validationMessage ->
- ValidationMessage.Type.ERROR.equals(validationMessage.getType()));
+ ValidationMessage.Type.FATAL.equals(validationMessage.getType())
+ || ValidationMessage.Type.ERROR.equals(validationMessage.getType()));
}
private ImmutableList<CommitValidationMessage> validationMessagesWithIncludedSummaryMessage() {
@@ -995,6 +997,7 @@
* <p>The following validation message type will be returned:
*
* <ul>
+ * <li>FATAL: if any of the validation message has type fatal
* <li>ERROR: if any of the validation message has type error
* <li>WARNING: if any of the validation message has type warning and none has type error
* <li>HINT: otherwise
@@ -1005,6 +1008,10 @@
if (!validationMessages().isEmpty()) {
for (CommitValidationMessage validationMessage : validationMessages()) {
+ if (ValidationMessage.Type.FATAL.equals(validationMessage.getType())) {
+ validationMessageType = ValidationMessage.Type.FATAL;
+ break;
+ }
if (ValidationMessage.Type.ERROR.equals(validationMessage.getType())) {
validationMessageType = ValidationMessage.Type.ERROR;
break;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
index c1d7f0b..2c7b15d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -159,7 +159,7 @@
ImmutableMap.of(
codeOwnerConfigPath,
ImmutableList.of(
- error(
+ fatal(
String.format(
"invalid code owner config file '%s':\n %s",
codeOwnerConfigPath,
@@ -475,6 +475,10 @@
pathOfInvalidConfig3)))));
}
+ private ConsistencyProblemInfo fatal(String message) {
+ return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.FATAL, message);
+ }
+
private ConsistencyProblemInfo error(String message) {
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
index 9f4d64d..57b9506 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
@@ -126,7 +126,7 @@
.containsExactly(
codeOwnerConfigPath,
ImmutableList.of(
- error(
+ fatal(
String.format(
"invalid code owner config file '%s':\n %s",
codeOwnerConfigPath,
@@ -398,6 +398,10 @@
unknownEmail3, codeOwnerConfigPath3))));
}
+ private ConsistencyProblemInfo fatal(String message) {
+ return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.FATAL, message);
+ }
+
private ConsistencyProblemInfo error(String message) {
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
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 357c5d8..bc42987 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -319,7 +319,7 @@
"Add code owners",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
"INVALID");
- assertOkWithErrors(
+ assertOkWithFatals(
r,
"invalid code owner config files",
String.format(
@@ -552,7 +552,7 @@
"Add code owners",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
"INVALID");
- assertErrorWithMessages(
+ assertFatalWithMessages(
r,
"invalid code owner config files",
String.format(
@@ -582,7 +582,7 @@
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getJGitFilePath(),
"ALSO-INVALID"));
PushOneCommit.Result r = push.to("refs/for/master");
- assertErrorWithMessages(
+ assertFatalWithMessages(
r,
"invalid code owner config files",
String.format(
@@ -1481,6 +1481,7 @@
private static void assertOkWithoutMessages(PushOneCommit.Result pushResult) {
pushResult.assertOkStatus();
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
@@ -1493,10 +1494,23 @@
pushResult.assertMessage(
String.format("hint: commit %s: %s", abbreviateName(pushResult.getCommit()), hint));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
}
+ private void assertOkWithFatals(PushOneCommit.Result pushResult, String... errors)
+ throws Exception {
+ pushResult.assertOkStatus();
+ for (String error : errors) {
+ pushResult.assertMessage(
+ String.format("fatal: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
+ }
+ pushResult.assertNotMessage("error");
+ pushResult.assertNotMessage("warning");
+ pushResult.assertNotMessage("hint");
+ }
+
private void assertOkWithErrors(PushOneCommit.Result pushResult, String... errors)
throws Exception {
pushResult.assertOkStatus();
@@ -1504,6 +1518,7 @@
pushResult.assertMessage(
String.format("error: commit %s: %s", abbreviateName(pushResult.getCommit()), error));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
}
@@ -1515,6 +1530,7 @@
pushResult.assertMessage(
String.format("warning: commit %s: %s", abbreviateName(pushResult.getCommit()), warning));
}
+ pushResult.assertNotMessage("fatal");
pushResult.assertNotMessage("error");
pushResult.assertNotMessage("hint");
}
@@ -1526,6 +1542,19 @@
for (String error : errors) {
pushResult.assertMessage(String.format("error: commit %s: %s", abbreviatedCommit, error));
}
+ pushResult.assertNotMessage("fatal");
+ pushResult.assertNotMessage("warning");
+ pushResult.assertNotMessage("hint");
+ }
+
+ private void assertFatalWithMessages(
+ PushOneCommit.Result pushResult, String summaryMessage, String... errors) throws Exception {
+ String abbreviatedCommit = abbreviateName(pushResult.getCommit());
+ pushResult.assertErrorStatus(String.format("commit %s: %s", abbreviatedCommit, summaryMessage));
+ for (String error : errors) {
+ pushResult.assertMessage(String.format("fatal: commit %s: %s", abbreviatedCommit, error));
+ }
+ pushResult.assertNotMessage("error");
pushResult.assertNotMessage("warning");
pushResult.assertNotMessage("hint");
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScannerTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScannerTest.java
new file mode 100644
index 0000000..b00b8b1
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScannerTest.java
@@ -0,0 +1,287 @@
+// 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.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.plugins.codeowners.JgitPath;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Optional;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+/** Tests for {@link CodeOwnerConfigFileUpdateScanner}. */
+public class CodeOwnerConfigFileUpdateScannerTest extends AbstractCodeOwnersTest {
+ @Rule public final MockitoRule mockito = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
+
+ @Mock private CodeOwnerConfigFileUpdater updater;
+
+ @Inject private ProjectOperations projectOperations;
+
+ private CodeOwnerConfigOperations codeOwnerConfigOperations;
+ private CodeOwnerConfigFileUpdateScanner codeOwnerConfigFileUpdateScanner;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ codeOwnerConfigFileUpdateScanner =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigFileUpdateScanner.class);
+ }
+
+ @Test
+ public void cannotUpdateCodeOwnerConfigsForNullBranch() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ codeOwnerConfigFileUpdateScanner.update(
+ null,
+ "Update code owner configs",
+ (codeOwnerConfigFilePath, codeOwnerConfigFileContent) -> Optional.empty()));
+ assertThat(npe).hasMessageThat().isEqualTo("branchNameKey");
+ }
+
+ @Test
+ public void cannotUpdateCodeOwnerConfigsWithNullCommitMessage() throws Exception {
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "master");
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ codeOwnerConfigFileUpdateScanner.update(
+ branchNameKey,
+ null,
+ (codeOwnerConfigFilePath, codeOwnerConfigFileContent) -> Optional.empty()));
+ assertThat(npe).hasMessageThat().isEqualTo("commitMessage");
+ }
+
+ @Test
+ public void cannotUpdateCodeOwnerConfigsWithNullUpdater() throws Exception {
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "master");
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ codeOwnerConfigFileUpdateScanner.update(
+ branchNameKey, "Update code owner configs", null));
+ assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfigFileUpdater");
+ }
+
+ @Test
+ public void cannotUpdateCodeOwnerConfigsForNonExistingBranch() throws Exception {
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "non-existing");
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ codeOwnerConfigFileUpdateScanner.update(
+ branchNameKey,
+ "Update code owner configs",
+ (codeOwnerConfigFilePath, codeOwnerConfigFileContent) -> Optional.empty()));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "branch %s of project %s not found", branchNameKey.branch(), project.get()));
+ }
+
+ @Test
+ public void noUpdateIfNoCodeOwnerConfigFilesExists() throws Exception {
+ Optional<RevCommit> commit =
+ codeOwnerConfigFileUpdateScanner.update(
+ BranchNameKey.create(project, "master"), "Update code owner configs", updater);
+ assertThat(commit).isEmpty();
+ verifyZeroInteractions(updater);
+ }
+
+ @Test
+ public void noUpdateForNonCodeOwnerConfigFiles() throws Exception {
+ // Create some non code owner config files.
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ Ref ref = testRepo.getRepository().exactRef("refs/heads/master");
+ RevCommit head = testRepo.getRevWalk().parseCommit(ref.getObjectId());
+ testRepo.update(
+ "refs/heads/master",
+ testRepo
+ .commit()
+ .parent(head)
+ .message("Add some non code owner config files")
+ .add("owners.txt", "some content")
+ .add("owners", "some content")
+ .add("foo/bar/owners.txt", "some content")
+ .add("foo/bar/owners", "some content"));
+ }
+
+ Optional<RevCommit> commit =
+ codeOwnerConfigFileUpdateScanner.update(
+ BranchNameKey.create(project, "master"), "Update code owner configs", updater);
+ assertThat(commit).isEmpty();
+ verifyZeroInteractions(updater);
+ }
+
+ @Test
+ public void noUpdateIfCallbackDoesntReturnNewFileContent() throws Exception {
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .fileName("OWNERS")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+ Path path =
+ Paths.get(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath());
+ String content = codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent();
+
+ RevCommit oldHead = projectOperations.project(project).getHead("master");
+
+ when(updater.update(path, content)).thenReturn(Optional.empty());
+ Optional<RevCommit> commit =
+ codeOwnerConfigFileUpdateScanner.update(
+ BranchNameKey.create(project, "master"), "Update code owner configs", updater);
+ assertThat(commit).isEmpty();
+
+ // Verify the code owner config file was not updated.
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent())
+ .isEqualTo(content);
+
+ // Check that no commit was created.
+ RevCommit newHead = projectOperations.project(project).getHead("master");
+ assertThat(newHead).isEqualTo(oldHead);
+ }
+
+ @Test
+ public void updateCodeOwnerConfigFiles() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ CodeOwnerConfig.Key codeOwnerConfigKey1 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .fileName("OWNERS")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+ Path path1 =
+ Paths.get(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getFilePath());
+ String oldContent1 =
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getContent();
+ String newContent1 = user.email() + "\n";
+ when(updater.update(path1, oldContent1)).thenReturn(Optional.of(newContent1));
+
+ CodeOwnerConfig.Key codeOwnerConfigKey2 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .fileName("OWNERS")
+ .addCodeOwnerEmail(user.email())
+ .create();
+ Path path2 =
+ Paths.get(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getFilePath());
+ String oldContent2 =
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getContent();
+ String newContent2 = user2.email() + "\n";
+ when(updater.update(path2, oldContent2)).thenReturn(Optional.of(newContent2));
+
+ RevCommit oldHead = projectOperations.project(project).getHead("master");
+
+ String commitMessage = "Update code owner configs";
+ Optional<RevCommit> commit =
+ codeOwnerConfigFileUpdateScanner.update(
+ BranchNameKey.create(project, "master"), commitMessage, updater);
+ assertThat(commit).isPresent();
+
+ // Verify that we received the expected callbacks for the invalid code onwer config.
+ Mockito.verify(updater).update(path1, oldContent1);
+ Mockito.verify(updater).update(path2, oldContent2);
+ verifyNoMoreInteractions(updater);
+
+ // Verify the code owner config files were updated.
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getContent())
+ .isEqualTo(newContent1);
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getContent())
+ .isEqualTo(newContent2);
+
+ // Check that exactly 1 commit was created.
+ RevCommit newHead = projectOperations.project(project).getHead("master");
+ assertThat(commit.get()).isEqualTo(newHead);
+ assertThat(newHead).isNotEqualTo(oldHead);
+ assertThat(newHead.getShortMessage()).isEqualTo(commitMessage);
+ assertThat(newHead.getParent(0)).isEqualTo(oldHead);
+ }
+
+ @Test
+ public void updateInvalidCodeOwnerConfigFile() throws Exception {
+ CodeOwnerConfig.Key codeOwnerConfigKey = createInvalidCodeOwnerConfig("/OWNERS", "INVALID");
+
+ when(updater.update(any(Path.class), any(String.class)))
+ .thenReturn(Optional.of("STILL INVALID"));
+ Optional<RevCommit> update =
+ codeOwnerConfigFileUpdateScanner.update(
+ BranchNameKey.create(project, "master"), "Update code owner configs", updater);
+ assertThat(update).isPresent();
+
+ // Verify that we received the expected callbacks for the invalid code onwer config.
+ Mockito.verify(updater).update(Paths.get("/OWNERS"), "INVALID");
+ verifyNoMoreInteractions(updater);
+
+ // Verify the code owner config file was updated.
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent())
+ .isEqualTo("STILL INVALID");
+ }
+
+ private CodeOwnerConfig.Key createInvalidCodeOwnerConfig(String filePath, String content)
+ throws Exception {
+ disableCodeOwnersForProject(project);
+ String changeId =
+ createChange("Add invalid code owners file", JgitPath.of(filePath).get(), content)
+ .getChangeId();
+ approve(changeId);
+ gApi.changes().id(changeId).current().submit();
+ enableCodeOwnersForProject(project);
+ Path path = Paths.get(filePath);
+ return CodeOwnerConfig.Key.create(
+ project, "master", path.getParent().toString(), path.getFileName().toString());
+ }
+}
