Consider only emails that belong to multiple active accounts as ambiguous
Code owner emails that belong to multiple accounts are detected as
ambiguous and their code ownership is ignored. This is intended however
if an email belongs to multiple accounts and only of them is active,
that's fine as there is exactly one active account to which the code
ownership is assigned.
For CodeOwnerResolver this means that it has to load the accounts, and
check if they are active, before deciding if an email is ambiguous.
Doing this changes, actually improves, the debug logs in case there is
an orphaned email.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I50f06e4e055ee04ac17c8873f0e08c0d6aca4b5e
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/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/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
