QueryBuilder: Support : in field values
Prior to this change, "field:foo:bar" is parsed as "field:foo", silently
discarding the ":bar". Worse, "field:foo:bar baz:quux" is _also_ parsed
as "field:foo", discarding not just part of the first term but the
entire second term.
Fix this by treating "SINGLE_WORD ':' fieldValue" as a valid definition
of a field value, and add tests.
This solution seems inelegant in general; it seems like we should be
able to concatenate the tokens together into a single AST node at the
Antlr level. Also, it doesn't work in all cases: it fails to parse field
values ending in ':'
I'm far from an Antlr expert and there may be a better/easier way to do
this. But this change is still strictly better than it was before, and
it sets an example of how to write tests for parser functionality.
Change-Id: If275623b3244b02f81114fb2997ba16ee1492a4f
diff --git a/antlr3/com/google/gerrit/index/query/Query.g b/antlr3/com/google/gerrit/index/query/Query.g
index 2f2f629..1bf20aa 100644
--- a/antlr3/com/google/gerrit/index/query/Query.g
+++ b/antlr3/com/google/gerrit/index/query/Query.g
@@ -120,13 +120,24 @@
;
conditionBase
: '('! conditionOr ')'!
- | (FIELD_NAME ':') => FIELD_NAME^ ':'! fieldValue
+ | (FIELD_NAME COLON) => FIELD_NAME^ COLON! fieldValue
| fieldValue -> ^(DEFAULT_FIELD fieldValue)
;
fieldValue
// Rewrite by invoking SINGLE_WORD fragment lexer rule, passing the field name as an argument.
: n=FIELD_NAME -> SINGLE_WORD[n]
+
+ // Allow field values to contain a colon. We can't do this at the lexer level, because we need to
+ // emit a separate token for the field name. If we were to allow ':' in SINGLE_WORD, then
+ // everything would just lex as DEFAULT_FIELD.
+ //
+ // Field values with a colon may be lexed either as <field>:<rest> or <word>:<rest>, depending on
+ // whether the part before the colon looks like a field name.
+ // TODO(dborowitz): Field values ending in colon still don't work.
+ | (FIELD_NAME COLON) => n=FIELD_NAME COLON fieldValue -> SINGLE_WORD[n] COLON fieldValue
+ | (SINGLE_WORD COLON) => SINGLE_WORD COLON fieldValue
+
| SINGLE_WORD
| EXACT_PHRASE
;
@@ -135,6 +146,8 @@
OR: 'OR' ;
NOT: 'NOT' ;
+COLON: ':' ;
+
WS
: ( ' ' | '\r' | '\t' | '\n' ) { $channel=HIDDEN; }
;
@@ -173,7 +186,7 @@
// '-' permit
// '.' permit
// '/' permit
- | ':'
+ | COLON
| ';'
// '<' permit
// '=' permit
diff --git a/java/com/google/gerrit/index/query/QueryBuilder.java b/java/com/google/gerrit/index/query/QueryBuilder.java
index 41cefa1..04a77ca 100644
--- a/java/com/google/gerrit/index/query/QueryBuilder.java
+++ b/java/com/google/gerrit/index/query/QueryBuilder.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.index.query.Predicate.not;
import static com.google.gerrit.index.query.Predicate.or;
import static com.google.gerrit.index.query.QueryParser.AND;
+import static com.google.gerrit.index.query.QueryParser.COLON;
import static com.google.gerrit.index.query.QueryParser.DEFAULT_FIELD;
import static com.google.gerrit.index.query.QueryParser.EXACT_PHRASE;
import static com.google.gerrit.index.query.QueryParser.FIELD_NAME;
@@ -217,24 +218,41 @@
return defaultField(onlyChildOf(r));
case FIELD_NAME:
- return operator(r.getText(), onlyChildOf(r));
+ return operator(r.getText(), concatenateChildText(r));
default:
throw error("Unsupported operator: " + r);
}
}
- private Predicate<T> operator(String name, Tree val) throws QueryParseException {
- switch (val.getType()) {
- case SINGLE_WORD:
- case EXACT_PHRASE:
- if (val.getChildCount() != 0) {
- throw error("Expected no children under: " + val);
- }
- return operator(name, val.getText());
+ private static String concatenateChildText(Tree r) throws QueryParseException {
+ if (r.getChildCount() == 0) {
+ throw error("Expected children under: " + r);
+ }
+ if (r.getChildCount() == 1) {
+ return getFieldValue(r.getChild(0));
+ }
+ StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < r.getChildCount(); i++) {
+ sb.append(getFieldValue(r.getChild(i)));
+ }
+ return sb.toString();
+ }
+ private static String getFieldValue(Tree r) throws QueryParseException {
+ if (r.getChildCount() != 0) {
+ throw error("Expected no children under: " + r);
+ }
+ switch (r.getType()) {
+ case SINGLE_WORD:
+ case COLON:
+ case EXACT_PHRASE:
+ return r.getText();
default:
- throw error("Unsupported node in operator " + name + ": " + val);
+ throw error(
+ String.format(
+ "Unsupported %s node in operator %s: %s",
+ QueryParser.tokenNames[r.getType()], r.getParent(), r));
}
}
diff --git a/javatests/com/google/gerrit/index/query/QueryBuilderTest.java b/javatests/com/google/gerrit/index/query/QueryBuilderTest.java
index 10fa0e8..d275fa8 100644
--- a/javatests/com/google/gerrit/index/query/QueryBuilderTest.java
+++ b/javatests/com/google/gerrit/index/query/QueryBuilderTest.java
@@ -75,6 +75,41 @@
.isEqualTo("line 1:2 no viable alternative at input '('");
}
+ @Test
+ public void fieldNameAndValueThatLooksLikeFieldNameColonValue() throws Exception {
+ assertThat(parse("a:foo:bar")).isEqualTo(new TestPredicate("a", "foo:bar"));
+ }
+
+ @Test
+ public void fieldNameAndValueThatLooksLikeWordColonValue() throws Exception {
+ assertThat(parse("a:*:bar")).isEqualTo(new TestPredicate("a", "*:bar"));
+ }
+
+ @Test
+ public void fieldNameAndValueWithMultipleColons() throws Exception {
+ assertThat(parse("a:*:*:*")).isEqualTo(new TestPredicate("a", "*:*:*"));
+ }
+
+ @Test
+ public void exactPhraseWithQuotes() throws Exception {
+ assertThat(parse("a:\"foo bar\"")).isEqualTo(new TestPredicate("a", "foo bar"));
+ }
+
+ @Test
+ public void exactPhraseWithQuotesAndColon() throws Exception {
+ assertThat(parse("a:\"foo:bar\"")).isEqualTo(new TestPredicate("a", "foo:bar"));
+ }
+
+ @Test
+ public void exactPhraseWithBraces() throws Exception {
+ assertThat(parse("a:{foo bar}")).isEqualTo(new TestPredicate("a", "foo bar"));
+ }
+
+ @Test
+ public void exactPhraseWithBracesAndColon() throws Exception {
+ assertThat(parse("a:{foo:bar}")).isEqualTo(new TestPredicate("a", "foo:bar"));
+ }
+
private static Predicate<Object> parse(String query) throws Exception {
return new TestQueryBuilder().parse(query);
}
diff --git a/javatests/com/google/gerrit/index/query/QueryParserTest.java b/javatests/com/google/gerrit/index/query/QueryParserTest.java
index cf1f035..250c40c 100644
--- a/javatests/com/google/gerrit/index/query/QueryParserTest.java
+++ b/javatests/com/google/gerrit/index/query/QueryParserTest.java
@@ -14,6 +14,9 @@
package com.google.gerrit.index.query;
+import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.index.query.QueryParser.AND;
+import static com.google.gerrit.index.query.QueryParser.COLON;
import static com.google.gerrit.index.query.QueryParser.FIELD_NAME;
import static com.google.gerrit.index.query.QueryParser.SINGLE_WORD;
import static com.google.gerrit.index.query.QueryParser.parse;
@@ -34,4 +37,163 @@
assertThat(r).child(0).hasText("tools/gerrit");
assertThat(r).child(0).hasNoChildren();
}
+
+ @Test
+ public void fieldNameAndValueThatLooksLikeFieldNameColon() throws Exception {
+ // This should work, but doesn't due to a known issue.
+ assertParseFails("project:foo:");
+ }
+
+ @Test
+ public void fieldNameAndValueThatLooksLikeFieldNameColonValue() throws Exception {
+ Tree r = parse("project:foo:bar");
+ assertThat(r).hasType(FIELD_NAME);
+ assertThat(r).hasText("project");
+ assertThat(r).hasChildCount(3);
+ assertThat(r).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).hasText("foo");
+ assertThat(r).child(0).hasNoChildren();
+ assertThat(r).child(1).hasType(COLON);
+ assertThat(r).child(1).hasText(":");
+ assertThat(r).child(1).hasNoChildren();
+ assertThat(r).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(2).hasText("bar");
+ assertThat(r).child(2).hasNoChildren();
+ }
+
+ @Test
+ public void fieldNameAndValueThatLooksLikeWordColonValue() throws Exception {
+ Tree r = parse("project:x*y:a*b");
+ assertThat(r).hasType(FIELD_NAME);
+ assertThat(r).hasText("project");
+ assertThat(r).hasChildCount(3);
+ assertThat(r).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).hasText("x*y");
+ assertThat(r).child(0).hasNoChildren();
+ assertThat(r).child(1).hasType(COLON);
+ assertThat(r).child(1).hasText(":");
+ assertThat(r).child(1).hasNoChildren();
+ assertThat(r).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(2).hasText("a*b");
+ assertThat(r).child(2).hasNoChildren();
+ }
+
+ @Test
+ public void fieldNameAndValueThatLooksLikeWordColon() throws Exception {
+ // This should work, but doesn't due to a known issue.
+ assertParseFails("project:x*y:");
+ }
+
+ @Test
+ public void fieldNameAndValueWithMultipleColons() throws Exception {
+ Tree r = parse("project:*:*:*");
+ assertThat(r).hasType(FIELD_NAME);
+ assertThat(r).hasText("project");
+ assertThat(r).hasChildCount(5);
+ assertThat(r).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).hasText("*");
+ assertThat(r).child(0).hasNoChildren();
+ assertThat(r).child(1).hasType(COLON);
+ assertThat(r).child(1).hasText(":");
+ assertThat(r).child(1).hasNoChildren();
+ assertThat(r).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(2).hasText("*");
+ assertThat(r).child(2).hasNoChildren();
+ assertThat(r).child(3).hasType(COLON);
+ assertThat(r).child(3).hasText(":");
+ assertThat(r).child(3).hasNoChildren();
+ assertThat(r).child(4).hasType(SINGLE_WORD);
+ assertThat(r).child(4).hasText("*");
+ assertThat(r).child(4).hasNoChildren();
+ }
+
+ @Test
+ public void fieldNameAndValueWithColonFollowedByAnotherField() throws Exception {
+ Tree r = parse("project:foo:bar file:baz");
+ assertThat(r).hasType(AND);
+ assertThat(r).hasChildCount(2);
+
+ assertThat(r).child(0).hasType(FIELD_NAME);
+ assertThat(r).child(0).hasText("project");
+ assertThat(r).child(0).hasChildCount(3);
+ assertThat(r).child(0).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(0).hasText("foo");
+ assertThat(r).child(0).child(0).hasNoChildren();
+ assertThat(r).child(0).child(1).hasType(COLON);
+ assertThat(r).child(0).child(1).hasText(":");
+ assertThat(r).child(0).child(1).hasNoChildren();
+ assertThat(r).child(0).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(2).hasText("bar");
+ assertThat(r).child(0).child(2).hasNoChildren();
+
+ assertThat(r).child(1).hasType(FIELD_NAME);
+ assertThat(r).child(1).hasText("file");
+ assertThat(r).child(1).hasChildCount(1);
+ assertThat(r).child(1).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(1).child(0).hasText("baz");
+ assertThat(r).child(1).child(0).hasNoChildren();
+ }
+
+ @Test
+ public void fieldNameAndValueWithColonFollowedByOpenParen() throws Exception {
+ Tree r = parse("project:foo:bar (file:baz)");
+ assertThat(r).hasType(AND);
+ assertThat(r).hasChildCount(2);
+
+ assertThat(r).child(0).hasType(FIELD_NAME);
+ assertThat(r).child(0).hasText("project");
+ assertThat(r).child(0).hasChildCount(3);
+ assertThat(r).child(0).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(0).hasText("foo");
+ assertThat(r).child(0).child(0).hasNoChildren();
+ assertThat(r).child(0).child(1).hasType(COLON);
+ assertThat(r).child(0).child(1).hasText(":");
+ assertThat(r).child(0).child(1).hasNoChildren();
+ assertThat(r).child(0).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(2).hasText("bar");
+ assertThat(r).child(0).child(2).hasNoChildren();
+
+ assertThat(r).child(1).hasType(FIELD_NAME);
+ assertThat(r).child(1).hasText("file");
+ assertThat(r).child(1).hasChildCount(1);
+ assertThat(r).child(1).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(1).child(0).hasText("baz");
+ assertThat(r).child(1).child(0).hasNoChildren();
+ }
+
+ @Test
+ public void fieldNameAndValueWithColonFollowedByCloseParen() throws Exception {
+ Tree r = parse("(project:foo:bar) file:baz");
+ assertThat(r).hasType(AND);
+ assertThat(r).hasChildCount(2);
+
+ assertThat(r).child(0).hasType(FIELD_NAME);
+ assertThat(r).child(0).hasText("project");
+ assertThat(r).child(0).hasChildCount(3);
+ assertThat(r).child(0).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(0).hasText("foo");
+ assertThat(r).child(0).child(0).hasNoChildren();
+ assertThat(r).child(0).child(1).hasType(COLON);
+ assertThat(r).child(0).child(1).hasText(":");
+ assertThat(r).child(0).child(1).hasNoChildren();
+ assertThat(r).child(0).child(2).hasType(SINGLE_WORD);
+ assertThat(r).child(0).child(2).hasText("bar");
+ assertThat(r).child(0).child(2).hasNoChildren();
+
+ assertThat(r).child(1).hasType(FIELD_NAME);
+ assertThat(r).child(1).hasText("file");
+ assertThat(r).child(1).hasChildCount(1);
+ assertThat(r).child(1).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(1).child(0).hasText("baz");
+ assertThat(r).child(1).child(0).hasNoChildren();
+ }
+
+ private static void assertParseFails(String query) {
+ try {
+ parse(query);
+ assert_().fail("expected parse to fail: %s", query);
+ } catch (QueryParseException e) {
+ // Expected.
+ }
+ }
}
