Merge "Use new Get Code Owner Branch Config REST endpoint in frontend"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index 027075f..101845a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -17,6 +17,8 @@
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.UserInitiated;
@@ -40,6 +42,8 @@
}
install(new CodeOwnerSubmitRule.Module());
+
+ DynamicSet.bind(binder(), ExceptionHook.class).to(CodeOwnersExceptionHook.class);
}
@Provides
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 7e402c7..36d8cc9 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -30,6 +30,7 @@
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.RequiredApproval;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -49,6 +50,7 @@
import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -80,6 +82,7 @@
private final CodeOwnerConfigScanner codeOwnerConfigScanner;
private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
+ private final ApprovalsUtil approvalsUtil;
@Inject
CodeOwnerApprovalCheck(
@@ -89,7 +92,8 @@
ChangedFiles changedFiles,
CodeOwnerConfigScanner codeOwnerConfigScanner,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
- Provider<CodeOwnerResolver> codeOwnerResolver) {
+ Provider<CodeOwnerResolver> codeOwnerResolver,
+ ApprovalsUtil approvalsUtil) {
this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
@@ -97,6 +101,7 @@
this.codeOwnerConfigScanner = codeOwnerConfigScanner;
this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
this.codeOwnerResolver = codeOwnerResolver;
+ this.approvalsUtil = approvalsUtil;
}
/**
@@ -612,7 +617,18 @@
*/
private ImmutableSet<Account.Id> getApproverAccountIds(
RequiredApproval requiredApproval, ChangeNotes changeNotes) {
- return changeNotes.getApprovals().get(changeNotes.getCurrentPatchSet().id()).stream()
+ return StreamSupport.stream(
+ approvalsUtil
+ .byPatchSet(
+ changeNotes,
+ changeNotes.getCurrentPatchSet().id(),
+ /** revWalk */
+ null,
+ /** repoConfig */
+ null)
+ .spliterator(),
+ /** parallel */
+ false)
.filter(requiredApproval::isApprovedBy)
.map(PatchSetApproval::accountId)
.collect(toImmutableSet());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
new file mode 100644
index 0000000..b4e2931
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -0,0 +1,70 @@
+// 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 com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.plugins.codeowners.config.InvalidPluginConfigurationException;
+import com.google.gerrit.server.ExceptionHook;
+import java.util.Optional;
+
+/**
+ * Class to define the HTTP response status code and message for exceptions that can occur for all
+ * REST endpoints and which should not result in a 500 Internal Server Error.
+ *
+ * <p>The following exceptions are handled:
+ *
+ * <ul>
+ * <li>exception due to invalid plugin configuration ({@link
+ * InvalidPluginConfigurationException}): mapped to {@code 409 Conflict}
+ * <li>exception due to invalid code owner config files ({@link
+ * org.eclipse.jgit.errors.ConfigInvalidException}): mapped to {@code 409 Conflict}
+ * </ul>
+ */
+public class CodeOwnersExceptionHook implements ExceptionHook {
+ @Override
+ public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
+ return isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable);
+ }
+
+ @Override
+ public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
+ if (isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)) {
+ return ImmutableList.of(throwable.getMessage());
+ }
+ return ImmutableList.of();
+ }
+
+ @Override
+ public Optional<Status> getStatus(Throwable throwable) {
+ if (isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)) {
+ return Optional.of(Status.create(409, "Conflict"));
+ }
+ return Optional.empty();
+ }
+
+ private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
+ return Throwables.getCausalChain(throwable).stream()
+ .anyMatch(t -> t instanceof InvalidPluginConfigurationException);
+ }
+
+ private static boolean isInvalidCodeOwnerConfigException(Throwable throwable) {
+ return CodeOwners.getInvalidConfigCause(throwable).isPresent();
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/config/ConfigModule.java b/java/com/google/gerrit/plugins/codeowners/config/ConfigModule.java
index a2193c1..ec439ad 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/ConfigModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/ConfigModule.java
@@ -15,7 +15,6 @@
package com.google.gerrit.plugins.codeowners.config;
import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.inject.AbstractModule;
@@ -25,7 +24,5 @@
protected void configure() {
DynamicSet.bind(binder(), CommitValidationListener.class)
.to(CodeOwnersPluginConfigValidator.class);
- DynamicSet.bind(binder(), ExceptionHook.class)
- .to(InvalidPluginConfigurationException.ExceptionHook.class);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java b/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
index 08f75bc..dd78ada 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
@@ -14,52 +14,11 @@
package com.google.gerrit.plugins.codeowners.config;
-import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwners;
-import java.util.Optional;
-
/**
* Exception that is thrown if a configuration parameter of the code-owners plugin has an invalid
* value.
*/
public class InvalidPluginConfigurationException extends RuntimeException {
- public static class ExceptionHook implements com.google.gerrit.server.ExceptionHook {
- @Override
- public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
- return isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable);
- }
-
- @Override
- public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)) {
- return ImmutableList.of(throwable.getMessage());
- }
- return ImmutableList.of();
- }
-
- @Override
- public Optional<Status> getStatus(Throwable throwable) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)) {
- return Optional.of(Status.create(409, "Conflict"));
- }
- return Optional.empty();
- }
-
- private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
- return Throwables.getCausalChain(throwable).stream()
- .anyMatch(t -> t instanceof InvalidPluginConfigurationException);
- }
-
- private static boolean isInvalidCodeOwnerConfigException(Throwable throwable) {
- return CodeOwners.getInvalidConfigCause(throwable).isPresent();
- }
- }
-
private static final long serialVersionUID = 1L;
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 1249301..5ea1eb4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -1870,6 +1870,137 @@
assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void approvedByStickyApprovalOnOldPatchSet() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // Create a code owner config file with 'user' as code owner
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Let the 'user' approve the change.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+
+ // Change some other file ('user' who uploads the change is a code owner and hence owner
+ // approvals are implicit for this change)
+ String changeId2 =
+ createChange(user, "Change Other File", "other.txt", "file content").getChangeId();
+ approve(changeId2);
+ gApi.changes().id(changeId2).current().submit();
+
+ // Rebase the first change (trivial rebase).
+ gApi.changes().id(changeId).rebase();
+
+ // Check that the approval from 'user' is still there (since Code-Review is sticky on trivial
+ // rebase).
+ assertThat(gApi.changes().id(changeId).get().labels.get("Code-Review").approved.email)
+ .isEqualTo(user.email());
+
+ // Check that the file is still approved.
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+1")
+ public void codeReviewPlus2CountsAsApprovalIfCodeReviewPlus1IsRequired() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // Create a code owner config file with 'user' as code owner
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create a change as 'user2' that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Let 'user' approve the change (vote Code-Review+2)
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 5f3724c..b67693b 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -141,6 +141,10 @@
The configured label must exist for all projects for which this setting
applies (all projects that have code owners enabled and for which this
setting is not overridden).\
+ If the definition of the configured label has [copy
+ rules](../../../Documentation/config-labels.html#label_copyAnyScore)
+ enabled so that votes are sticky across patch sets, also the code owner
+ approvals will be sticky.\
Can be overridden per project by setting
[codeOwners.requiredApproval](#codeOwnersRequiredApproval) in
`@PLUGIN@.config`.\
@@ -363,6 +367,10 @@
The configured label must exist for all projects for which this setting
applies (all child projects that have code owners enabled and for which
this setting is not overridden).\
+ If the definition of the configured label has [copy
+ rules](../../../Documentation/config-labels.html#label_copyAnyScore)
+ enabled so that votes are sticky across patch sets, also the code owner
+ approvals will be sticky.\
Overrides the global setting
[plugin.@PLUGIN@.requiredApproval](#pluginCodeOwnersRequiredApproval) in
`gerrit.config`.\
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 9afd554..22fd38a 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -455,6 +455,16 @@
}
```
+## <a id="general-responses"> General Responses
+
+All REST endpoints may return the following responses:
+
+* `409 Conflict` is returned if a request cannot be executed due to:
+** an non-parseable code owner config file (in this case the project owners need
+ to fix the code owner config file)
+** an invalid plugin configuration (in this case the project owners need to fix
+ the code-owners plugin configuration)
+
## <a id="ids"> IDs
### <a id="path"> \{path\}
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index f94044f..c662be7 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -219,6 +219,12 @@
(e.g. change the `Code-Review` label definition to not require a `Code-Review+2`
vote for change submission.
+**NOTE:** Whether code owner approvals are sticky across patch sets depends on
+the definition of the required label. If the label definition has [copy
+rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so
+that votes are sticky across patch sets, then also the code owner approvals
+which are based on these votes will be sticky.
+
### <a id="grantCodeOwnerPermissions">5. Grant code owners permission to vote on the label that counts as code owner approval
Code owners must be granted permissions to vote on the label that counts as code
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md
index 8e98f59..83194b2 100644
--- a/resources/Documentation/user-guide.md
+++ b/resources/Documentation/user-guide.md
@@ -86,6 +86,12 @@
non-code-owner approvals are required and missing, or if further non-code-owner
submit requirements are not fulfilled yet.
+**NOTE:** Whether code owner approvals are sticky across patch sets depends on
+the definition of the required label. If the label definition has [copy
+rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so
+that votes are sticky across patch sets, then also the code owner approvals
+which are based on these votes will be sticky.
+
## <a id="codeOwnerOverride">Code owner override
Usually some privileged users, such as sheriffs, are allowed to override the
