Fix resolving exempted users to accounts in background threads
The code-owners submit rule can be executed in background threads where
a current user is not available (e.g. when a change is indexed).
AccountResolver needs the current user to check the account visibility
and fails if it is not available. Hence we must not use AccountResolver
to resolve exempted users.
Using AccountResolver for this was also a mistake due to other reasons:
* it checks account visibility, which we don't want to do
* it may access the account index, which we also don't want to do
* it searched over all account fields, but we are only interested in
emails (the code handled this case, but it adds unnecessary
complexity)
Fix all of this by using the Emails class, instead of AccountResolver,
to resolve exempted users.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I238d343a7c9b1a64b789f4f671707bb0227e52b8
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
index 783019b..37b286b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -34,10 +35,7 @@
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
-import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
-import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
@@ -48,7 +46,6 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
/** Snapshot of the code-owners plugin configuration for one project. */
@@ -62,7 +59,7 @@
private final String pluginName;
private final PluginConfigFactory pluginConfigFactory;
private final ProjectCache projectCache;
- private final AccountResolver accountResolver;
+ private final Emails emails;
private final BackendConfig backendConfig;
private final GeneralConfig generalConfig;
private final OverrideApprovalConfig overrideApprovalConfig;
@@ -78,7 +75,7 @@
@PluginName String pluginName,
PluginConfigFactory pluginConfigFactory,
ProjectCache projectCache,
- AccountResolver accountResolver,
+ Emails emails,
BackendConfig backendConfig,
GeneralConfig generalConfig,
OverrideApprovalConfig overrideApprovalConfig,
@@ -88,7 +85,7 @@
this.pluginName = pluginName;
this.pluginConfigFactory = pluginConfigFactory;
this.projectCache = projectCache;
- this.accountResolver = accountResolver;
+ this.emails = emails;
this.backendConfig = backendConfig;
this.generalConfig = generalConfig;
this.overrideApprovalConfig = overrideApprovalConfig;
@@ -225,36 +222,27 @@
}
private ImmutableSet<Account.Id> lookupExemptedAccounts() {
- ImmutableSet.Builder<Account.Id> exemptedAccounts = ImmutableSet.builder();
- for (String exemptedUser : generalConfig.getExemptedUsers(pluginConfig)) {
- try {
- AccountState accountState = accountResolver.resolve(exemptedUser).asUnique();
+ ImmutableSet<String> exemptedUsers = generalConfig.getExemptedUsers(pluginConfig);
- // We only support specifying exempted users by email, if another account identifier (full
- // name, account ID, etc.) was used in the config we want to ignore it. Hence after looking
- // up the account we check that the identifier from the config was indeed an email of the
- // account.
- if (!ExternalId.getEmails(accountState.externalIds())
- .anyMatch(email -> email.equals(exemptedUser))) {
- logger.atWarning().log(
- "Ignoring exempted user %s for project %s: not an email", exemptedUser, projectName);
- continue;
- }
+ try {
+ ImmutableSetMultimap<String, Account.Id> exemptedAccounts =
+ emails.getAccountsFor(exemptedUsers.toArray(new String[0]));
- exemptedAccounts.add(accountState.account().id());
- } catch (UnresolvableAccountException e) {
- logger.atWarning().log(
- "Ignoring exempted user %s for project %s: %s",
- exemptedUser, projectName, e.getMessage());
- } catch (IOException | ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
- String.format(
- "Failed to resolve exempted user %s on project %s", exemptedUser, projectName),
- e);
- }
+ exemptedUsers.stream()
+ .filter(exemptedUser -> !exemptedAccounts.containsKey(exemptedUser))
+ .forEach(
+ exemptedUser ->
+ logger.atWarning().log(
+ "Ignoring exempted user %s for project %s: not found",
+ exemptedUser, projectName));
+
+ return ImmutableSet.copyOf(exemptedAccounts.values());
+ } catch (IOException e) {
+ throw new CodeOwnersInternalServerErrorException(
+ String.format(
+ "Failed to resolve exempted users %s on project %s", exemptedUsers, projectName),
+ e);
}
-
- return exemptedAccounts.build();
}
/** Gets the override info URL that is configured. */
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index ecb1a9e..ef9c2b6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -20,6 +20,8 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -297,4 +299,43 @@
.hasMessageThat()
.isEqualTo(String.format("destination branch \"refs/heads/%s\" not found.", branchName));
}
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.exemptedUser", value = "exempted-user@example.com")
+ public void changeIsSubmittableIfUserIsExcempted() throws Exception {
+ TestAccount exemptedUser =
+ accountCreator.create(
+ "exemptedUser", "exempted-user@example.com", "Exempted User", /* displayName= */ null);
+
+ PushOneCommit.Result r = createChange(exemptedUser, "Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ // Apply Code-Review+2 by a non-code-owner to satisfy the MaxWithBlock function of the
+ // Code-Review label.
+ approve(changeId);
+
+ ChangeInfo changeInfo =
+ gApi.changes()
+ .id(changeId)
+ .get(
+ ListChangesOption.SUBMITTABLE,
+ ListChangesOption.ALL_REVISIONS,
+ ListChangesOption.CURRENT_ACTIONS);
+ assertThat(changeInfo.submittable).isTrue();
+
+ // Check that the submit button is enabled.
+ assertThat(changeInfo.revisions.get(r.getCommit().getName()).actions.get("submit").enabled)
+ .isTrue();
+
+ // Check the submit requirement.
+ SubmitRequirementInfoSubject submitRequirementInfoSubject =
+ assertThatCollection(changeInfo.requirements).onlyElement();
+ submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
+
+ // Submit the change.
+ gApi.changes().id(changeId).current().submit();
+ assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
}
