Use a match cache to skip duplicate queries for a change
Change-Id: I65859dc858b6eedfb8320ccaeb590ceaba413543
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java b/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java
new file mode 100644
index 0000000..53c41e1
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java
@@ -0,0 +1,57 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.task;
+
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import java.util.HashMap;
+import java.util.Map;
+
+public class MatchCache {
+ protected final PredicateCache predicateCache;
+ protected final ChangeData changeData;
+
+ protected final Map<String, Boolean> matchResultByQuery = new HashMap<>();
+
+ public MatchCache(PredicateCache predicateCache, ChangeData changeData) {
+ this.predicateCache = predicateCache;
+ this.changeData = changeData;
+ }
+
+ protected boolean match(String query) throws OrmException, QueryParseException {
+ if (query == null) {
+ return true;
+ }
+ Boolean isMatched = matchResultByQuery.get(query);
+ if (isMatched == null) {
+ isMatched = predicateCache.match(changeData, query);
+ matchResultByQuery.put(query, isMatched);
+ }
+ return isMatched;
+ }
+
+ protected Boolean matchOrNull(String query) {
+ if (query == null) {
+ return null;
+ }
+ Boolean isMatched = matchResultByQuery.get(query);
+ if (isMatched == null) {
+ isMatched = predicateCache.matchOrNull(changeData, query);
+ matchResultByQuery.put(query, isMatched);
+ }
+ return isMatched;
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
index 802944d..7b1faf0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
@@ -106,12 +106,18 @@
protected class AttributeFactory {
public ChangeData changeData;
public Node node;
+ public MatchCache matchCache;
protected Task definition;
protected TaskAttribute attribute;
protected AttributeFactory(ChangeData changeData, Node node) {
+ this(changeData, node, new MatchCache(predicateCache, changeData));
+ }
+
+ protected AttributeFactory(ChangeData changeData, Node node, MatchCache matchCache) {
this.changeData = changeData;
this.node = node;
+ this.matchCache = matchCache;
this.definition = node.definition;
this.attribute = new TaskAttribute(definition.name);
}
@@ -122,7 +128,7 @@
attribute.evaluationMilliSeconds = millis();
}
- boolean applicable = predicateCache.match(changeData, definition.applicable);
+ boolean applicable = matchCache.match(definition.applicable);
if (!definition.isVisible) {
if (!definition.isTrusted || (!applicable && !options.onlyApplicable)) {
return Optional.of(unknown());
@@ -146,8 +152,7 @@
attribute.applicable = applicable;
}
if (definition.inProgress != null) {
- attribute.inProgress =
- predicateCache.matchOrNull(changeData, definition.inProgress);
+ attribute.inProgress = matchCache.matchOrNull(definition.inProgress);
}
attribute.hint = getHint(attribute.status, definition);
attribute.exported = definition.exported;
@@ -187,7 +192,7 @@
}
if (definition.fail != null) {
- if (predicateCache.match(changeData, definition.fail)) {
+ if (matchCache.match(definition.fail)) {
// A FAIL definition is meant to be a hard blocking criteria
// (like a CodeReview -2). Thus, if hard blocked, it is
// irrelevant what the subtask states, or the PASS criteria are.
@@ -218,7 +223,7 @@
return Status.WAITING;
}
- if (definition.pass != null && !predicateCache.match(changeData, definition.pass)) {
+ if (definition.pass != null && !matchCache.match(definition.pass)) {
// Non-leaf tasks with no PASS criteria are supported in order
// to support "grouping tasks" (tasks with no function aside from
// organizing tasks). A task without a PASS criteria, cannot ever
@@ -245,7 +250,9 @@
if (subNode == null) {
subTasks.add(invalid());
} else {
- new AttributeFactory(changeData, subNode).create().ifPresent(t -> subTasks.add(t));
+ new AttributeFactory(changeData, subNode, matchCache)
+ .create()
+ .ifPresent(t -> subTasks.add(t));
}
}
if (subTasks.isEmpty()) {
@@ -256,9 +263,9 @@
protected boolean isValidQueries() {
try {
- predicateCache.match(changeData, definition.inProgress);
- predicateCache.match(changeData, definition.fail);
- predicateCache.match(changeData, definition.pass);
+ matchCache.match(definition.inProgress);
+ matchCache.match(definition.fail);
+ matchCache.match(definition.pass);
return true;
} catch (OrmException | QueryParseException | RuntimeException e) {
return false;
diff --git a/src/main/resources/Documentation/test.md b/src/main/resources/Documentation/test.md
new file mode 100644
index 0000000..d170e58
--- /dev/null
+++ b/src/main/resources/Documentation/test.md
@@ -0,0 +1,19 @@
+Manual Tests
+------------
+
+1. Test that task validation for more than one change provides different
+ results. This will ensure that the per-change match cache introduced
+ to avoid duplicate queries is being re-newed for each change.
+
+ Pick two changes which have different results for atleast one of the
+ task-applicable queries. For example, we can use a task with status:open
+ as applicability, one of the changes can be open and the other one merged.
+
+ For example, 12345 is open and 12346 is merged and atleast one task has
+ status:open as applicability. Below query should return different results
+ for both changes:
+
+ ssh -x -p 29418 review.example.com gerrit query
+ --format JSON 'change:12345 OR change:12346'
+ --task--all --task--preview 12347,1
+
