From cdbe958a25a5afa61bcf1912bdaddc884ff3a96a Mon Sep 17 00:00:00 2001 From: Tomas Ljunggren Date: Mon, 28 Nov 2011 13:17:06 +0100 Subject: E-mail filter for publish comments In order to have more precise control of when e-mail notifications is sent a filter expression can now be configured in site configuration. An example filter configuration for QT sanity review bot: [review] filter = SRVW < 0 || reviewer != qt_sanity_bot If filter evaluates true the e-mail will be sent Task-number: QTQAINFRA-220 Change-Id: I827c8083ab3fadd7cc898031a9ac046d777f83f8 Reviewed-by: Tomas Ljunggren --- .../gerrit/server/patch/PublishComments.java | 27 +- .../gerrit/server/util/BooleanExpression.java | 370 +++++++++++++++++++++ .../gerrit/server/util/BooleanExpressionTest.java | 42 +++ 3 files changed, 437 insertions(+), 2 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/util/BooleanExpressionTest.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index 99c11dea3c..ce2e796d4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.PatchSetApproval; import com.google.gerrit.reviewdb.ReviewDb; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MergeQueue; @@ -36,17 +37,20 @@ import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchRefException; +import com.google.gerrit.server.util.BooleanExpression; import com.google.gerrit.server.workflow.FunctionState; import com.google.gwtjsonrpc.client.VoidResult; import com.google.gwtorm.client.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.text.ParseException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -78,6 +82,8 @@ public class PublishComments implements Callable { private final MergeOp.Factory mergeFactory; private final MergeQueue merger; + private final Config config; + private final PatchSet.Id patchSetId; private final String messageText; private final Set approvals; @@ -98,7 +104,7 @@ public class PublishComments implements Callable { final GitRepositoryManager gitManager, final MergeOp.Factory mergeFactory, final MergeQueue merger, - + @GerritServerConfig final Config config, @Assisted final PatchSet.Id patchSetId, @Assisted final String messageText, @Assisted final Set approvals) { @@ -113,7 +119,7 @@ public class PublishComments implements Callable { this.gitManager = gitManager; this.mergeFactory = mergeFactory; this.merger = merger; - + this.config = config; this.patchSetId = patchSetId; this.messageText = messageText; this.approvals = approvals; @@ -307,6 +313,21 @@ public class PublishComments implements Callable { private void email() { try { + String f = config.getString("review", null, "filter"); + if (f != null) { + // Only care about filter if available in configuration + BooleanExpression ef = new BooleanExpression(f); + HashMap am = new HashMap(); + for (ApprovalCategoryValue.Id a : approvals) { + am.put(a.getParentKey().get(), Short.toString(a.get())); + } + am.put("reviewer", user.getUserName()); + if (!ef.evaluate(am)) { + // If filter expression evaluates to false + // no email will be sent + return; + } + } final CommentSender cm = commentSenderFactory.create(change); cm.setFrom(user.getAccountId()); cm.setPatchSet(patchSet, patchSetInfoFactory.get(patchSetId)); @@ -317,6 +338,8 @@ public class PublishComments implements Callable { log.error("Cannot send comments by email for patch set " + patchSetId, e); } catch (PatchSetInfoNotAvailableException e) { log.error("Failed to obtain PatchSetInfo for patch set " + patchSetId, e); + } catch (ParseException e) { + log.error("Failed to parse filter expression from configuration", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java new file mode 100644 index 0000000000..cee3b1cc6d --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java @@ -0,0 +1,370 @@ +package com.google.gerrit.server.util; + +import java.text.ParseException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +public class BooleanExpression { + + private enum Token { + IDENTIFIER, NUMBER, TRUE, FALSE, LEFT_PARENTHESIS, RIGHT_PARENTHESIS, LOGICAL_AND, LOGICAL_OR, GREATER_THAN, GREATER_THAN_OR_EQUAL_TO, EQUAL_TO, NOT_EQUAL_TO, LESS_THAN, LESS_THAN_OR_EQUAL_TO, UNKNOWN, END_OF_INPUT, MINUS + } + + private static final int O_IDENTIFIER = -1; + private static final int O_NUMBER = -2; + private static final int O_TRUE = -3; + private static final int O_FALSE = -4; + private static final int O_AND = -5; + private static final int O_OR = -6; + private static final int O_EQUAL = -7; + private static final int O_GT = -8; + private static final int O_LT = -9; + private static final int O_LTOREQUAL = -10; + private static final int O_GTOREQUAL = -11; + private static final int O_NOTEQUAL = -12; + private static final int O_NONE = -1000; + + private List program = new ArrayList(); + private List identifiers = new ArrayList(); + private List numbers = new ArrayList(); + + private String input; + private int position; + private String value; + + private Token currentToken; + + /** + * Constructor. + * + * @param input Filter expression + */ + public BooleanExpression(String input) throws ParseException { + this.input = input; + position = 0; + nextToken(); + parseExpression(); + } + + public boolean evaluate(Map arguments) { + String[] stack = new String[16]; + int ppos = 0; + int spos = 0; + while (ppos < program.size()) { + int o = program.get(ppos); + if (o == O_IDENTIFIER) { + stack[spos++] = identifiers.get(program.get(++ppos)); + } else if (o == O_NUMBER) { + stack[spos++] = Integer.toString(numbers.get(program.get(++ppos))); + } else if (isLogicalOperator(o)) { + String b = stack[--spos]; + String a = stack[--spos]; + if (!isBooleanLiteral(a)) { + throw new IllegalArgumentException(a + " is not a boolean expression"); + } + if (!isBooleanLiteral(b)) { + throw new IllegalArgumentException(b + " is not a boolean expression"); + } + if (o == O_AND) { + stack[spos++] = logicalAnd(a, b); + } else if (o == O_OR) { + stack[spos++] = logicalOr(a, b); + } + } else if (isComparatorOperator(o)) { + String b = stack[--spos]; + String a = stack[--spos]; + if (arguments != null && arguments.get(a) != null) { + a = arguments.get(a); + } + if (arguments != null && arguments.get(b) != null) { + b = arguments.get(b); + } + if (o == O_EQUAL) { + stack[spos++] = Boolean.toString(a.equals(b)); + } else if (o == O_NOTEQUAL) { + stack[spos++] = Boolean.toString(!a.equals(b)); + } else { + // Only integer numbers can be compared + if (!isNumber(a)) { + throw new IllegalArgumentException(a + " is not a integer number"); + } + if (!isNumber(b)) { + throw new IllegalArgumentException(b + " is not a integer number"); + } + int ai = Integer.parseInt(a); + int bi = Integer.parseInt(b); + if (o == O_GT) { + stack[spos++] = Boolean.toString(ai > bi); + } else if (o == O_GTOREQUAL) { + stack[spos++] = Boolean.toString(ai >= bi); + } else if (o == O_LT) { + stack[spos++] = Boolean.toString(ai < bi); + } else if (o == O_LTOREQUAL) { + stack[spos++] = Boolean.toString(ai <= bi); + } + } + } else if (o == O_TRUE) { + stack[spos++] = "true"; + } else if (o == O_FALSE) { + stack[spos++] = "false"; + } + ppos++; + } + + return "true".equals(stack[spos - 1]); + } + + private boolean isBooleanLiteral(String value) { + if ("true".equals(value) || "false".equals(value)) { + return true; + } + return false; + } + + private boolean isNumber(String value) { + try { + Integer.parseInt(value); + return true; + } catch (NumberFormatException e) { + return false; + } + } + + private String logicalAnd(String a, String b) { + if ("true".equals(a) && "true".equals(b)) { + return "true"; + } + return "false"; + } + + private String logicalOr(String a, String b) { + if ("true".equals(a) || "true".equals(b)) { + return "true"; + } + return "false"; + } + + private boolean isLogicalOperator(int o) { + return (o == O_AND || o == O_OR); + } + + private boolean isComparatorOperator(int o) { + return (o == O_EQUAL || o == O_NOTEQUAL || o == O_GT || o == O_GTOREQUAL || o == O_LT || o == O_LTOREQUAL); + } + + private void nextToken() { + currentToken = Token.END_OF_INPUT; + if (position == input.length()) { + return; + } + char c = input.charAt(position); + while (Character.isWhitespace(c)) { + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + } + if (Character.isLetter(c)) { + parseIdentifier(); + } else if (Character.isDigit(c)) { + parseNumber(); + } else { + currentToken = Token.UNKNOWN; + if (c == '(') { + currentToken = Token.LEFT_PARENTHESIS; + } else if (c == ')') { + currentToken = Token.RIGHT_PARENTHESIS; + } else if (c == '>') { + currentToken = Token.GREATER_THAN; + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + if (c == '=') { + currentToken = Token.GREATER_THAN_OR_EQUAL_TO; + } + } else if (c == '<') { + currentToken = Token.LESS_THAN; + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + if (c == '=') { + currentToken = Token.LESS_THAN_OR_EQUAL_TO; + } + } else if (c == '&') { + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + if (c == '&') { + currentToken = Token.LOGICAL_AND; + } + } else if (c == '|') { + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + if (c == '|') { + currentToken = Token.LOGICAL_OR; + } + } else if (c == '=') { + currentToken = Token.EQUAL_TO; + } else if (c == '!') { + position++; + if (position == input.length()) { + return; + } + c = input.charAt(position); + if (c == '=') { + currentToken = Token.NOT_EQUAL_TO; + } + } else if (c == '-') { + currentToken = Token.MINUS; + } + if (currentToken != Token.UNKNOWN) { + position++; + } + } + } + + private void parseIdentifier() { + int s = position; + char c = input.charAt(position); + currentToken = Token.IDENTIFIER; + while (Character.isLetter(c) || c == '-' || c == '.' || c == '_') { + position++; + if (position == input.length()) { + break; + } + c = input.charAt(position); + } + value = input.substring(s, position); + if ("true".equalsIgnoreCase(value)) { + currentToken = Token.TRUE; + } else if ("false".equalsIgnoreCase(value)) { + currentToken = Token.FALSE; + } + } + + private void parseNumber() { + int s = position; + char c = input.charAt(position); + currentToken = Token.NUMBER; + while (Character.isDigit(c)) { + position++; + if (position == input.length()) { + break; + } + c = input.charAt(position); + } + value = input.substring(s, position); + } + + private void parseExpression() throws ParseException { + parseTerm(); + Token token = currentToken; + while (token == Token.LOGICAL_AND || token == Token.LOGICAL_OR) { + nextToken(); + parseTerm(); + program.add(toOperation(token)); + token = currentToken; + } + } + + private Integer toOperation(Token token) { + + int result = O_NONE; + + switch (token) { + case LOGICAL_AND: + result = O_AND; + break; + case LOGICAL_OR: + result = O_OR; + break; + case EQUAL_TO: + result = O_EQUAL; + break; + case NOT_EQUAL_TO: + result = O_NOTEQUAL; + break; + case GREATER_THAN: + result = O_GT; + break; + case GREATER_THAN_OR_EQUAL_TO: + result = O_GTOREQUAL; + break; + case LESS_THAN: + result = O_LT; + break; + case LESS_THAN_OR_EQUAL_TO: + result = O_LTOREQUAL; + break; + case TRUE: + result = O_TRUE; + break; + case FALSE: + result = O_FALSE; + break; + } + + return result; + + } + + private void parseTerm() throws ParseException { + parseElement(); + Token token = currentToken; + while (token == Token.EQUAL_TO || token == Token.NOT_EQUAL_TO || token == Token.GREATER_THAN + || token == Token.GREATER_THAN_OR_EQUAL_TO || token == Token.LESS_THAN + || token == Token.LESS_THAN_OR_EQUAL_TO) { + nextToken(); + parseElement(); + program.add(toOperation(token)); + token = currentToken; + } + } + + private void parseElement() throws ParseException { + if (currentToken == Token.LEFT_PARENTHESIS) { + nextToken(); + parseExpression(); + if (currentToken != Token.RIGHT_PARENTHESIS) { + throw new ParseException("Expected ')'", position); + } + nextToken(); + } else if (currentToken == Token.TRUE || currentToken == Token.FALSE) { + program.add(toOperation(currentToken)); + nextToken(); + } else if (currentToken == Token.IDENTIFIER) { + program.add(O_IDENTIFIER); + program.add(identifiers.size()); + identifiers.add(value); + nextToken(); + } else if (currentToken == Token.NUMBER) { + program.add(O_NUMBER); + program.add(numbers.size()); + numbers.add(Integer.parseInt(value)); + nextToken(); + } else if (currentToken == Token.MINUS) { + nextToken(); + if (currentToken == Token.NUMBER) { + program.add(O_NUMBER); + program.add(numbers.size()); + numbers.add(-Integer.parseInt(value)); + } + nextToken(); + } + else { + throw new ParseException("Unknown token", position); + } + + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/util/BooleanExpressionTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/util/BooleanExpressionTest.java new file mode 100644 index 0000000000..357fd7de23 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/util/BooleanExpressionTest.java @@ -0,0 +1,42 @@ +package com.google.gerrit.server.util; + +import static org.junit.Assert.*; + +import java.text.ParseException; +import java.util.HashMap; + +import org.junit.Test; + +public class BooleanExpressionTest { + + @Test + public void testEvaluate() { + + try { + assertTrue(new BooleanExpression("true && true && true").evaluate(null)); + assertTrue(new BooleanExpression("true || true || false").evaluate(null)); + assertTrue(new BooleanExpression("true || false || false").evaluate(null)); + assertTrue(new BooleanExpression("true || false || true").evaluate(null)); + assertTrue(new BooleanExpression("false || true || true").evaluate(null)); + assertTrue(new BooleanExpression("false || true || false").evaluate(null)); + assertFalse(new BooleanExpression("false || false || false").evaluate(null)); + assertFalse(new BooleanExpression("false && false || false").evaluate(null)); + assertFalse(new BooleanExpression("false && false && false").evaluate(null)); + assertTrue(new BooleanExpression("false && false || true").evaluate(null)); + assertTrue(new BooleanExpression("false = false").evaluate(null)); + assertTrue(new BooleanExpression("true = true").evaluate(null)); + assertFalse(new BooleanExpression("true = false").evaluate(null)); + assertFalse(new BooleanExpression("false = true").evaluate(null)); + + HashMap hashMap = new HashMap(); + hashMap.put("SRVW", "-1"); + hashMap.put("reviewer", "qt_sanity_bot"); + + assertTrue(new BooleanExpression("SRVW < 0 || reviewer != qt_sanity_bot").evaluate(hashMap)); + + } catch (ParseException e) { + fail(e.getMessage()); + } + } + +} -- cgit v1.2.3 From f1d4b8b86b8914689fee6b145c71290bd01677ca Mon Sep 17 00:00:00 2001 From: Tomas Ljunggren Date: Mon, 9 Jan 2012 18:37:34 +0100 Subject: Added a check to prevent NumberFormatException Since some parameters like review category will not be available at all time the boolean expression evaluator will evaluate expression containing these as false. Task-number: QTQAINFRA-220 Change-Id: Ic0471e52496c3ad74f51321d8a3bc721cdf1aaa1 Reviewed-by: Tomas Ljunggren --- .../gerrit/server/util/BooleanExpression.java | 30 ++++++++++------------ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java index cee3b1cc6d..8b986cdb3c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/BooleanExpression.java @@ -86,22 +86,20 @@ public class BooleanExpression { stack[spos++] = Boolean.toString(!a.equals(b)); } else { // Only integer numbers can be compared - if (!isNumber(a)) { - throw new IllegalArgumentException(a + " is not a integer number"); - } - if (!isNumber(b)) { - throw new IllegalArgumentException(b + " is not a integer number"); - } - int ai = Integer.parseInt(a); - int bi = Integer.parseInt(b); - if (o == O_GT) { - stack[spos++] = Boolean.toString(ai > bi); - } else if (o == O_GTOREQUAL) { - stack[spos++] = Boolean.toString(ai >= bi); - } else if (o == O_LT) { - stack[spos++] = Boolean.toString(ai < bi); - } else if (o == O_LTOREQUAL) { - stack[spos++] = Boolean.toString(ai <= bi); + if (!isNumber(a) || !isNumber(b)) { + stack[spos++] = "false"; + } else { + int ai = Integer.parseInt(a); + int bi = Integer.parseInt(b); + if (o == O_GT) { + stack[spos++] = Boolean.toString(ai > bi); + } else if (o == O_GTOREQUAL) { + stack[spos++] = Boolean.toString(ai >= bi); + } else if (o == O_LT) { + stack[spos++] = Boolean.toString(ai < bi); + } else if (o == O_LTOREQUAL) { + stack[spos++] = Boolean.toString(ai <= bi); + } } } } else if (o == O_TRUE) { -- cgit v1.2.3