diff options
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); |