From e0ebf5da1ccbc108a2b5ed1970f5a3ec3b141aca Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 13 Jul 2016 22:28:18 +0200 Subject: [PATCH] Template cleanup: * Removed `Template` class and unified script & template parsing logic. Templates are scripts, so they should be defined as a script. Unless there will be separate template infrastructure, templates should share as much code as possible with scripts. * Removed ScriptParseException in favour for ElasticsearchParseException * Moved TemplateQueryBuilder to lang-mustache module because this query is hard coded to work with mustache only --- .../elasticsearch/ElasticsearchException.java | 3 +- .../index/query/QueryBuilders.java | 22 -- .../script/AbstractScriptParser.java | 184 ------------- .../java/org/elasticsearch/script/Script.java | 249 +++++++++--------- .../org/elasticsearch/script/Template.java | 174 ------------ .../elasticsearch/search/SearchModule.java | 2 - .../phrase/PhraseSuggestionBuilder.java | 15 +- .../ExceptionSerializationTests.java | 2 +- .../query/HasChildQueryBuilderTests.java | 3 +- .../query/HasParentQueryBuilderTests.java | 5 +- .../index/query/NestedQueryBuilderTests.java | 2 +- .../query/QueryDSLDocumentationTests.java | 8 - .../search/SearchModuleTests.java | 1 - .../phrase/PhraseSuggestionBuilderTests.java | 6 +- .../query-dsl/template-query.asciidoc | 34 ++- .../migration/migrate_5_0/scripting.asciidoc | 21 ++ .../ingest/common/ScriptProcessorTests.java | 2 +- modules/lang-mustache/build.gradle | 1 + .../RestDeleteSearchTemplateAction.java | 6 +- .../template/RestGetSearchTemplateAction.java | 4 +- .../template/RestPutSearchTemplateAction.java | 4 +- .../script/mustache/MustachePlugin.java | 5 + .../mustache}/TemplateQueryBuilder.java | 36 ++- .../messy/tests/TemplateQueryParserTests.java | 2 +- .../messy/tests/TemplateQueryTests.java | 27 +- .../mustache}/TemplateQueryBuilderTests.java | 56 ++-- .../PercolateQueryBuilderTests.java | 2 +- .../reindex/RestUpdateByQueryAction.java | 61 ++++- .../test/AbstractQueryTestCase.java | 3 +- 29 files changed, 336 insertions(+), 604 deletions(-) delete mode 100644 core/src/main/java/org/elasticsearch/script/AbstractScriptParser.java delete mode 100644 core/src/main/java/org/elasticsearch/script/Template.java rename {core/src/main/java/org/elasticsearch/index/query => modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache}/TemplateQueryBuilder.java (78%) rename {core/src/test/java/org/elasticsearch/index/query => modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache}/TemplateQueryBuilderTests.java (76%) diff --git a/core/src/main/java/org/elasticsearch/ElasticsearchException.java b/core/src/main/java/org/elasticsearch/ElasticsearchException.java index 95d83bf3a9e..8222955c60b 100644 --- a/core/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/core/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -658,8 +658,7 @@ public class ElasticsearchException extends RuntimeException implements ToXConte org.elasticsearch.search.aggregations.InvalidAggregationPathException::new, 121), INDEX_ALREADY_EXISTS_EXCEPTION(org.elasticsearch.indices.IndexAlreadyExistsException.class, org.elasticsearch.indices.IndexAlreadyExistsException::new, 123), - SCRIPT_PARSE_EXCEPTION(org.elasticsearch.script.Script.ScriptParseException.class, - org.elasticsearch.script.Script.ScriptParseException::new, 124), + // 124 used to be Script.ScriptParseException HTTP_ON_TRANSPORT_EXCEPTION(TcpTransport.HttpOnTransportException.class, TcpTransport.HttpOnTransportException::new, 125), MAPPER_PARSING_EXCEPTION(org.elasticsearch.index.mapper.MapperParsingException.class, diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java index 96585ce810f..d63696ec4fb 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java @@ -30,7 +30,6 @@ import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilder; import org.elasticsearch.indices.TermsLookup; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; -import org.elasticsearch.script.Template; import java.io.IOException; import java.util.Collection; @@ -621,27 +620,6 @@ public abstract class QueryBuilders { return new WrapperQueryBuilder(source); } - /** - * Facilitates creating template query requests using an inline script - */ - public static TemplateQueryBuilder templateQuery(Template template) { - return new TemplateQueryBuilder(template); - } - - /** - * Facilitates creating template query requests using an inline script - */ - public static TemplateQueryBuilder templateQuery(String template, Map vars) { - return new TemplateQueryBuilder(new Template(template, ScriptService.ScriptType.INLINE, null, null, vars)); - } - - /** - * Facilitates creating template query requests - */ - public static TemplateQueryBuilder templateQuery(String template, ScriptService.ScriptType templateType, Map vars) { - return new TemplateQueryBuilder(new Template(template, templateType, null, null, vars)); - } - /** * A filter based on doc/mapping type. */ diff --git a/core/src/main/java/org/elasticsearch/script/AbstractScriptParser.java b/core/src/main/java/org/elasticsearch/script/AbstractScriptParser.java deleted file mode 100644 index d3f9e0c6098..00000000000 --- a/core/src/main/java/org/elasticsearch/script/AbstractScriptParser.java +++ /dev/null @@ -1,184 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you 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 org.elasticsearch.script; - -import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.script.Script.ScriptField; -import org.elasticsearch.script.Script.ScriptParseException; -import org.elasticsearch.script.ScriptService.ScriptType; - -import java.io.IOException; -import java.util.Iterator; -import java.util.Map; -import java.util.Map.Entry; - -public abstract class AbstractScriptParser { - - public abstract String parseInlineScript(XContentParser parser) throws IOException; - - protected abstract S createScript(String script, ScriptType type, String lang, Map params); - - protected abstract S createSimpleScript(XContentParser parser) throws IOException; - - public S parse(XContentParser parser, ParseFieldMatcher parseFieldMatcher) throws IOException { - - XContentParser.Token token = parser.currentToken(); - // If the parser hasn't yet been pushed to the first token, do it now - if (token == null) { - token = parser.nextToken(); - } - - if (token == XContentParser.Token.VALUE_STRING) { - return createSimpleScript(parser); - } - if (token != XContentParser.Token.START_OBJECT) { - throw new ScriptParseException("expected a string value or an object, but found [{}] instead", token); - } - - String script = null; - ScriptType type = null; - String lang = getDefaultScriptLang(); - Map params = null; - - String currentFieldName = null; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (parseFieldMatcher.match(currentFieldName, ScriptType.INLINE.getParseField())) { - type = ScriptType.INLINE; - script = parseInlineScript(parser); - } else if (parseFieldMatcher.match(currentFieldName, ScriptType.FILE.getParseField())) { - type = ScriptType.FILE; - if (token == XContentParser.Token.VALUE_STRING) { - script = parser.text(); - } else { - throw new ScriptParseException("expected a string value for field [{}], but found [{}]", currentFieldName, token); - } - } else if (parseFieldMatcher.match(currentFieldName, ScriptType.STORED.getParseField())) { - type = ScriptType.STORED; - if (token == XContentParser.Token.VALUE_STRING) { - script = parser.text(); - } else { - throw new ScriptParseException("expected a string value for field [{}], but found [{}]", currentFieldName, token); - } - } else if (parseFieldMatcher.match(currentFieldName, ScriptField.LANG)) { - if (token == XContentParser.Token.VALUE_STRING) { - lang = parser.text(); - } else { - throw new ScriptParseException("expected a string value for field [{}], but found [{}]", currentFieldName, token); - } - } else if (parseFieldMatcher.match(currentFieldName, ScriptField.PARAMS)) { - if (token == XContentParser.Token.START_OBJECT) { - params = parser.map(); - } else { - throw new ScriptParseException("expected an object for field [{}], but found [{}]", currentFieldName, token); - } - } else { - throw new ScriptParseException("unexpected field [{}]", currentFieldName); - } - } - if (script == null) { - throw new ScriptParseException("expected one of [{}], [{}] or [{}] fields, but found none", ScriptType.INLINE.getParseField() - .getPreferredName(), ScriptType.FILE.getParseField().getPreferredName(), ScriptType.STORED.getParseField() - .getPreferredName()); - } - assert type != null : "if script is not null, type should definitely not be null"; - return createScript(script, type, lang, params); - - } - - /** - * @return the default script language for this parser or null - * to use the default set in the ScriptService - */ - protected String getDefaultScriptLang() { - return null; - } - - public S parse(Map config, boolean removeMatchedEntries, ParseFieldMatcher parseFieldMatcher) { - String script = null; - ScriptType type = null; - String lang = null; - Map params = null; - for (Iterator> itr = config.entrySet().iterator(); itr.hasNext();) { - Entry entry = itr.next(); - String parameterName = entry.getKey(); - Object parameterValue = entry.getValue(); - if (parseFieldMatcher.match(parameterName, ScriptField.LANG)) { - if (parameterValue instanceof String || parameterValue == null) { - lang = (String) parameterValue; - if (removeMatchedEntries) { - itr.remove(); - } - } else { - throw new ScriptParseException("Value must be of type String: [" + parameterName + "]"); - } - } else if (parseFieldMatcher.match(parameterName, ScriptField.PARAMS)) { - if (parameterValue instanceof Map || parameterValue == null) { - params = (Map) parameterValue; - if (removeMatchedEntries) { - itr.remove(); - } - } else { - throw new ScriptParseException("Value must be of type String: [" + parameterName + "]"); - } - } else if (parseFieldMatcher.match(parameterName, ScriptType.INLINE.getParseField())) { - if (parameterValue instanceof String || parameterValue == null) { - script = (String) parameterValue; - type = ScriptType.INLINE; - if (removeMatchedEntries) { - itr.remove(); - } - } else { - throw new ScriptParseException("Value must be of type String: [" + parameterName + "]"); - } - } else if (parseFieldMatcher.match(parameterName, ScriptType.FILE.getParseField())) { - if (parameterValue instanceof String || parameterValue == null) { - script = (String) parameterValue; - type = ScriptType.FILE; - if (removeMatchedEntries) { - itr.remove(); - } - } else { - throw new ScriptParseException("Value must be of type String: [" + parameterName + "]"); - } - } else if (parseFieldMatcher.match(parameterName, ScriptType.STORED.getParseField())) { - if (parameterValue instanceof String || parameterValue == null) { - script = (String) parameterValue; - type = ScriptType.STORED; - if (removeMatchedEntries) { - itr.remove(); - } - } else { - throw new ScriptParseException("Value must be of type String: [" + parameterName + "]"); - } - } - } - if (script == null) { - throw new ScriptParseException("expected one of [{}], [{}] or [{}] fields, but found none", ScriptType.INLINE.getParseField() - .getPreferredName(), ScriptType.FILE.getParseField().getPreferredName(), ScriptType.STORED.getParseField() - .getPreferredName()); - } - assert type != null : "if script is not null, type should definitely not be null"; - return createScript(script, type, lang, params); - } - -} diff --git a/core/src/main/java/org/elasticsearch/script/Script.java b/core/src/main/java/org/elasticsearch/script/Script.java index ee9a9712c8e..cedae963ca4 100644 --- a/core/src/main/java/org/elasticsearch/script/Script.java +++ b/core/src/main/java/org/elasticsearch/script/Script.java @@ -19,79 +19,75 @@ package org.elasticsearch.script; -import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.logging.LoggerMessageFormat; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.script.ScriptService.ScriptType; import java.io.IOException; import java.util.Map; +import java.util.Objects; /** * Script holds all the parameters necessary to compile or find in cache and then execute a script. */ -public class Script implements ToXContent, Writeable { +public final class Script implements ToXContent, Writeable { public static final ScriptType DEFAULT_TYPE = ScriptType.INLINE; - private static final ScriptParser PARSER = new ScriptParser(); private String script; - @Nullable private ScriptType type; + private ScriptType type; @Nullable private String lang; @Nullable private Map params; + @Nullable private XContentType contentType; /** - * Constructor for simple inline script. The script will have no lang or - * params set. + * Constructor for simple inline script. The script will have no lang or params set. * - * @param script - * The inline script to execute. + * @param script The inline script to execute. */ public Script(String script) { - this(script, null); + this(script, ScriptType.INLINE, null, null); } - /** - * For sub-classes to use to override the default language - */ - protected Script(String script, String lang) { - this(script, ScriptType.INLINE, lang, null); + public Script(String script, ScriptType type, @Nullable String lang, @Nullable Map params) { + this(script, type, lang, params, null); } /** * Constructor for Script. * - * @param script - * The cache key of the script to be compiled/executed. For - * inline scripts this is the actual script source code. For - * indexed scripts this is the id used in the request. For on - * file scripts this is the file name. - * @param type - * The type of script -- dynamic, indexed, or file. - * @param lang - * The language of the script to be compiled/executed. - * @param params - * The map of parameters the script will be executed with. + * @param script The cache key of the script to be compiled/executed. For inline scripts this is the actual + * script source code. For indexed scripts this is the id used in the request. For on file + * scripts this is the file name. + * @param type The type of script -- dynamic, stored, or file. + * @param lang The language of the script to be compiled/executed. + * @param params The map of parameters the script will be executed with. + * @param contentType The {@link XContentType} of the script. Only relevant for inline scripts that have not been + * defined as a plain string, but as json or yaml content. This class needs this information + * when serializing the script back to xcontent. */ - public Script(String script, ScriptType type, @Nullable String lang, @Nullable Map params) { - if (script == null) { - throw new IllegalArgumentException("The parameter script (String) must not be null in Script."); + @SuppressWarnings("unchecked") + public Script(String script, ScriptType type, @Nullable String lang, @Nullable Map params, + @Nullable XContentType contentType) { + if (contentType != null && type != ScriptType.INLINE) { + throw new IllegalArgumentException("The parameter contentType only makes sense for inline scripts"); } - if (type == null) { - throw new IllegalArgumentException("The parameter type (ScriptType) must not be null in Script."); - } - this.script = script; - this.type = type; + this.script = Objects.requireNonNull(script); + this.type = Objects.requireNonNull(type); this.lang = lang; - this.params = (Map)params; + this.params = (Map) params; + this.contentType = contentType; } public Script(StreamInput in) throws IOException { @@ -100,13 +96,14 @@ public class Script implements ToXContent, Writeable { type = ScriptType.readFrom(in); } lang = in.readOptionalString(); + params = in.readMap(); if (in.readBoolean()) { - params = in.readMap(); + contentType = XContentType.readFrom(in); } } @Override - public final void writeTo(StreamOutput out) throws IOException { + public void writeTo(StreamOutput out) throws IOException { out.writeString(script); boolean hasType = type != null; out.writeBoolean(hasType); @@ -114,16 +111,14 @@ public class Script implements ToXContent, Writeable { ScriptType.writeTo(type, out); } out.writeOptionalString(lang); - boolean hasParams = params != null; - out.writeBoolean(hasParams); - if (hasParams) { - out.writeMap(params); + out.writeMap(params); + boolean hasContentType = contentType != null; + out.writeBoolean(hasContentType); + if (hasContentType) { + XContentType.writeTo(contentType, out); } - doWriteTo(out); } - protected void doWriteTo(StreamOutput out) throws IOException {}; - /** * Method for getting the script. * @return The cache key of the script to be compiled/executed. For dynamic scripts this is the actual @@ -137,7 +132,7 @@ public class Script implements ToXContent, Writeable { /** * Method for getting the type. * - * @return The type of script -- inline, indexed, or file. + * @return The type of script -- inline, stored, or file. */ public ScriptType getType() { return type == null ? DEFAULT_TYPE : type; @@ -161,14 +156,25 @@ public class Script implements ToXContent, Writeable { return params; } + /** + * @return The content type of the script if it is an inline script and the script has been defined as json + * or yaml content instead of a plain string. + */ + public XContentType getContentType() { + return contentType; + } + @Override - public final XContentBuilder toXContent(XContentBuilder builder, Params builderParams) throws IOException { + public XContentBuilder toXContent(XContentBuilder builder, Params builderParams) throws IOException { if (type == null) { return builder.value(script); } - builder.startObject(); - scriptFieldToXContent(script, type, builder, builderParams); + if (type == ScriptType.INLINE && contentType != null && builder.contentType() == contentType) { + builder.rawField(type.getParseField().getPreferredName(), new BytesArray(script)); + } else { + builder.field(type.getParseField().getPreferredName(), script); + } if (lang != null) { builder.field(ScriptField.LANG.getPreferredName(), lang); } @@ -179,29 +185,80 @@ public class Script implements ToXContent, Writeable { return builder; } - protected XContentBuilder scriptFieldToXContent(String script, ScriptType type, XContentBuilder builder, Params builderParams) - throws IOException { - builder.field(type.getParseField().getPreferredName(), script); - return builder; - } - - public static Script parse(Map config, boolean removeMatchedEntries, ParseFieldMatcher parseFieldMatcher) { - return PARSER.parse(config, removeMatchedEntries, parseFieldMatcher); - } - public static Script parse(XContentParser parser, ParseFieldMatcher parseFieldMatcher) throws IOException { - return PARSER.parse(parser, parseFieldMatcher); + return parse(parser, parseFieldMatcher, null); + } + + public static Script parse(XContentParser parser, ParseFieldMatcher parseFieldMatcher, @Nullable String lang) throws IOException { + XContentParser.Token token = parser.currentToken(); + // If the parser hasn't yet been pushed to the first token, do it now + if (token == null) { + token = parser.nextToken(); + } + if (token == XContentParser.Token.VALUE_STRING) { + return new Script(parser.text()); + } + if (token != XContentParser.Token.START_OBJECT) { + throw new ElasticsearchParseException("expected a string value or an object, but found [{}] instead", token); + } + String script = null; + ScriptType type = null; + Map params = null; + XContentType contentType = null; + String cfn = null; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + cfn = parser.currentName(); + } else if (parseFieldMatcher.match(cfn, ScriptType.INLINE.getParseField())) { + type = ScriptType.INLINE; + if (parser.currentToken() == XContentParser.Token.START_OBJECT) { + contentType = parser.contentType(); + XContentBuilder builder = XContentFactory.contentBuilder(contentType); + script = builder.copyCurrentStructure(parser).bytes().utf8ToString(); + } else { + script = parser.text(); + } + } else if (parseFieldMatcher.match(cfn, ScriptType.FILE.getParseField())) { + type = ScriptType.FILE; + if (token == XContentParser.Token.VALUE_STRING) { + script = parser.text(); + } else { + throw new ElasticsearchParseException("expected a string value for field [{}], but found [{}]", cfn, token); + } + } else if (parseFieldMatcher.match(cfn, ScriptType.STORED.getParseField())) { + type = ScriptType.STORED; + if (token == XContentParser.Token.VALUE_STRING) { + script = parser.text(); + } else { + throw new ElasticsearchParseException("expected a string value for field [{}], but found [{}]", cfn, token); + } + } else if (parseFieldMatcher.match(cfn, ScriptField.LANG)) { + if (token == XContentParser.Token.VALUE_STRING) { + lang = parser.text(); + } else { + throw new ElasticsearchParseException("expected a string value for field [{}], but found [{}]", cfn, token); + } + } else if (parseFieldMatcher.match(cfn, ScriptField.PARAMS)) { + if (token == XContentParser.Token.START_OBJECT) { + params = parser.map(); + } else { + throw new ElasticsearchParseException("expected an object for field [{}], but found [{}]", cfn, token); + } + } else { + throw new ElasticsearchParseException("unexpected field [{}]", cfn); + } + } + if (script == null) { + throw new ElasticsearchParseException("expected one of [{}], [{}] or [{}] fields, but found none", + ScriptType.INLINE.getParseField() .getPreferredName(), ScriptType.FILE.getParseField().getPreferredName(), + ScriptType.STORED.getParseField() .getPreferredName()); + } + return new Script(script, type, lang, params, contentType); } @Override public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((lang == null) ? 0 : lang.hashCode()); - result = prime * result + ((params == null) ? 0 : params.hashCode()); - result = prime * result + ((script == null) ? 0 : script.hashCode()); - result = prime * result + ((type == null) ? 0 : type.hashCode()); - return result; + return Objects.hash(lang, params, script, type, contentType); } @Override @@ -210,52 +267,18 @@ public class Script implements ToXContent, Writeable { if (obj == null) return false; if (getClass() != obj.getClass()) return false; Script other = (Script) obj; - if (lang == null) { - if (other.lang != null) return false; - } else { - if (!lang.equals(other.lang)) return false; - } - if (params == null) { - if (other.params != null) return false; - } else { - if (!params.equals(other.params)) return false; - } - if (script == null) { - if (other.script != null) return false; - } else { - if (!script.equals(other.script)) return false; - } - if (type != other.type) return false; - return true; + + return Objects.equals(lang, other.lang) && + Objects.equals(params, other.params) && + Objects.equals(script, other.script) && + Objects.equals(type, other.type) && + Objects.equals(contentType, other.contentType); } @Override public String toString() { - return "[script: " + script + ", type: " + type.getParseField().getPreferredName() + ", lang: " + lang + ", params: " + params - + "]"; - } - - private static class ScriptParser extends AbstractScriptParser