From 7deacf1f1f3db7d4609de28fdabd3fb4628c57a1 Mon Sep 17 00:00:00 2001 From: Maxime Guerreiro Date: Wed, 27 Jun 2018 13:13:19 +0000 Subject: [PATCH] Add a method to query accounts only by their external ID's email And use this method from the Prolog predicates, in order to remove the dependency on the index when indexing rules. This fixes the two remaining tests in RulesIT. The newly introduced method doesn't query the index to find accounts by their preferred email. Instead, the external IDs cache is assumed to be consistent with reality. This method is used in PatchSetInfoFactory and EventFactory to allow usage of these two classes from an environment with no index access. The underlying issue (accounts with no external IDs) is already fixed, and this change is a first step towards removing the patches required by it. Change-Id: I93a91c11f6473dfe9e4d9b56c27a351234b50e56 --- java/com/google/gerrit/server/account/Emails.java | 10 ++++++++++ java/gerrit/AbstractCommitUserIdentityPredicate.java | 5 ++--- .../google/gerrit/acceptance/server/rules/RulesIT.java | 8 ++------ 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java index 8a481677fe..e91ce492f3 100644 --- a/java/com/google/gerrit/server/account/Emails.java +++ b/java/com/google/gerrit/server/account/Emails.java @@ -95,6 +95,16 @@ public class Emails { return builder.build(); } + /** + * Returns the accounts with the given email. + * + *

This method behaves just like {@link #getAccountFor(String)}, except that accounts are not + * looked up by their preferred email. Thus, this method does not rely on the accounts index. + */ + public ImmutableSet getAccountForExternal(String email) throws IOException { + return externalIds.byEmail(email).stream().map(ExternalId::accountId).collect(toImmutableSet()); + } + private T executeIndexQuery(Action action) throws OrmException { try { return retryHelper.execute(ActionType.INDEX_QUERY, action, OrmException.class::isInstance); diff --git a/java/gerrit/AbstractCommitUserIdentityPredicate.java b/java/gerrit/AbstractCommitUserIdentityPredicate.java index f8ef065c1b..1bfc95c46c 100644 --- a/java/gerrit/AbstractCommitUserIdentityPredicate.java +++ b/java/gerrit/AbstractCommitUserIdentityPredicate.java @@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.Emails; import com.google.gerrit.server.rules.PrologEnvironment; -import com.google.gwtorm.server.OrmException; import com.googlecode.prolog_cafe.exceptions.PrologException; import com.googlecode.prolog_cafe.exceptions.SystemException; import com.googlecode.prolog_cafe.lang.IntegerTerm; @@ -56,11 +55,11 @@ abstract class AbstractCommitUserIdentityPredicate extends Predicate.P3 { Emails emails = env.getArgs().getEmails(); Account.Id id = null; try { - ImmutableSet ids = emails.getAccountFor(userId.getEmailAddress()); + ImmutableSet ids = emails.getAccountForExternal(userId.getEmailAddress()); if (ids.size() == 1) { id = ids.iterator().next(); } - } catch (IOException | OrmException e) { + } catch (IOException e) { throw new SystemException(e.getMessage()); } diff --git a/javatests/com/google/gerrit/acceptance/server/rules/RulesIT.java b/javatests/com/google/gerrit/acceptance/server/rules/RulesIT.java index bf01a215aa..98dca3ef31 100644 --- a/javatests/com/google/gerrit/acceptance/server/rules/RulesIT.java +++ b/javatests/com/google/gerrit/acceptance/server/rules/RulesIT.java @@ -71,21 +71,17 @@ public class RulesIT extends AbstractDaemonTest { @Test public void testUserPredicate() throws Exception { - // This test results in a RULE_ERROR as Prolog tries to find accounts by email, using the index. - // TODO(maximeg) get OK results modifySubmitRules( String.format( "gerrit:commit_author(user(%d), '%s', '%s')", user.getId().get(), user.fullName, user.email)); - assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.RULE_ERROR); + assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.OK); } @Test public void testCommitAuthorPredicate() throws Exception { - // This test results in a RULE_ERROR as Prolog tries to find accounts by email, using the index. - // TODO(maximeg) get OK results modifySubmitRules("gerrit:commit_author(Id)"); - assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.RULE_ERROR); + assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.OK); } private SubmitRecord.Status statusForRule() throws Exception {