Fix issue with querying inactive user changes

Introduce the custom account activity predicate to allow customise the
way of resolving accounts during the search.
Current implementation filters out inactive users during the change data
query but changes from inactive user should be visible.

Bug: Issue 11367
Change-Id: Ib8cfcf3e7c4f21a0207f801c111c5359716bda9c

java/com/google/gerrit/server/account/AccountResolver.java

@@ -516,7 +516,12 @@ public class AccountResolver {
    * @throws IOException if an error occurs.
    */
   public Result resolve(String input) throws ConfigInvalidException, IOException {
-    return searchImpl(input, searchers, visibilitySupplier());
+    return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate());
+  }
+
+  public Result resolve(String input, Predicate<AccountState> accountActivityPredicate)
+      throws ConfigInvalidException, IOException {
+    return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate);
   }
 
   /**
@@ -544,18 +549,24 @@ public class AccountResolver {
    */
   @Deprecated
   public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException {
-    return searchImpl(input, nameOrEmailSearchers, visibilitySupplier());
+    return searchImpl(
+        input, nameOrEmailSearchers, visibilitySupplier(), accountActivityPredicate());
   }
 
   private Supplier<Predicate<AccountState>> visibilitySupplier() {
     return () -> accountControlFactory.get()::canSee;
   }
 
