Handle missing accounts explicitly and skip accounts without preferred email
The account cache offers 2 methods to lookup accounts, which are both
being renamed in Gerrit core:
1. 'get' (new name 'getEvenIfMissing'):
Always returns an AccountState, even if the requested account is
missing. If the account is missing the returned AccountState is
empty, e.g. it doesn't have a preferred email (preferred email ==
null).
2. 'maybeGet' (new name 'get'):
Returns an Optional<AccountState>. If the requested account is
missing, an empty Optional is returned.
So far the find-owners plugin was using method 1. This change migrates
it to using method 2 so that the handling of missing accounts is done
explicitly now. Using method 2 is better because it makes it obvious
that accounts can be missing, e.g. it seems that so far the find-owners
plugin was not aware that method 1 can return an empty AccountState and
hence the preferred email can be null. The preferred email can also be
null for existing accounts. It's a purely optional field in the account
data. It may be null even if the account is linked to emails via
external IDs. So far if the preferred email is null the find-owners
plugin included a null value into the result (e.g. list of reviewers, map of
preferred emails to votes). These null values were not useful and are
skipped now. This is the only change in behaviour.
Change-Id: Ib67642f1d865d0e5217d48d26e7e91087f8174c6
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
index 1971491..361cecc 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -14,13 +14,15 @@
package com.googlesource.gerrit.plugins.findowners;
+import static java.util.stream.Collectors.toList;
+
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Streams;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.webui.UiAction;
-import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -46,6 +48,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -141,17 +144,21 @@
/** Returns reviewer emails got from ChangeData. */
static List<String> getReviewers(ChangeData changeData, AccountCache accountCache) {
- List<String> result = new ArrayList<>();
try {
- for (Account.Id id : changeData.reviewers().all()) {
- Account account = accountCache.get(id).getAccount();
- result.add(account.getPreferredEmail());
- }
+ // Reviewers may have no preferred email, skip them if the preferred email is not set.
+ return changeData
+ .reviewers()
+ .all()
+ .stream()
+ .map(accountCache::get)
+ .flatMap(Streams::stream)
+ .map(a -> a.getAccount().getPreferredEmail())
+ .filter(Objects::nonNull)
+ .collect(toList());
} catch (OrmException e) {
log.error("Exception for " + Config.getChangeId(changeData), e);
- result = new ArrayList<>();
+ return new ArrayList<>();
}
- return result;
}
/** Returns the current patchset number or the given patchsetNum if it is valid. */
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index 85f4bec..54861a1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -26,6 +26,7 @@
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -55,16 +56,24 @@
Map<String, Integer> map = new HashMap<>();
for (PatchSetApproval p : changeData.currentApprovals()) {
if (p.getValue() != 0) {
- map.put(
- accountCache.get(p.getAccountId()).getAccount().getPreferredEmail(),
- Integer.valueOf(p.getValue()));
+ // Reviewers may have no preferred email, skip them if the preferred email is not set.
+ Optional<String> preferredEmail =
+ accountCache.get(p.getAccountId()).map(a -> a.getAccount().getPreferredEmail());
+ if (preferredEmail.isPresent()) {
+ map.put(preferredEmail.get(), Integer.valueOf(p.getValue()));
+ }
}
}
// Give CL author a default minVoteLevel vote.
- String author =
- accountCache.get(changeData.change().getOwner()).getAccount().getPreferredEmail();
- if (!map.containsKey(author) || map.get(author) == 0) {
- map.put(author, minVoteLevel);
+ // The preferred email of the author may not be set. Pushing changes only requires an email in
+ // the external IDs, but the preferred email may still be null (also emails may have been
+ // deleted after creating the change). Skip the author if it doesn't have a preferred email.
+ Optional<String> author =
+ accountCache
+ .get(changeData.change().getOwner())
+ .map(a -> a.getAccount().getPreferredEmail());
+ if (author.isPresent() && (!map.containsKey(author.get()) || map.get(author.get()) == 0)) {
+ map.put(author.get(), minVoteLevel);
}
return map;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index 2c0c704..bd84c13 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -152,7 +152,12 @@
if (ids == null || ids.size() != 1) {
errors.add(owner);
} else {
- email = accountCache.get(ids.iterator().next()).getAccount().getPreferredEmail();
+ // Accounts may have no preferred email.
+ email =
+ accountCache
+ .get(ids.iterator().next())
+ .map(a -> a.getAccount().getPreferredEmail())
+ .orElse(null);
}
}
} catch (Exception e) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 9ad5140..323a861 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.plugins.findowners;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -32,11 +33,13 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.inject.Inject;
import java.util.Collection;
+import java.util.Optional;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevObject;
@@ -208,7 +211,9 @@
assertThat(id1).isEqualTo(id3);
assertThat(id1).isEqualTo(id4);
// Action.getReviewers and Checker.getVotes use accountCache to get email address.
- assertThat(accountCache.get(id1).getAccount().getPreferredEmail()).isEqualTo(emails1[i]);
+ Optional<Account> account = accountCache.get(id1).map(AccountState::getAccount);
+ assertThat(account).named("account %s", id1).isPresent();
+ assertThat(account.get().getPreferredEmail()).isEqualTo(emails1[i]);
}
// Wrong or non-existing email address.
String[] wrongEmails = {"nobody", "@g.com", "nobody@g.com", "*"};
