Merge changes I3adba5cb,I9ba44609,I7647bd97,I1759c5ba,I50f06e4e
* changes:
OnCodeOwnerApproval/OnCodeOwnerOverride: Remove wrong error log
Validation: Log exceptions that happen in dry run mode of as a warning
Add retrying to add change message on add reviewer
ChangeFiles: Limit the number of changed files that are written to trace
Consider only emails that belong to multiple active accounts as ambiguous
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 1e76b05..787e452 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -59,6 +59,8 @@
public class ChangedFiles {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static int MAX_CHANGED_FILES_TO_LOG = 25;
+
private final GitRepositoryManager repoManager;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final PatchListCache patchListCache;
@@ -203,7 +205,14 @@
List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, revCommit);
ImmutableSet<ChangedFile> changedFiles =
diffEntries.stream().map(ChangedFile::create).collect(toImmutableSet());
- logger.atFine().log("changed files = %s", changedFiles);
+ if (changedFiles.size() <= MAX_CHANGED_FILES_TO_LOG) {
+ logger.atFine().log("changed files = %s", changedFiles);
+ } else {
+ logger.atFine().log(
+ "changed files = %s (and %d more)",
+ changedFiles.asList().subList(0, MAX_CHANGED_FILES_TO_LOG),
+ changedFiles.size() - MAX_CHANGED_FILES_TO_LOG);
+ }
return changedFiles;
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 5cb187f..cd874ee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -21,6 +21,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
@@ -302,24 +303,14 @@
return OptionalResultWithMessages.createEmpty(messages);
}
- OptionalResultWithMessages<Account.Id> lookupEmailResult = lookupEmail(email);
- messages.addAll(lookupEmailResult.messages());
- if (lookupEmailResult.isEmpty()) {
+ OptionalResultWithMessages<AccountState> activeAccountResult =
+ lookupActiveAccountForEmail(email);
+ messages.addAll(activeAccountResult.messages());
+ if (activeAccountResult.isEmpty()) {
return OptionalResultWithMessages.createEmpty(messages);
}
- Account.Id accountId = lookupEmailResult.get();
- OptionalResultWithMessages<AccountState> lookupAccountResult = lookupAccount(accountId, email);
- messages.addAll(lookupAccountResult.messages());
- if (lookupAccountResult.isEmpty()) {
- return OptionalResultWithMessages.createEmpty(messages);
- }
-
- AccountState accountState = lookupAccountResult.get();
- if (!accountState.account().isActive()) {
- messages.add(String.format("account %s for email %s is inactive", accountId, email));
- return OptionalResultWithMessages.createEmpty(messages);
- }
+ AccountState accountState = activeAccountResult.get();
if (enforceVisibility) {
OptionalResultWithMessages<Boolean> isVisibleResult = isVisible(accountState, email);
messages.addAll(isVisibleResult.messages());
@@ -343,15 +334,15 @@
}
/**
- * Looks up an email and returns the ID of the account to which it belongs.
+ * Looks up an email and returns the ID of the active account to which it belongs.
*
- * <p>If the email is ambiguous (it belongs to multiple accounts) it is considered as
- * non-resolvable and {@link Optional#empty()} is returned.
+ * <p>If the email is ambiguous (it belongs to multiple active accounts) it is considered as
+ * non-resolvable and empty result is returned.
*
* @param email the email that should be looked up
* @return the ID of the account to which the email belongs if was found
*/
- private OptionalResultWithMessages<Account.Id> lookupEmail(String email) {
+ private OptionalResultWithMessages<AccountState> lookupActiveAccountForEmail(String email) {
ImmutableSet<ExternalId> extIds;
try {
extIds = externalIds.byEmail(email);
@@ -366,12 +357,51 @@
"cannot resolve code owner email %s: no account with this email exists", email));
}
- if (extIds.stream().map(ExternalId::accountId).distinct().count() > 1) {
- return OptionalResultWithMessages.createEmpty(
- String.format("cannot resolve code owner email %s: email is ambiguous", email));
+ List<String> messages = new ArrayList<>();
+ OptionalResultWithMessages<ImmutableSet<AccountState>> activeAccountsResult =
+ lookupActiveAccounts(extIds, email);
+ ImmutableSet<AccountState> activeAccounts = activeAccountsResult.get();
+ messages.addAll(activeAccountsResult.messages());
+
+ if (activeAccounts.isEmpty()) {
+ messages.add(
+ String.format(
+ "cannot resolve code owner email %s: no active account with this email found",
+ email));
+ return OptionalResultWithMessages.createEmpty(messages);
}
- return OptionalResultWithMessages.create(extIds.stream().findFirst().get().accountId());
+ if (activeAccounts.size() > 1) {
+ messages.add(String.format("cannot resolve code owner email %s: email is ambiguous", email));
+ return OptionalResultWithMessages.createEmpty(messages);
+ }
+
+ return OptionalResultWithMessages.create(Iterables.getOnlyElement(activeAccounts));
+ }
+
+ private OptionalResultWithMessages<ImmutableSet<AccountState>> lookupActiveAccounts(
+ ImmutableSet<ExternalId> extIds, String email) {
+ ImmutableSet<OptionalResultWithMessages<AccountState>> accountStateResults =
+ extIds.stream()
+ .map(externalId -> lookupAccount(externalId.accountId(), externalId.email()))
+ .collect(toImmutableSet());
+
+ ImmutableSet.Builder<AccountState> activeAccounts = ImmutableSet.builder();
+ List<String> messages = new ArrayList<>();
+ for (OptionalResultWithMessages<AccountState> accountStateResult : accountStateResults) {
+ messages.addAll(accountStateResult.messages());
+ if (accountStateResult.isPresent()) {
+ AccountState accountState = accountStateResult.get();
+ if (accountState.account().isActive()) {
+ activeAccounts.add(accountState);
+ } else {
+ messages.add(
+ String.format(
+ "account %s for email %s is inactive", accountState.account().id(), email));
+ }
+ }
+ }
+ return OptionalResultWithMessages.create(activeAccounts.build(), messages);
}
/**
@@ -388,9 +418,7 @@
if (!accountState.isPresent()) {
return OptionalResultWithMessages.createEmpty(
String.format(
- "cannot resolve code owner email %s: email belongs to account %s,"
- + " but no account with this ID exists",
- email, accountId));
+ "cannot resolve account %s for email %s: account does not exists", accountId, email));
}
return OptionalResultWithMessages.create(accountState.get());
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 92db861..8e57412 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -35,7 +35,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -61,7 +61,7 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final Provider<CurrentUser> userProvider;
- private final BatchUpdate.Factory batchUpdateFactory;
+ private final RetryHelper retryHelper;
private final ChangeNotes.Factory changeNotesFactory;
private final AccountCache accountCache;
private final ChangeMessagesUtil changeMessageUtil;
@@ -71,14 +71,14 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerApprovalCheck codeOwnerApprovalCheck,
Provider<CurrentUser> userProvider,
- BatchUpdate.Factory batchUpdateFactory,
+ RetryHelper retryHelper,
ChangeNotes.Factory changeNotesFactory,
AccountCache accountCache,
ChangeMessagesUtil changeMessageUtil) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
this.userProvider = userProvider;
- this.batchUpdateFactory = batchUpdateFactory;
+ this.retryHelper = retryHelper;
this.changeNotesFactory = changeNotesFactory;
this.accountCache = accountCache;
this.changeMessageUtil = changeMessageUtil;
@@ -95,11 +95,20 @@
return;
}
- try (BatchUpdate batchUpdate =
- batchUpdateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
- batchUpdate.addOp(changeId, new Op(event.getReviewers()));
- batchUpdate.execute();
- } catch (RestApiException | UpdateException e) {
+ try {
+ retryHelper
+ .changeUpdate(
+ "addCodeOwnersMessageOnAddReviewer",
+ updateFactory -> {
+ try (BatchUpdate batchUpdate =
+ updateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
+ batchUpdate.addOp(changeId, new Op(event.getReviewers()));
+ batchUpdate.execute();
+ }
+ return null;
+ })
+ .call();
+ } catch (Exception e) {
logger.atSevere().withCause(e).log(
String.format(
"Failed to post code-owners change message for reviewer on change %s in project %s.",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index dff2876..1cf59ce 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -199,16 +199,7 @@
newVote, user.getName()));
}
} else {
- logger.atSevere().log(
- "code owner approval was neither newly applied, removed, upgraded nor downgraded:"
- + " project = %s, change = %s, patch set = %d, oldApprvals = %s, approvals = %s,"
- + " requiredApproval = %s",
- changeNotes.getProjectName(),
- changeNotes.getChangeId(),
- patchSet.id().get(),
- oldApprovals,
- approvals,
- requiredApproval);
+ // non-approval was downgraded (e.g. -1 to -2)
return Optional.empty();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
index 52ca037..8a8c702 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
@@ -20,7 +20,6 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
@@ -44,8 +43,6 @@
*/
@Singleton
class OnCodeOwnerOverride implements OnPostReview {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
@Inject
@@ -82,8 +79,7 @@
continue;
}
- buildMessageForCodeOwnerOverride(
- user, changeNotes, patchSet, oldApprovals, approvals, overrideApproval)
+ buildMessageForCodeOwnerOverride(user, patchSet, oldApprovals, approvals, overrideApproval)
.ifPresent(messages::add);
}
@@ -95,7 +91,6 @@
private Optional<String> buildMessageForCodeOwnerOverride(
IdentifiedUser user,
- ChangeNotes changeNotes,
PatchSet patchSet,
Map<String, Short> oldApprovals,
Map<String, Short> approvals,
@@ -137,16 +132,7 @@
"By voting %s the code-owners submit requirement is still overridden by %s",
newVote, user.getName()));
}
- logger.atSevere().log(
- "code owner override was neither newly applied, removed, upgraded nor downgraded:"
- + " project = %s, change = %s, patch set = %d, oldApprvals = %s, approvals = %s,"
- + " requiredApproval = %s",
- changeNotes.getProjectName(),
- changeNotes.getChangeId(),
- patchSet.id().get(),
- oldApprovals,
- approvals,
- overrideApproval);
+ // non-approval was downgraded (e.g. -1 to -2)
return Optional.empty();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 74dae62..f230eb9 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -202,7 +202,7 @@
// The validation was executed as dry-run and failures during the validation should not
// cause an error. Hence we swallow the exception here.
- logger.atFine().withCause(e).log(
+ logger.atWarning().withCause(e).log(
"ignoring failure during validation of code owner config files in revision %s"
+ " (project = %s, branch = %s) because the validation was performed as dry-run",
receiveEvent.commit.getName(),
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index 709797a..8b56cd5 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -344,9 +344,11 @@
"found email %s as code owner in %s",
orphanedEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
- "cannot resolve code owner email %s: email belongs to account %s,"
- + " but no account with this ID exists",
- orphanedEmail, accountId));
+ "cannot resolve account %s for email %s: account does not exists",
+ accountId, orphanedEmail),
+ String.format(
+ "cannot resolve code owner email %s: no active account with this email found",
+ orphanedEmail));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index 79b3924..971f8e9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -1061,4 +1061,28 @@
+ "* %s\n",
user.fullName(), path));
}
+
+ @Test
+ public void changeMessageNotExtendedIfNonApprovalIsDowngraded() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ // Apply the Code-Review-1.
+ gApi.changes().id(changeId).current().review(ReviewInput.dislike());
+
+ // Change Code-Review-1 to Code-Review-2
+ gApi.changes().id(changeId).current().review(ReviewInput.reject());
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review-2");
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
index 5f17972..1265466 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
@@ -599,4 +599,45 @@
+ "By voting Owners-Override+1 the code-owners submit requirement is overridden by %s\n",
user.fullName()));
}
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void changeMessageNotExtendedIfNonApprovalIsDowngraded() throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values =
+ ImmutableMap.of("+1", "Override", " 0", "No Override", "-1", "Dislike", "-2", "Blocked");
+ gApi.projects().name(project.get()).label("Owners-Override").create(input);
+
+ // Allow to vote on the Owners-Override label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Owners-Override")
+ .range(-2, 1)
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", -1));
+
+ // Downgrade the vote from -1 to -2.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", -2));
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Owners-Override-2");
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 0297426..6a4ea64 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -121,6 +121,28 @@
}
@Test
+ public void resolveCodeOwnerReferenceForAmbiguousEmailIfOtherAccountIsInactive()
+ throws Exception {
+ // Create an external ID for 'user' account that has the same email as the 'admin' account.
+ accountsUpdate
+ .get()
+ .update(
+ "Test update",
+ user.id(),
+ (a, u) ->
+ u.addExternalId(
+ ExternalId.create(
+ "foo", "bar", user.id(), admin.email(), /* hashedPassword= */ null)));
+
+ // Deactivate the 'user' account.
+ accountOperations.account(user.id()).forUpdate().inactive().update();
+
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+ }
+
+ @Test
public void resolveCodeOwnerReferenceForAmbiguousEmail() throws Exception {
// Create an external ID for 'user' account that has the same email as the 'admin' account.
accountsUpdate
@@ -159,10 +181,13 @@
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
- .contains(
+ .containsAnyOf(
String.format(
- "cannot resolve code owner email %s: email belongs to account %s, but no account with this ID exists",
- email, accountId));
+ "cannot resolve account %s for email %s: account does not exists",
+ accountId, email),
+ String.format(
+ "cannost resolve code owner email %s: no active account with this email found",
+ email));
}
@Test
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index 3b0dc95..69929f4 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -102,7 +102,7 @@
setting)
* it is a non-visible secondary email
* there is no account that has this email assigned
- * it is ambiguous (the same email is assigned to multiple accounts)
+ * it is ambiguous (the same email is assigned to multiple active accounts)
* it has an email domain that is disallowed (see
[allowedEmailDomain](config.html#pluginCodeOwnersAllowedEmailDomain))
configuration
