opendev/gerrit
Code Issues Proposed changes

Fix owner: search operator substring regression

Browse Source 
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>
This commit is contained in:
Shawn O. Pearce
2010-08-04 15:04:09 -07:00
parent 6cbee9cb64
commit f76fb4f82c
4 changed files with 146 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

89
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
View File

@@ -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 @@ public class AccountResolver {
* 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 &lt;email@example&gt;", 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 class AccountResolver {
*/
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 &lt;email@example&gt;", 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));
List<Account> m = schema.get().accounts().byFullName(nameOrEmail).toList();
if (m.size() == 1) {
return Collections.singleton(m.get(0).getId());
}
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();
// 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());
}
return null;
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());
}
private static Account oneAccount(final ResultSet<Account> rs) {
final List<Account> r = rs.toList();
return r.size() == 1 ? r.get(0) : null;
for (AccountExternalId extId : schema.get().accountExternalIds()
.suggestByEmailAddress(a, b, 10)) {
result.add(extId.getAccountId());
}
private Account findByUserName(final String userName) {
AccountState as = byId.getByUsername(userName);
return as != null ? as.getAccount() : null;
return result;
}
}

18
gerrit-server/src/main/java/com/google/gerrit/server/query/RewritePredicate.java
View File

@@ -20,10 +20,12 @@ import java.util.Collections;
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 @@ public abstract class RewritePredicate<T> extends Predicate<T> {
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

35
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -40,8 +40,11 @@ import com.google.inject.assistedinject.Assisted;
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 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
@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

43
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
View File

@@ -38,8 +38,7 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
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 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
};
}
@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()