opendev/gerrit
Code Issues Proposed changes

QueryBuilder: Support : in field values

Browse Source 
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
This commit is contained in:
Dave Borowitz
2019-03-22 14:23:23 -07:00
parent 37fe9cc1f5
commit 12e3757e3b
4 changed files with 240 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
javatests/com/google/gerrit/index/query/QueryBuilderTest.java
View File

@@ -75,6 +75,41 @@ public class QueryBuilderTest extends GerritBaseTests {
.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);
}

162
javatests/com/google/gerrit/index/query/QueryParserTest.java
View File

@@ -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 @@ public class QueryParserTest extends GerritBaseTests {
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.
}
}
}