ChangeQueryBuilder: Throw error on ambiguous visibleto by display name

If the given account identifier resolved to more than one user, the
visibleto would only use the first user from the result. This was OK
for queries by username or account ID because they always resolve to
either no user or exactly one user. However when querying by display
name it is possible that multiple users will be returned, and in this
case it was incorrect to only use the first one.

Fix it to instead throw an error when multiple accounts are resolved,
and extend the tests to cover this use case.

This has been broken since change I87e58dda4 which was introduced in
2012 and was discovered by Sonar Lint which reported the following
message on the broken code:

  Loops with at most one iteration should be refactored (squid:S1751)

  A loop with at most one iteration is equivalent to the use of an if
  statement to conditionally execute one piece of code. No developer
  expects to find such a use of a loop statement. If the initial
  intention of the author was really to conditionally execute one piece
  of code, an if statement should be used instead.

  At worst that was not the initial intention of the author and so the
  body of the loop should be fixed to use the nested return, break or
  throw statements in a more appropriate way.

Change-Id: Ibb16a70e6cc61c22516f5d9ac3e1b6b461c94dc2

2 files changed, 35 insertions, 9 deletions

diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 10b3bc8a2e..61726b8479 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -22,6 +22,7 @@ import static java.util.stream.Collectors.toSet;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Enums;
 import com.google.common.base.Splitter;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.data.GroupDescription;
@@ -913,12 +914,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       return isVisible();
     }
     Set<Account.Id> m = args.accountResolver.findAll(who);
-    if (!m.isEmpty()) {
-      List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(m.size());
-      for (Account.Id id : m) {
-        return visibleto(args.userFactory.create(id));
-      }
-      return Predicate.or(p);
+    if (m.size() == 1) {
+      return visibleto(args.userFactory.create(Iterables.getOnlyElement(m)));
+    } else if (m.size() > 1) {
+      throw error(String.format("\"%s\" resolves to multiple accounts", who));
     }
 
     // If its not an account, maybe its a group?
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 0ebbdc9fe0..bd2b3596d5 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1710,16 +1710,43 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery(q + " visibleto:self", change2, change1);
 
     // Second user cannot see first user's private change
-    Account.Id user2 = createAccount("anotheruser");
+    Account.Id user2 = createAccount("user2");
     assertQuery(q + " visibleto:" + user2.get(), change1);
-    assertQuery(q + " visibleto:anotheruser", change1);
+    assertQuery(q + " visibleto:user2", change1);
 
     String g1 = createGroup("group1", "Administrators");
-    gApi.groups().id(g1).addMembers("anotheruser");
+    gApi.groups().id(g1).addMembers("user2");
     assertQuery(q + " visibleto:" + g1, change1);
 
     requestContext.setContext(newRequestContext(user2));
     assertQuery("is:visible", change1);
+
+    Account.Id user3 = createAccount("user3");
+
+    // Explicitly authenticate user2 and user3 so that display name gets set
+    AuthRequest authRequest = AuthRequest.forUser("user2");
+    authRequest.setDisplayName("Another User");
+    authRequest.setEmailAddress("user2@example.com");
+    accountManager.authenticate(authRequest);
+    authRequest = AuthRequest.forUser("user3");
+    authRequest.setDisplayName("Another User");
+    authRequest.setEmailAddress("user3@example.com");
+    accountManager.authenticate(authRequest);
+
+    // Switch to user3
+    requestContext.setContext(newRequestContext(user3));
+    Change change3 = insert(repo, newChange(repo), user3);
+    Change change4 = insert(repo, newChangePrivate(repo), user3);
+
+    // User3 can see both their changes and the first user's change
+    assertQuery(q + " visibleto:" + user3.get(), change4, change3, change1);
+
+    // User2 cannot see user3's private change
+    assertQuery(q + " visibleto:" + user2.get(), change3, change1);
+
+    // Query as user3 by display name matching user2 and user3; bad request
+    assertFailingQuery(
+        q + " visibleto:\"Another User\"", "\"Another User\" resolves to multiple accounts");
   }
 
   @Test