Skip to content

Commit

Permalink
Search Template - parse template if it is provided as a single escape…
Browse files Browse the repository at this point in the history 
…d string. Closes elastic#8308

- Search template is correctly parsed if it is provided as single escaped string
- Ignore exceptions in case of second parsing of template content (assuming it is escaped template string content)
- New tests for relevant use cases

Conflicts:
	src/main/java/org/elasticsearch/search/SearchService.java
  • Loading branch information
@lukas-vlcek
lukas-vlcek committed Dec 8, 2014
1 parent 89d3241 commit 1874efa
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 18 deletions.
31 changes: 18 additions & 13 deletions src/main/java/org/elasticsearch/index/query/TemplateQueryParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,9 @@
package org.elasticsearch.index.query;

import org.apache.lucene.search.Query;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.logging.ESLogger;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down Expand Up @@ -113,17 +110,25 @@ public static TemplateContext parse(XContentParser parser, String paramsFieldnam
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (parameterMap.containsKey(currentFieldName)) {
type = parameterMap.get(currentFieldName);
if (token == XContentParser.Token.START_OBJECT) {
XContentBuilder builder = XContentBuilder.builder(parser.contentType().xContent());
builder.copyCurrentStructure(parser);
templateNameOrTemplateContent = builder.string();
} else {
templateNameOrTemplateContent = parser.text();
} else {
// template can be expresses as a single string value (see #8393)
boolean isTemplateString = ("template".equals(currentFieldName) && token == XContentParser.Token.VALUE_STRING);
if (parameterMap.containsKey(currentFieldName) || isTemplateString) {
if (isTemplateString) {
type = ScriptService.ScriptType.INLINE;
} else {
type = parameterMap.get(currentFieldName);
}
if (token == XContentParser.Token.START_OBJECT) {
XContentBuilder builder = XContentBuilder.builder(parser.contentType().xContent());
builder.copyCurrentStructure(parser);
templateNameOrTemplateContent = builder.string();
} else {
templateNameOrTemplateContent = parser.text();
}
} else if (paramsFieldname.equals(currentFieldName)) {
params = parser.map();
}
} else if (paramsFieldname.equals(currentFieldName)) {
params = parser.map();
}
}

Expand Down
28 changes: 23 additions & 5 deletions src/main/java/org/elasticsearch/search/SearchService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.carrotsearch.hppc.ObjectOpenHashSet;
import com.carrotsearch.hppc.ObjectSet;
import com.carrotsearch.hppc.cursors.ObjectCursor;
import com.fasterxml.jackson.core.JsonParseException;
import com.google.common.base.Charsets;
import com.google.common.collect.ImmutableMap;
import org.apache.lucene.index.AtomicReaderContext;
import org.apache.lucene.index.NumericDocValues;
Expand Down Expand Up @@ -618,11 +620,27 @@ private void parseTemplate(ShardSearchRequest request) {

if (templateContext.scriptType().equals(ScriptService.ScriptType.INLINE)) {
//Try to double parse for nested template id/file
parser = XContentFactory.xContent(templateContext.template().getBytes(Charset.defaultCharset())).createParser(templateContext.template().getBytes(Charset.defaultCharset()));
TemplateQueryParser.TemplateContext innerContext = TemplateQueryParser.parse(parser, "params");
if (hasLength(innerContext.template()) && !innerContext.scriptType().equals(ScriptService.ScriptType.INLINE)) {
//An inner template referring to a filename or id
templateContext = new TemplateQueryParser.TemplateContext(innerContext.scriptType(), innerContext.template(), templateContext.params());
parser = null;
try {
parser = XContentFactory.xContent(templateContext.template().getBytes(Charset.defaultCharset())).createParser(templateContext.template().getBytes(Charset.defaultCharset()));
} catch (ElasticsearchParseException epe) {
//This was an non-nested template, the parse failure was due to this, it is safe to assume this refers to a file
//for backwards compatibility and keep going
templateContext = new TemplateQueryParser.TemplateContext(ScriptService.ScriptType.FILE, templateContext.template(), templateContext.params());
}
if (parser != null) {
try {
TemplateQueryParser.TemplateContext innerContext = TemplateQueryParser.parse(parser, "params");
if (hasLength(innerContext.template()) && !innerContext.scriptType().equals(ScriptService.ScriptType.INLINE)) {
//An inner template referring to a filename or id
templateContext = new TemplateQueryParser.TemplateContext(innerContext.scriptType(), innerContext.template(), templateContext.params());
}
} catch (JsonParseException e) { // TODO: consider throwing better exception, dependency on Jackson exception is not ideal
// If template is provided as a single string then it can contain conditional clauses.
// In such case the parsing is expected to fail, it is ok to ignore the exception here because
// the string is going to be processes by scripting engine and if it is invalid then
// relevant exception will be fired by scripting engine later.
}
}
}
} catch (IOException e) {
Expand Down
68 changes: 68 additions & 0 deletions src/test/java/org/elasticsearch/index/query/TemplateQueryParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,74 @@ public void testParser() throws IOException {
assertTrue("Parsing template query failed.", query instanceof ConstantScoreQuery);
}

/**
* Test that the template query parser can parse and evaluate template expressed as a single string
* with conditional clause.
*/
@Test
public void testParseTemplateAsSingleStringWithConditionalClause() throws IOException {
String templateString = "{" +
" \"template\" : \"{ \\\"match_{{#use_it}}{{template}}{{/use_it}}\\\":{} }\"," +
" \"params\":{" +
" \"template\":\"all\"," +
" \"use_it\": true" +
" }" +
"}";

XContentParser templateSourceParser = XContentFactory.xContent(templateString).createParser(templateString);
context.reset(templateSourceParser);

TemplateQueryParser parser = injector.getInstance(TemplateQueryParser.class);
Query query = parser.parse(context);
assertTrue("Parsing template query failed.", query instanceof ConstantScoreQuery);
}

/**
* Test that the template query parser can parse and evaluate template expressed as a single string
* but still it expects only the query specification (thus this test should fail with specific exception).
*/
@Test(expected = QueryParsingException.class)
public void testParseTemplateFailsToParseCompleteQueryAsSingleString() throws IOException {
String templateString = "{" +
" \"template\" : \"{ \\\"size\\\": \\\"{{size}}\\\", \\\"query\\\":{\\\"match_all\\\":{}}}\"," +
" \"params\":{" +
" \"size\":2" +
" }\n" +
"}";

XContentParser templateSourceParser = XContentFactory.xContent(templateString).createParser(templateString);
context.reset(templateSourceParser);

TemplateQueryParser parser = injector.getInstance(TemplateQueryParser.class);
parser.parse(context);
}

/**
* Test that the template query parser can parse and evaluate template expressed as a single string
* but still it expects only the query specification (thus this test should fail with specific exception).
*
* Compared to {@link #testParseTemplateFailsToParseCompleteQueryAsSingleString} this test verify
* that it will fail correctly even if the query specification is wrapped by the "query: { ... }".
* The point is that if it were to accept such input then it could silently ignore additional content
* such as "size: X, from: Y" following the "query" without letting user know leading to hard-to-spot issues.
* In fact this logic can be also sensitive to internal implementation of parser.
*/
@Test(expected = QueryParsingException.class)
public void testParseTemplateFailsToParseCompleteQueryAsSingleString2() throws IOException {
String templateString = "{" +
" \"template\" : \"{ \\\"query\\\":{\\\"match_{{template}}\\\":{}}}\"," +
" \"params\":{" +
" \"template\":\"all\"" +
" }" +
"}";

XContentParser templateSourceParser = XContentFactory.xContent(templateString).createParser(templateString);
context.reset(templateSourceParser);

TemplateQueryParser parser = injector.getInstance(TemplateQueryParser.class);
parser.parse(context);
}

@Test
public void testParserCanExtractTemplateNames() throws Exception {
String templateString = "{ \"template\": { \"file\": \"storedTemplate\" ,\"params\":{\"template\":\"all\" } } } ";
Expand Down
78 changes: 78 additions & 0 deletions src/test/java/org/elasticsearch/index/query/TemplateQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,84 @@ public void testSearchRequestFail() throws Exception {
assertThat(searchResponse.getHits().hits().length, equalTo(1));
}


@Test
public void testSearchTemplateQueryFromFile() throws Exception {
SearchRequest searchRequest = new SearchRequest();
searchRequest.indices("_all");
String templateString = "{" +
" \"template\" : { \"file\": \"full-query-template\" }," +
" \"params\":{" +
" \"mySize\": 2," +
" \"myField\": \"text\"," +
" \"myValue\": \"value1\"" +
" }" +
"}";
BytesReference bytesRef = new BytesArray(templateString);
searchRequest.templateSource(bytesRef, false);
SearchResponse searchResponse = client().search(searchRequest).get();
assertThat(searchResponse.getHits().hits().length, equalTo(1));
}

/**
* Test that template can be expressed as a single escaped string.
*/
@Test
public void testTemplateQueryAsEscapedString() throws Exception {
SearchRequest searchRequest = new SearchRequest();
searchRequest.indices("_all");
String templateString = "{" +
" \"template\" : \"{ \\\"size\\\": \\\"{{size}}\\\", \\\"query\\\":{\\\"match_all\\\":{}}}\"," +
" \"params\":{" +
" \"size\": 1" +
" }" +
"}";
BytesReference bytesRef = new BytesArray(templateString);
searchRequest.templateSource(bytesRef, false);
SearchResponse searchResponse = client().search(searchRequest).get();
assertThat(searchResponse.getHits().hits().length, equalTo(1));
}

/**
* Test that template can contain conditional clause. In this case it is at the beginning of the string.
*/
@Test
public void testTemplateQueryAsEscapedStringStartingWithConditionalClause() throws Exception {
SearchRequest searchRequest = new SearchRequest();
searchRequest.indices("_all");
String templateString = "{" +
" \"template\" : \"{ {{#use_size}} \\\"size\\\": \\\"{{size}}\\\", {{/use_size}} \\\"query\\\":{\\\"match_all\\\":{}}}\"," +
" \"params\":{" +
" \"size\": 1," +
" \"use_size\": true" +
" }" +
"}";
BytesReference bytesRef = new BytesArray(templateString);
searchRequest.templateSource(bytesRef, false);
SearchResponse searchResponse = client().search(searchRequest).get();
assertThat(searchResponse.getHits().hits().length, equalTo(1));
}

/**
* Test that template can contain conditional clause. In this case it is at the end of the string.
*/
@Test
public void testTemplateQueryAsEscapedStringWithConditionalClauseAtEnd() throws Exception {
SearchRequest searchRequest = new SearchRequest();
searchRequest.indices("_all");
String templateString = "{" +
" \"template\" : \"{ \\\"query\\\":{\\\"match_all\\\":{}} {{#use_size}}, \\\"size\\\": \\\"{{size}}\\\" {{/use_size}} }\"," +
" \"params\":{" +
" \"size\": 1," +
" \"use_size\": true" +
" }" +
"}";
BytesReference bytesRef = new BytesArray(templateString);
searchRequest.templateSource(bytesRef, false);
SearchResponse searchResponse = client().search(searchRequest).get();
assertThat(searchResponse.getHits().hits().length, equalTo(1));
}

@Test
public void testThatParametersCanBeSet() throws Exception {
index("test", "type", "1", jsonBuilder().startObject().field("theField", "foo").endObject());
Expand Down

0 comments on commit 1874efa

Please sign in to comment.