Fix duplicated results on status:open project:P branch:B
The changes().byBranchClosedPrev() method does not support pagination
within the database. It only loads all open changes on the supplied
project/branch pair. Because it does not support pagination, we cannot
accept the sortkey_after and limit predicates as part of this rewrite.
AndSource has a special case within it where Paginated sources will
be run again using the sort key from a prior result row whenever
the AndSource has not yet gathered the caller's requested limit
worth of results. When the rewritten predicate ignores sortkey_after
and just returns rows already seen by the AndSource, AndSource assumes
these are new and appends them to the results.
This re-execution of the Paginated (but not really!) ChangeSource
is what causes duplicate results with the topic predicate. But the
topic predicate was just a misleading fact, it actually produces
duplicates anytime the user can see at least one change that matches
the rest of their query string, they see the results repeated out
until the requested pagination limit.
Whenever there are no results, e.g. past the end of pagination, or
there are no matches at all, the server was going into an infinite
loop within AndSource, constantly re-running the same database query
to scan all open changes by the supplied project and branch pair.
This could overload a database server, or Gerrit server with a lot
of unnecessary CPU load.
The correct solution is to declare the ChangeSource to be *not*
Paginated by extending ChangeSource rather than PaginatedSource,
and don't consume the sortkey_after and limit predicates from the
query, allowing those to be applied elsewhere in the query tree.
Change-Id: I25a0171ad23d146c093be1f129fe6f0f41fe7681
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 bf2e4cc..34ec2f4 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
@@ -119,50 +119,23 @@
return a.getValue().compareTo(b.getValue()) >= 0 ? a : b;
}
- @Rewrite("status:open P=(project:*) B=(branch:*) S=(sortkey_after:*) L=(limit:*)")
- public Predicate<ChangeData> r05_byBranchOpenPrev(
+ @Rewrite("status:open P=(project:*) B=(branch:*)")
+ public Predicate<ChangeData> r05_byBranchOpen(
@Named("P") final ProjectPredicate p,
- @Named("B") final BranchPredicate b,
- @Named("S") final SortKeyPredicate.After s,
- @Named("L") final IntPredicate<ChangeData> l) {
- return new PaginatedSource(500, s.getValue(), l.intValue()) {
+ @Named("B") final BranchPredicate b) {
+ return new ChangeSource(500) {
@Override
- ResultSet<Change> scan(ChangeAccess a, String key, int limit)
+ ResultSet<Change> scan(ChangeAccess a)
throws OrmException {
- return a.byBranchOpenAll(new Branch.NameKey(p.getValueKey(), b
- .getValue()));
+ return a.byBranchOpenAll(
+ new Branch.NameKey(p.getValueKey(), b.getValue()));
}
@Override
public boolean match(ChangeData cd) throws OrmException {
- return cd.change(dbProvider).getStatus().isOpen() //
- && p.match(cd) //
- && b.match(cd) //
- && s.match(cd);
- }
- };
- }
-
- @Rewrite("status:open P=(project:*) B=(branch:*) S=(sortkey_before:*) L=(limit:*)")
- public Predicate<ChangeData> r05_byBranchOpenNext(
- @Named("P") final ProjectPredicate p,
- @Named("B") final BranchPredicate b,
- @Named("S") final SortKeyPredicate.Before s,
- @Named("L") final IntPredicate<ChangeData> l) {
- return new PaginatedSource(500, s.getValue(), l.intValue()) {
- @Override
- ResultSet<Change> scan(ChangeAccess a, String key, int limit)
- throws OrmException {
- return a.byBranchOpenAll(new Branch.NameKey(p.getValueKey(), b
- .getValue()));
- }
-
- @Override
- public boolean match(ChangeData cd) throws OrmException {
- return cd.change(dbProvider).getStatus().isOpen() //
- && p.match(cd) //
- && b.match(cd) //
- && s.match(cd);
+ return cd.change(dbProvider).getStatus().isOpen()
+ && p.match(cd)
+ && b.match(cd);
}
};
}