+  private Predicate<AccountState> accountActivityPredicate() {
+    return (AccountState accountState) -> accountState.getAccount().isActive();
+  }
+
   @VisibleForTesting
   Result searchImpl(
       String input,
       ImmutableList<Searcher<?>> searchers,
-      Supplier<Predicate<AccountState>> visibilitySupplier)
+      Supplier<Predicate<AccountState>> visibilitySupplier,
+      Predicate<AccountState> accountActivityPredicate)
       throws ConfigInvalidException, IOException {
     visibilitySupplier = Suppliers.memoize(visibilitySupplier::get);
     List<AccountState> inactive = new ArrayList<>();
@@ -576,7 +587,7 @@ public class AccountResolver {
         // Keep track of all inactive candidates discovered by any searchers. If we end up short-
         // circuiting, the inactive list will be discarded.
         List<AccountState> active = new ArrayList<>();
-        results.forEach(a -> (a.getAccount().isActive() ? active : inactive).add(a));
+        results.forEach(a -> (accountActivityPredicate.test(a) ? active : inactive).add(a));
         list = active;
       } else {
         list = results.collect(toImmutableList());

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -54,6 +54,7 @@ import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.account.AccountCache;
 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.GroupBackend;
 import com.google.gerrit.server.account.GroupBackends;
 import com.google.gerrit.server.account.GroupMembers;
@@ -1002,7 +1003,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   @Operator
   public Predicate<ChangeData> owner(String who)
       throws QueryParseException, IOException, ConfigInvalidException {
-    return owner(parseAccount(who));
+    return owner(parseAccount(who, (AccountState s) -> true));
   }
 
   private Predicate<ChangeData> owner(Set<Account.Id> who) {
@@ -1025,7 +1026,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   @Operator
   public Predicate<ChangeData> assignee(String who)
       throws QueryParseException, IOException, ConfigInvalidException {
-    return assignee(parseAccount(who));
+    return assignee(parseAccount(who, (AccountState s) -> true));
   }
 
   private Predicate<ChangeData> assignee(Set<Account.Id> who) {
@@ -1351,6 +1352,19 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     }
   }
 
+  private Set<Account.Id> parseAccount(
+      String who, java.util.function.Predicate<AccountState> activityFilter)
+      throws QueryParseException, IOException, ConfigInvalidException {
+    try {
+      return args.accountResolver.resolve(who, activityFilter).asNonEmptyIdSet();
+    } catch (UnresolvableAccountException e) {
+      if (e.isSelf()) {
+        throw new QueryRequiresAuthException(e.getMessage(), e);
+      }
+      throw new QueryParseException(e.getMessage(), e);
+    }
+  }
+
   private GroupReference parseGroup(String group) throws QueryParseException {
     GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
     if (g == null) {

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -558,6 +558,24 @@ public class AccountIT extends AbstractDaemonTest {
     accountIndexedCounter.assertReindexOf(user);
   }
 
+  @Test
+  public void shouldAllowQueryByEmailForInactiveUser() throws Exception {
+    Account.Id activatableAccountId =
+        accountOperations.newAccount().inactive().preferredEmail("foo@activatable.com").create();
+    accountIndexedCounter.assertReindexOf(activatableAccountId, 1);
+
+    gApi.changes().query("owner:foo@activatable.com").get();
+  }
+
+  @Test
+  public void shouldAllowQueryByUserNameForInactiveUser() throws Exception {
+    Account.Id activatableAccountId =
+        accountOperations.newAccount().inactive().username("foo").create();
+    accountIndexedCounter.assertReindexOf(activatableAccountId, 1);
+
+    gApi.changes().query("owner:foo").get();
+  }
+
   @Test
   public void validateAccountActivation() throws Exception {
     Account.Id activatableAccountId =

javatests/com/google/gerrit/server/account/AccountResolverTest.java

@@ -162,6 +162,23 @@ public class AccountResolverTest extends GerritBaseTests {
     assertThat(filteredInactiveIds(result)).isEmpty();
   }
 
+  @Test
+  public void shouldUseCustomAccountActivityPredicate() throws Exception {
+    TestSearcher searcher1 = new TestSearcher("foo", false, newInactiveAccount(1));
+    searcher1.setCallerShouldFilterOutInactiveCandidates();
+    TestSearcher searcher2 = new TestSearcher("f.*", false, newInactiveAccount(2));
+    searcher2.setCallerShouldFilterOutInactiveCandidates();
+    ImmutableList<Searcher<?>> searchers = ImmutableList.of(searcher1, searcher2);
+
+    Result result = search("foo", searchers, allVisible(), (a) -> true);
+    // Searchers always short-circuit when finding a non-empty result list,
+    // and this one didn't filter out inactive results,
+    // so the second searcher never ran.
+    assertThat(result.asIdSet()).containsExactlyElementsIn(ids(1));
+    assertThat(getOnlyElement(result.asList()).getAccount().isActive()).isFalse();
+    assertThat(filteredInactiveIds(result)).isEmpty();
+  }
+
   @Test
   public void filterInactiveEventuallyFindingResults() throws Exception {
     TestSearcher searcher1 = new TestSearcher("foo", false, newInactiveAccount(1));
@@ -324,7 +341,16 @@ public class AccountResolverTest extends GerritBaseTests {
       ImmutableList<Searcher<?>> searchers,
       Supplier<Predicate<AccountState>> visibilitySupplier)
       throws Exception {
-    return newAccountResolver().searchImpl(input, searchers, visibilitySupplier);
+    return search(input, searchers, visibilitySupplier, activityPrediate());
+  }
+
+  private Result search(
+      String input,
+      ImmutableList<Searcher<?>> searchers,
+      Supplier<Predicate<AccountState>> visibilitySupplier,
+      Predicate<AccountState> activityPredicate)
+      throws Exception {
+    return newAccountResolver().searchImpl(input, searchers, visibilitySupplier, activityPredicate);
   }
 
   private static AccountResolver newAccountResolver() {
@@ -350,6 +376,10 @@ public class AccountResolverTest extends GerritBaseTests {
     return () -> a -> true;
   }
 
+  private Predicate<AccountState> activityPrediate() {
+    return (AccountState accountState) -> accountState.getAccount().isActive();
+  }
+
   private static Supplier<Predicate<AccountState>> only(int... ids) {
     ImmutableSet<Account.Id> idSet =
         Arrays.stream(ids).mapToObj(Account.Id::new).collect(toImmutableSet());