Merge changes from topic "colon-in-query"
* changes:
QueryBuilder: Support : in field values
QueryBuilder: Remove support for AND/OR within a single field operator
Query.g: Add comment about FIELD_NAME to SINGLE_WORD rewrite
Add simple test of index-agnostic QueryBuilder
Add TreeSubject over antlr3 trees
diff --git a/antlr3/BUILD b/antlr3/BUILD
index fc96715..2d3050e 100644
--- a/antlr3/BUILD
+++ b/antlr3/BUILD
@@ -20,7 +20,8 @@
name = "query_parser",
srcs = [":query"],
visibility = [
- "//java/com/google/gerrit/index:__pkg__",
+ "//java/com/google/gerrit/index:__subpackages__",
+ "//javatests/com/google/gerrit:__subpackages__",
"//javatests/com/google/gerrit/index:__pkg__",
"//plugins:__pkg__",
],
diff --git a/antlr3/com/google/gerrit/index/query/Query.g b/antlr3/com/google/gerrit/index/query/Query.g
index 953a473..1bf20aa 100644
--- a/antlr3/com/google/gerrit/index/query/Query.g
+++ b/antlr3/com/google/gerrit/index/query/Query.g
@@ -120,12 +120,24 @@
;
conditionBase
: '('! conditionOr ')'!
- | (FIELD_NAME ':') => FIELD_NAME^ ':'! fieldValue
+ | (FIELD_NAME COLON) => FIELD_NAME^ COLON! fieldValue
| fieldValue -> ^(DEFAULT_FIELD fieldValue)
;
fieldValue
- : n=FIELD_NAME -> SINGLE_WORD[n]
+ // 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
;
@@ -134,6 +146,8 @@
OR: 'OR' ;
NOT: 'NOT' ;
+COLON: ':' ;
+
WS
: ( ' ' | '\r' | '\t' | '\n' ) { $channel=HIDDEN; }
;
@@ -172,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 12d1dd6..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,41 +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()) {
- // Expand multiple values, "foo:(a b c)", as though they were written
- // out with the longer form, "foo:a foo:b foo:c".
- //
- case AND:
- case OR:
- {
- List<Predicate<T>> p = new ArrayList<>(val.getChildCount());
- for (int i = 0; i < val.getChildCount(); i++) {
- final Tree c = val.getChild(i);
- if (c.getType() != DEFAULT_FIELD) {
- throw error("Nested operator not expected: " + c);
- }
- p.add(operator(name, onlyChildOf(c)));
- }
- return val.getType() == AND ? and(p) : or(p);
- }
+ 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:
- if (val.getChildCount() != 0) {
- throw error("Expected no children under: " + val);
- }
- return operator(name, val.getText());
-
+ 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/java/com/google/gerrit/index/query/testing/BUILD b/java/com/google/gerrit/index/query/testing/BUILD
new file mode 100644
index 0000000..030b327
--- /dev/null
+++ b/java/com/google/gerrit/index/query/testing/BUILD
@@ -0,0 +1,16 @@
+package(
+ default_testonly = True,
+ default_visibility = ["//visibility:public"],
+)
+
+java_library(
+ name = "testing",
+ srcs = glob(["*.java"]),
+ deps = [
+ "//antlr3:query_parser",
+ "//java/com/google/gerrit/index",
+ "//lib:guava",
+ "//lib/antlr:java-runtime",
+ "//lib/truth",
+ ],
+)
diff --git a/java/com/google/gerrit/index/query/testing/TreeSubject.java b/java/com/google/gerrit/index/query/testing/TreeSubject.java
new file mode 100644
index 0000000..c60b363
--- /dev/null
+++ b/java/com/google/gerrit/index/query/testing/TreeSubject.java
@@ -0,0 +1,73 @@
+// Copyright (C) 2019 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.google.gerrit.index.query.testing;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.truth.Truth.assertAbout;
+import static java.util.Objects.requireNonNull;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.Subject;
+import com.google.gerrit.index.query.QueryParser;
+import org.antlr.runtime.tree.Tree;
+
+public class TreeSubject extends Subject<TreeSubject, Tree> {
+ public static TreeSubject assertThat(Tree actual) {
+ return assertAbout(TreeSubject::new).that(actual);
+ }
+
+ private TreeSubject(FailureMetadata failureMetadata, Tree tree) {
+ super(failureMetadata, tree);
+ }
+
+ public void hasType(int expectedType) {
+ isNotNull();
+ check("getType()").that(typeName(actual().getType())).isEqualTo(typeName(expectedType));
+ }
+
+ public void hasText(String expectedText) {
+ requireNonNull(expectedText);
+ isNotNull();
+ check("getText()").that(actual().getText()).isEqualTo(expectedText);
+ }
+
+ public void hasNoChildren() {
+ isNotNull();
+ check("getChildCount()").that(actual().getChildCount()).isEqualTo(0);
+ }
+
+ public void hasChildCount(int expectedChildCount) {
+ checkArgument(
+ expectedChildCount > 0, "expected child count must be positive: %s", expectedChildCount);
+ isNotNull();
+ check("getChildCount()").that(actual().getChildCount()).isEqualTo(expectedChildCount);
+ }
+
+ public TreeSubject child(int childIndex) {
+ isNotNull();
+ return check("getChild(%s)", childIndex)
+ .about(TreeSubject::new)
+ .that(actual().getChild(childIndex));
+ }
+
+ private static String typeName(int type) {
+ checkArgument(
+ type >= 0 && type < QueryParser.tokenNames.length,
+ "invalid token type %s, max is %s",
+ type,
+ QueryParser.tokenNames.length - 1);
+ return QueryParser.tokenNames[type];
+ }
+}
diff --git a/javatests/com/google/gerrit/index/BUILD b/javatests/com/google/gerrit/index/BUILD
index e3436bc..a1f60de 100644
--- a/javatests/com/google/gerrit/index/BUILD
+++ b/javatests/com/google/gerrit/index/BUILD
@@ -9,6 +9,7 @@
"//antlr3:query_parser",
"//java/com/google/gerrit/index",
"//java/com/google/gerrit/index:query_exception",
+ "//java/com/google/gerrit/index/query/testing",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:guava",
"//lib:junit",
diff --git a/javatests/com/google/gerrit/index/query/QueryBuilderTest.java b/javatests/com/google/gerrit/index/query/QueryBuilderTest.java
new file mode 100644
index 0000000..d275fa8
--- /dev/null
+++ b/javatests/com/google/gerrit/index/query/QueryBuilderTest.java
@@ -0,0 +1,125 @@
+// Copyright (C) 2019 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.google.gerrit.index.query;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.truth.ThrowableSubject;
+import com.google.gerrit.testing.GerritBaseTests;
+import java.util.Collection;
+import java.util.Objects;
+import org.junit.Test;
+
+public class QueryBuilderTest extends GerritBaseTests {
+ private static class TestPredicate extends Predicate<Object> {
+ private final String field;
+ private final String value;
+
+ TestPredicate(String field, String value) {
+ this.field = field;
+ this.value = value;
+ }
+
+ @Override
+ public Predicate<Object> copy(Collection<? extends Predicate<Object>> children) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(field, value);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof TestPredicate)) {
+ return false;
+ }
+ TestPredicate p = (TestPredicate) o;
+ return Objects.equals(field, p.field) && Objects.equals(value, p.value);
+ }
+ }
+
+ private static class TestQueryBuilder extends QueryBuilder<Object> {
+ TestQueryBuilder() {
+ super(new QueryBuilder.Definition<>(TestQueryBuilder.class));
+ }
+
+ @Operator
+ public Predicate<Object> a(String value) {
+ return new TestPredicate("a", value);
+ }
+ }
+
+ @Test
+ public void fieldNameAndValue() throws Exception {
+ assertThat(parse("a:foo")).isEqualTo(new TestPredicate("a", "foo"));
+ }
+
+ @Test
+ public void fieldWithParenthesizedValues() throws Exception {
+ assertThatParseException("a:(foo bar)")
+ .hasMessageThat()
+ .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);
+ }
+
+ private static ThrowableSubject assertThatParseException(String query) {
+ try {
+ new TestQueryBuilder().parse(query);
+ throw new AssertionError("expected QueryParseException for " + query);
+ } catch (QueryParseException e) {
+ return assertThat(e);
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/index/query/QueryParserTest.java b/javatests/com/google/gerrit/index/query/QueryParserTest.java
index 2175f7d..250c40c 100644
--- a/javatests/com/google/gerrit/index/query/QueryParserTest.java
+++ b/javatests/com/google/gerrit/index/query/QueryParserTest.java
@@ -14,7 +14,13 @@
package com.google.gerrit.index.query;
-import static org.junit.Assert.assertEquals;
+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;
+import static com.google.gerrit.index.query.testing.TreeSubject.assertThat;
import com.google.gerrit.testing.GerritBaseTests;
import org.antlr.runtime.tree.Tree;
@@ -22,27 +28,172 @@
public class QueryParserTest extends GerritBaseTests {
@Test
- public void projectBare() throws QueryParseException {
- Tree r;
-
- r = parse("project:tools/gerrit");
- assertSingleWord("project", "tools/gerrit", r);
-
- r = parse("project:tools/*");
- assertSingleWord("project", "tools/*", r);
+ public void fieldNameAndValue() throws Exception {
+ Tree r = parse("project:tools/gerrit");
+ assertThat(r).hasType(FIELD_NAME);
+ assertThat(r).hasText("project");
+ assertThat(r).hasChildCount(1);
+ assertThat(r).child(0).hasType(SINGLE_WORD);
+ assertThat(r).child(0).hasText("tools/gerrit");
+ assertThat(r).child(0).hasNoChildren();
}
- private static void assertSingleWord(String name, String value, Tree r) {
- assertEquals(QueryParser.FIELD_NAME, r.getType());
- assertEquals(name, r.getText());
- assertEquals(1, r.getChildCount());
- final Tree c = r.getChild(0);
- assertEquals(QueryParser.SINGLE_WORD, c.getType());
- assertEquals(value, c.getText());
- assertEquals(0, c.getChildCount());
+ @Test
+ public void fieldNameAndValueThatLooksLikeFieldNameColon() throws Exception {
+ // This should work, but doesn't due to a known issue.
+ assertParseFails("project:foo:");
}
- private static Tree parse(String str) throws QueryParseException {
- return QueryParser.parse(str);
+ @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.
+ }
}
}
