Fix owner: search operator substring regression
In prior releases the owner: and reviewer: operators performed
substring searching to locate user accounts that match the argument,
and find all changes relevant to those accounts. Fix the operators
to perform that multiple-account searching again.
Bug: issue 646
Change-Id: Ie0d67ad15ddffa7d2d774466c42f84dc1a760f25
Signed-off-by: Shawn O. Pearce <sop@google.com>
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
index e738c6f..3dce94e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
@@ -15,12 +15,14 @@
package com.google.gerrit.server.account;
import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.AccountExternalId;
import com.google.gerrit.reviewdb.ReviewDb;
import com.google.gwtorm.client.OrmException;
-import com.google.gwtorm.client.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.regex.Matcher;
@@ -52,28 +54,37 @@
* there are multiple candidates.
*/
public Account find(final String nameOrEmail) throws OrmException {
+ Set<Account.Id> r = findAll(nameOrEmail);
+ return r.size() == 1 ? byId.get(r.iterator().next()).getAccount() : null;
+ }
+
+ /**
+ * Locate exactly one account matching the name or name/email string.
+ *
+ * @param nameOrEmail a string of the format
+ * "Full Name <email@example>", just the email address
+ * ("email@example"), a full name ("Full Name"), an account id
+ * ("18419") or an user name ("username").
+ * @return the accounts that match, empty collection if none. Never null.
+ */
+ public Set<Account.Id> findAll(String nameOrEmail) throws OrmException {
Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail);
if (m.matches()) {
- return byId.get(Account.Id.parse(m.group(1))).getAccount();
+ return Collections.singleton(Account.Id.parse(m.group(1)));
}
if (nameOrEmail.matches("^[1-9][0-9]*$")) {
- return byId.get(Account.Id.parse(nameOrEmail)).getAccount();
+ return Collections.singleton(Account.Id.parse(nameOrEmail));
}
if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) {
- Account who = findByUserName(nameOrEmail);
+ AccountState who = byId.getByUsername(nameOrEmail);
if (who != null) {
- return who;
+ return Collections.singleton(who.getAccount().getId());
}
}
- Account account = findByNameOrEmail(nameOrEmail);
- if (account != null) {
- return account;
- }
-
- return null;
+ return findAllByNameOrEmail(nameOrEmail);
}
/**
@@ -87,39 +98,61 @@
*/
public Account findByNameOrEmail(final String nameOrEmail)
throws OrmException {
+ Set<Account.Id> r = findAllByNameOrEmail(nameOrEmail);
+ return r.size() == 1 ? byId.get(r.iterator().next()).getAccount() : null;
+ }
+
+ /**
+ * Locate exactly one account matching the name or name/email string.
+ *
+ * @param nameOrEmail a string of the format
+ * "Full Name <email@example>", just the email address
+ * ("email@example"), a full name ("Full Name").
+ * @return the accounts that match, empty collection if none. Never null.
+ */
+ public Set<Account.Id> findAllByNameOrEmail(final String nameOrEmail)
+ throws OrmException {
final int lt = nameOrEmail.indexOf('<');
final int gt = nameOrEmail.indexOf('>');
if (lt >= 0 && gt > lt && nameOrEmail.contains("@")) {
- return findByEmail(nameOrEmail.substring(lt + 1, gt));
+ return byEmail.get(nameOrEmail.substring(lt + 1, gt));
}
if (nameOrEmail.contains("@")) {
- return findByEmail(nameOrEmail);
+ return byEmail.get(nameOrEmail);
}
final Account.Id id = realm.lookup(nameOrEmail);
if (id != null) {
- return byId.get(id).getAccount();
+ return Collections.singleton(id);
}
- return oneAccount(schema.get().accounts().byFullName(nameOrEmail));
- }
-
- private Account findByEmail(final String email) {
- final Set<Account.Id> candidates = byEmail.get(email);
- if (1 == candidates.size()) {
- return byId.get(candidates.iterator().next()).getAccount();
+ List<Account> m = schema.get().accounts().byFullName(nameOrEmail).toList();
+ if (m.size() == 1) {
+ return Collections.singleton(m.get(0).getId());
}
- return null;
- }
- private static Account oneAccount(final ResultSet<Account> rs) {
- final List<Account> r = rs.toList();
- return r.size() == 1 ? r.get(0) : null;
- }
-
- private Account findByUserName(final String userName) {
- AccountState as = byId.getByUsername(userName);
- return as != null ? as.getAccount() : null;
+ // At this point we have no clue. Just perform a whole bunch of suggestions
+ // and pray we come up with a reasonable result list.
+ //
+ Set<Account.Id> result = new HashSet<Account.Id>();
+ String a = nameOrEmail;
+ String b = nameOrEmail + "\u9fa5";
+ for (Account act : schema.get().accounts().suggestByFullName(a, b, 10)) {
+ result.add(act.getId());
+ }
+ for (AccountExternalId extId : schema
+ .get()
+ .accountExternalIds()
+ .suggestByKey(
+ new AccountExternalId.Key(AccountExternalId.SCHEME_USERNAME, a),
+ new AccountExternalId.Key(AccountExternalId.SCHEME_USERNAME, b), 10)) {
+ result.add(extId.getAccountId());
+ }
+ for (AccountExternalId extId : schema.get().accountExternalIds()
+ .suggestByEmailAddress(a, b, 10)) {
+ result.add(extId.getAccountId());
+ }
+ return result;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/RewritePredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/RewritePredicate.java
index c0a00ca..35fbc1c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/RewritePredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/RewritePredicate.java
@@ -20,10 +20,12 @@
import java.util.List;
public abstract class RewritePredicate<T> extends Predicate<T> {
- private String name = getClass().getName();
+ private boolean init;
+ private String name = getClass().getSimpleName();
private List<Predicate<T>> children = Collections.emptyList();
- void init(String name, Predicate<T>[] args) {
+ protected void init(String name, Predicate<T>... args) {
+ this.init = true;
this.name = name;
this.children = Arrays.asList(args);
}
@@ -33,10 +35,18 @@
return this;
}
+ @SuppressWarnings("unchecked")
@Override
public boolean equals(Object other) {
- return getClass() == other.getClass()
- && children.equals(((RewritePredicate) other).children);
+ if (other instanceof RewritePredicate) {
+ RewritePredicate that = (RewritePredicate<T>) other;
+ if (this.init && that.init) {
+ return this.getClass() == that.getClass()
+ && this.name.equals(that.name)
+ && this.children.equals(that.children);
+ }
+ }
+ return this == other;
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 828a126..eccb2e8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -40,8 +40,11 @@
import org.eclipse.jgit.lib.AbbreviatedObjectId;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
import java.util.regex.Pattern;
/**
@@ -346,21 +349,37 @@
@Operator
public Predicate<ChangeData> owner(String who) throws QueryParseException,
OrmException {
- Account account = args.accountResolver.find(who);
- if (account == null) {
+ Set<Account.Id> m = args.accountResolver.findAll(who);
+ if (m.isEmpty()) {
throw error("User " + who + " not found");
+ } else if (m.size() == 1) {
+ Account.Id id = m.iterator().next();
+ return new OwnerPredicate(args.dbProvider, id);
+ } else {
+ List<OwnerPredicate> p = new ArrayList<OwnerPredicate>(m.size());
+ for (Account.Id id : m) {
+ p.add(new OwnerPredicate(args.dbProvider, id));
+ }
+ return Predicate.or(p);
}
- return new OwnerPredicate(args.dbProvider, account.getId());
}
@Operator
- public Predicate<ChangeData> reviewer(String nameOrEmail)
+ public Predicate<ChangeData> reviewer(String who)
throws QueryParseException, OrmException {
- Account account = args.accountResolver.find(nameOrEmail);
- if (account == null) {
- throw error("Reviewer " + nameOrEmail + " not found");
+ Set<Account.Id> m = args.accountResolver.findAll(who);
+ if (m.isEmpty()) {
+ throw error("User " + who + " not found");
+ } else if (m.size() == 1) {
+ Account.Id id = m.iterator().next();
+ return new ReviewerPredicate(args.dbProvider, id);
+ } else {
+ List<ReviewerPredicate> p = new ArrayList<ReviewerPredicate>(m.size());
+ for (Account.Id id : m) {
+ p.add(new ReviewerPredicate(args.dbProvider, id));
+ }
+ return Predicate.or(p);
}
- return new ReviewerPredicate(args.dbProvider, account.getId());
}
@Operator
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
index 98b12f7..904bc3c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
@@ -38,8 +38,7 @@
new ChangeQueryBuilder.Arguments( //
new InvalidProvider<ReviewDb>(), //
new InvalidProvider<ChangeQueryRewriter>(), //
- null, null, null, null, null, null, null, null, null),
- null));
+ null, null, null, null, null, null, null, null, null), null));
private final Provider<ReviewDb> dbProvider;
@@ -287,11 +286,16 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:merged S=(sortkey_after:*) L=(limit:*)")
public Predicate<ChangeData> r20_byMergedPrev(
@Named("S") final SortKeyPredicate.After s,
@Named("L") final IntPredicate<ChangeData> l) {
return new PaginatedSource(50000, s.getValue(), l.intValue()) {
+ {
+ init("r20_byMergedPrev", s, l);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a, String key, int limit)
throws OrmException {
@@ -306,11 +310,16 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:merged S=(sortkey_before:*) L=(limit:*)")
public Predicate<ChangeData> r20_byMergedNext(
@Named("S") final SortKeyPredicate.Before s,
@Named("L") final IntPredicate<ChangeData> l) {
return new PaginatedSource(50000, s.getValue(), l.intValue()) {
+ {
+ init("r20_byMergedNext", s, l);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a, String key, int limit)
throws OrmException {
@@ -325,11 +334,16 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:abandoned S=(sortkey_after:*) L=(limit:*)")
public Predicate<ChangeData> r20_byAbandonedPrev(
@Named("S") final SortKeyPredicate.After s,
@Named("L") final IntPredicate<ChangeData> l) {
return new PaginatedSource(50000, s.getValue(), l.intValue()) {
+ {
+ init("r20_byAbandonedPrev", s, l);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a, String key, int limit)
throws OrmException {
@@ -344,11 +358,16 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:abandoned S=(sortkey_before:*) L=(limit:*)")
public Predicate<ChangeData> r20_byAbandonedNext(
@Named("S") final SortKeyPredicate.Before s,
@Named("L") final IntPredicate<ChangeData> l) {
return new PaginatedSource(50000, s.getValue(), l.intValue()) {
+ {
+ init("r20_byAbandonedNext", s, l);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a, String key, int limit)
throws OrmException {
@@ -379,10 +398,15 @@
return or(r20_byMergedNext(s, l), r20_byAbandonedNext(s, l));
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:open O=(owner:*)")
public Predicate<ChangeData> r25_byOwnerOpen(
@Named("O") final OwnerPredicate o) {
return new ChangeSource(50) {
+ {
+ init("r25_byOwnerOpen", o);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a) throws OrmException {
return a.byOwnerOpen(o.getAccountId());
@@ -395,10 +419,15 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:closed O=(owner:*)")
public Predicate<ChangeData> r25_byOwnerClosed(
@Named("O") final OwnerPredicate o) {
return new ChangeSource(5000) {
+ {
+ init("r25_byOwnerClosed", o);
+ }
+
@Override
ResultSet<Change> scan(ChangeAccess a) throws OrmException {
return a.byOwnerClosedAll(o.getAccountId());
@@ -417,10 +446,15 @@
return or(r25_byOwnerOpen(o), r25_byOwnerClosed(o));
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:open R=(reviewer:*)")
public Predicate<ChangeData> r30_byReviewerOpen(
@Named("R") final ReviewerPredicate r) {
return new Source() {
+ {
+ init("r30_byReviewerOpen", r);
+ }
+
@Override
public ResultSet<ChangeData> read() throws OrmException {
return ChangeDataResultSet.patchSetApproval(dbProvider.get()
@@ -445,10 +479,15 @@
};
}
+ @SuppressWarnings("unchecked")
@Rewrite("status:closed R=(reviewer:*)")
public Predicate<ChangeData> r30_byReviewerClosed(
@Named("R") final ReviewerPredicate r) {
return new Source() {
+ {
+ init("r30_byReviewerClosed", r);
+ }
+
@Override
public ResultSet<ChangeData> read() throws OrmException {
return ChangeDataResultSet.patchSetApproval(dbProvider.get()
