Fix endless loop when using "is:watched" in project watches

The "is:watched" predicate matches changes that are being watched.

To match changes that are being watched the "is:watched" predicate is
expanded to an OR query that has one "project:<watched-project>
<project-watch-filter>" predicate per project watch
("project:<watched-project>" is omitted if the project watch is the
All-Projects project, "<project-watch-filter>" is omitted if the project
watch doesn't set a filter). This expansion happens when the
IsWatchedByPredicate is instantiated. Expanding the query requires
loading the project watches of the user and parsing the filter (to
convert the filter string into Predicates).

If the filter of a project watch used the "is:watched" predicate,
querying changes by "is:watched" or checking whether a change matches
the project watch triggered an endless loop: If "is:watched" is used
ChangeQueryBuilder.parse creates an IsWatchedByPredicate instance (in
the is(String) method), which is expanded to the OR query (in the
IsWatchedByPredicate constructor), which requires parsing the project
watch filters. Parsing the project watch filters was done by
ChangeQueryBuilder.parse which creates another IsWatchedByPredicate
instance (in the is(String) method), which is again expanded to the OR
query (in the IsWatchedByPredicate constructor), which again requires
parsing the project watch filters, starting the loop anew.

To fix this we:

1. Disallow using "is:watched" in ProjectWatch.WatcherChangeQueryBuilder
   which is a subclass of ChangeQueryBuilder.parse used to check whether
   a change matches a project watch.

2. Change IsWatchedByPredicate to use
   ProjectWatch.WatcherChangeQueryBuilder to parse the project watch
   filters instead of ChangeQueryBuilder.

Using ProjectWatch.WatcherChangeQueryBuilder in IsWatchedByPredicate
makes the matching logic for project watches when a change is updated
consistent with the matching logic for project watches when "is:watched"
is used in a regular change query.

By disallowing "is:watched" in ProjectWatch.WatcherChangeQueryBuilder
project watches that use "is:watched" in their filter do not match any
change now. Before this change they triggered an endless loop, affecting
the stability of the service.

Note, IsWatchedByPredicate did have a check to prevent an endless loop
in this case, but it didn't work since the endless loop happened before
this check was reached.

Bug: Issue 321784734
Release-Notes: Fix endless loop when using "is:watched" in project watches
Change-Id: Ie38535b2df123a62dfd6a6e4b4ee60a80b0254f3
Signed-off-by: Edwin Kempin <ekempin@google.com>

4 files changed, 61 insertions, 11 deletions

diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index d71033a25d..94a0e37094 100644
--- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -267,8 +267,8 @@ public class ProjectWatch {
     return p == null || p.asMatchable().match(changeData);
   }
 
-  private static class WatcherChangeQueryBuilder extends ChangeQueryBuilder {
-    private WatcherChangeQueryBuilder(Arguments args) {
+  public static class WatcherChangeQueryBuilder extends ChangeQueryBuilder {
+    public WatcherChangeQueryBuilder(Arguments args) {
       super(args);
     }
 
@@ -301,5 +301,16 @@ public class ProjectWatch {
       // predicates.
       return Predicate.or(predicates);
     }
+
+    @Override
+    public Predicate<ChangeData> is(String value) throws QueryParseException {
+      if ("watched".equalsIgnoreCase(value)) {
+        // project watches cannot use "is:watched" as this would trigger an endless loop in
+        // IsWatchedByPredicate
+        throw new QueryParseException(
+            String.format("Operator 'is:watched' cannot be used in project watches."));
+      }
+      return super.is(value);
+    }
   }
 }
diff --git a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
index 046d24c74b..7144d3e04e 100644
--- a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
+++ b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
@@ -15,18 +15,21 @@
 package com.google.gerrit.server.query.change;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.index.query.AndPredicate;
 import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryBuilder;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.ProjectWatches.ProjectWatchKey;
+import com.google.gerrit.server.mail.send.ProjectWatch;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
 public class IsWatchedByPredicate extends AndPredicate<ChangeData> {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   protected static String describe(CurrentUser user) {
     if (user.isIdentifiedUser()) {
       return user.getAccountId().toString();
@@ -44,20 +47,16 @@ public class IsWatchedByPredicate extends AndPredicate<ChangeData> {
   protected static ImmutableList<Predicate<ChangeData>> filters(ChangeQueryBuilder.Arguments args)
       throws QueryParseException {
     List<Predicate<ChangeData>> r = new ArrayList<>();
-    ChangeQueryBuilder builder = new ChangeQueryBuilder(args);
+    ProjectWatch.WatcherChangeQueryBuilder builder =
+        new ProjectWatch.WatcherChangeQueryBuilder(args);
     for (ProjectWatchKey w : getWatches(args)) {
       Predicate<ChangeData> f = null;
       if (w.filter() != null) {
         try {
           f = builder.parse(w.filter());
-          if (QueryBuilder.find(f, IsWatchedByPredicate.class) != null) {
-            // If the query is going to infinite loop, assume it
-            // will never match and return null. Yes this test
-            // prevents you from having a filter that matches what
-            // another user is filtering on. :-)
-            continue;
-          }
         } catch (QueryParseException e) {
+          logger.atFine().log(
+              "Ignoring non-parseable filter of project watch %s: %s", w, e.getMessage());
           continue;
         }
       }
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index 014933f6fb..5accd00a0f 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -694,4 +694,26 @@ public class ProjectWatchIT extends AbstractDaemonTest {
     assertThat(m.body()).contains("Change subject: TRIGGER\n");
     assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
   }
+
+  @Test
+  public void watchThatUsesIsWatchedDoesntMatchAnything() throws Exception {
+    String watchedProject = projectOperations.newProject().create().get();
+
+    // configure a project watch for user that uses "is:watched"
+    requestScopeOperations.setApiUser(user.id());
+    watch(watchedProject, "is:watched");
+
+    // create a change as admin user that may trigger the project watch
+    requestScopeOperations.setApiUser(admin.id());
+    TestRepository<InMemoryRepository> watchedRepo =
+        cloneProject(Project.nameKey(watchedProject), admin);
+    PushOneCommit.Result r =
+        pushFactory
+            .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+            .to("refs/for/master");
+    r.assertOkStatus();
+
+    // assert that there was no email notification for user
+    assertThat(sender.getMessages()).isEmpty();
+  }
 }
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 7a7cff58f4..a0aad96c58 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -3627,6 +3627,24 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   }
 
   @Test
+  public void watched_projectWatchThatUsesIsWatchedIsIgnored() throws Exception {
+    Project.NameKey project = Project.nameKey("repo");
+    createProject(project);
+    insert(project, newChangeWithStatus(project, Change.Status.NEW));
+
+    List<ProjectWatchInfo> projectsToWatch = new ArrayList<>();
+    ProjectWatchInfo pwi = new ProjectWatchInfo();
+    pwi.project = "repo";
+    pwi.filter = "is:watched";
+    pwi.notifyNewChanges = true;
+    projectsToWatch.add(pwi);
+    gApi.accounts().self().setWatchedProjects(projectsToWatch);
+    resetUser();
+
+    assertQuery("is:watched");
+  }
+
+  @Test
   public void trackingid() throws Exception {
     Project.NameKey project = Project.nameKey("repo");
     repo = createAndOpenProject(project);