From 061dc24be9a1e4774461af389f5020698535df2a Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 26 Apr 2018 16:08:39 -0700 Subject: [PATCH 1/3] Deprecate the use of empty templates. Bug fix allows empty templates/scripts to be loaded on start up. --- .../elasticsearch/script/ScriptMetaData.java | 21 ++++++- .../script/StoredScriptSource.java | 61 ++++++++++++++++--- .../script/ScriptMetaDataTests.java | 41 +++++++++++++ .../script/StoredScriptSourceTests.java | 2 +- .../script/StoredScriptTests.java | 36 ++++++++++- 5 files changed, 147 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java b/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java index dca17ce486607..9505875ae1ebc 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java @@ -29,6 +29,8 @@ 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.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -46,6 +48,11 @@ */ public final class ScriptMetaData implements MetaData.Custom, Writeable, ToXContentFragment { + /** + * Standard deprecation logger for used to deprecate allowance of empty templates. + */ + private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ScriptMetaData.class)); + /** * A builder used to modify the currently stored scripts data held within * the {@link ClusterState}. Scripts can be added or deleted, then built @@ -161,8 +168,8 @@ static ScriptMetaData deleteStoredScript(ScriptMetaData previous, String id) { * * {@code * { - * "" : "<{@link StoredScriptSource#fromXContent(XContentParser)}>", - * "" : "<{@link StoredScriptSource#fromXContent(XContentParser)}>", + * "" : "<{@link StoredScriptSource#fromXContent(XContentParser, boolean)}>", + * "" : "<{@link StoredScriptSource#fromXContent(XContentParser, boolean)}>", * ... * } * } @@ -209,6 +216,14 @@ public static ScriptMetaData fromXContent(XContentParser parser) throws IOExcept lang = id.substring(0, split); id = id.substring(split + 1); source = new StoredScriptSource(lang, parser.text(), Collections.emptyMap()); + + if (source.getSource().isEmpty()) { + if (source.getLang().equals(Script.DEFAULT_TEMPLATE_LANG)) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + } else { + DEPRECATION_LOGGER.deprecated("empty scripts should no longer be used"); + } + } } exists = scripts.get(id); @@ -231,7 +246,7 @@ public static ScriptMetaData fromXContent(XContentParser parser) throws IOExcept } exists = scripts.get(id); - source = StoredScriptSource.fromXContent(parser); + source = StoredScriptSource.fromXContent(parser, true); if (exists == null) { scripts.put(id, source); diff --git a/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java b/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java index 9c52ff943d2a1..374ce00ff1fcc 100644 --- a/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java +++ b/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java @@ -32,6 +32,8 @@ 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.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ObjectParser; @@ -57,6 +59,11 @@ */ public class StoredScriptSource extends AbstractDiffable implements Writeable, ToXContentObject { + /** + * Standard deprecation logger for used to deprecate allowance of empty templates. + */ + private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(StoredScriptSource.class)); + /** * Standard {@link ParseField} for outer level of stored script source. */ @@ -109,7 +116,7 @@ private void setLang(String lang) { private void setSource(XContentParser parser) { try { if (parser.currentToken() == Token.START_OBJECT) { - //this is really for search templates, that need to be converted to json format + // this is really for search templates, that need to be converted to json format XContentBuilder builder = XContentFactory.jsonBuilder(); source = Strings.toString(builder.copyCurrentStructure(parser)); options.put(Script.CONTENT_TYPE_OPTION, XContentType.JSON.mediaType()); @@ -131,8 +138,11 @@ private void setOptions(Map options) { /** * Validates the parameters and creates an {@link StoredScriptSource}. + * + * @param ignore Specify as {@code true} to ignore the empty source check. This allows + * empty templates to be loaded for backwards compatibility. */ - private StoredScriptSource build() { + private StoredScriptSource build(boolean ignore) { if (lang == null) { throw new IllegalArgumentException("must specify lang for stored script"); } else if (lang.isEmpty()) { @@ -140,9 +150,25 @@ private StoredScriptSource build() { } if (source == null) { - throw new IllegalArgumentException("must specify source for stored script"); + if (ignore || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + } else { + DEPRECATION_LOGGER.deprecated("empty scripts should no longer be used"); + } + } else { + throw new IllegalArgumentException("must specify source for stored script"); + } } else if (source.isEmpty()) { - throw new IllegalArgumentException("source cannot be empty"); + if (ignore || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + } else { + DEPRECATION_LOGGER.deprecated("empty scripts should no longer be used"); + } + } else { + throw new IllegalArgumentException("source cannot be empty"); + } } if (options.size() > 1 || options.size() == 1 && options.get(Script.CONTENT_TYPE_OPTION) == null) { @@ -257,6 +283,8 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon token = parser.nextToken(); if (token == Token.END_OBJECT) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap()); } @@ -271,7 +299,7 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon token = parser.nextToken(); if (token == Token.START_OBJECT) { - return PARSER.apply(parser, null).build(); + return PARSER.apply(parser, null).build(false); } else { throw new ParsingException(parser.getTokenLocation(), "unexpected token [" + token + "], expected [{, ]"); } @@ -280,7 +308,13 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon token = parser.nextToken(); if (token == Token.VALUE_STRING) { - return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, parser.text(), Collections.emptyMap()); + String source = parser.text(); + + if (source == null || source.isEmpty()) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + } + + return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, source, Collections.emptyMap()); } } @@ -293,7 +327,13 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon builder.copyCurrentStructure(parser); } - return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, Strings.toString(builder), Collections.emptyMap()); + String source = Strings.toString(builder); + + if (source == null || source.isEmpty()) { + DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); + } + + return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, source, Collections.emptyMap()); } } } catch (IOException ioe) { @@ -320,9 +360,12 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon * * Note that the "source" parameter can also handle template parsing including from * a complex JSON object. + * + * @param ignore Specify as {@code true} to ignore the empty source check. This allows + * empty templates to be loaded for backwards compatibility. */ - public static StoredScriptSource fromXContent(XContentParser parser) { - return PARSER.apply(parser, null).build(); + public static StoredScriptSource fromXContent(XContentParser parser, boolean ignore) { + return PARSER.apply(parser, null).build(ignore); } /** diff --git a/server/src/test/java/org/elasticsearch/script/ScriptMetaDataTests.java b/server/src/test/java/org/elasticsearch/script/ScriptMetaDataTests.java index d5769cd192b75..32d4d48a44810 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptMetaDataTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptMetaDataTests.java @@ -22,6 +22,8 @@ import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -130,6 +132,45 @@ public void testBuilder() { assertEquals("1 + 1", result.getStoredScript("_id").getSource()); } + public void testLoadEmptyScripts() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + builder.startObject().field("mustache#empty", "").endObject(); + XContentParser parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + BytesReference.bytes(builder).streamInput()); + ScriptMetaData.fromXContent(parser); + assertWarnings("empty templates should no longer be used"); + + builder = XContentFactory.jsonBuilder(); + builder.startObject().field("lang#empty", "").endObject(); + parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + BytesReference.bytes(builder).streamInput()); + ScriptMetaData.fromXContent(parser); + assertWarnings("empty scripts should no longer be used"); + + builder = XContentFactory.jsonBuilder(); + builder.startObject().startObject("script").field("lang", "lang").field("source", "").endObject().endObject(); + parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + BytesReference.bytes(builder).streamInput()); + ScriptMetaData.fromXContent(parser); + assertWarnings("empty scripts should no longer be used"); + + builder = XContentFactory.jsonBuilder(); + builder.startObject().startObject("script").field("lang", "mustache").field("source", "").endObject().endObject(); + parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + BytesReference.bytes(builder).streamInput()); + ScriptMetaData.fromXContent(parser); + assertWarnings("empty templates should no longer be used"); + } + + @Override + protected boolean enableWarningsCheck() { + return true; + } + private ScriptMetaData randomScriptMetaData(XContentType sourceContentType, int minNumberScripts) throws IOException { ScriptMetaData.Builder builder = new ScriptMetaData.Builder(null); int numScripts = scaledRandomIntBetween(minNumberScripts, 32); diff --git a/server/src/test/java/org/elasticsearch/script/StoredScriptSourceTests.java b/server/src/test/java/org/elasticsearch/script/StoredScriptSourceTests.java index 168ec4fc553b9..8aa4ca57acfed 100644 --- a/server/src/test/java/org/elasticsearch/script/StoredScriptSourceTests.java +++ b/server/src/test/java/org/elasticsearch/script/StoredScriptSourceTests.java @@ -58,7 +58,7 @@ protected StoredScriptSource createTestInstance() { @Override protected StoredScriptSource doParseInstance(XContentParser parser) { - return StoredScriptSource.fromXContent(parser); + return StoredScriptSource.fromXContent(parser, false); } @Override diff --git a/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java b/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java index 2bf0216c546ec..ae811cb9826f9 100644 --- a/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java +++ b/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.AbstractSerializingTestCase; +import java.io.IOException; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -204,6 +205,39 @@ public void testSourceParsingErrors() throws Exception { } } + public void testEmptyTemplateDeprecations() throws IOException { + /*try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { + builder.startObject().endObject(); + + StoredScriptSource parsed = StoredScriptSource.parse(BytesReference.bytes(builder), XContentType.JSON); + StoredScriptSource source = new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap()); + + assertThat(parsed, equalTo(source)); + assertWarnings("empty templates should no longer be used"); + } + + try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { + builder.startObject().field("template", "").endObject(); + + StoredScriptSource parsed = StoredScriptSource.parse(BytesReference.bytes(builder), XContentType.JSON); + StoredScriptSource source = new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap()); + + assertThat(parsed, equalTo(source)); + assertWarnings("empty templates should no longer be used"); + }*/ + + try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { + builder.startObject().field("script").startObject().field("lang", "mustache") + .field("source", "").endObject().endObject(); + + StoredScriptSource parsed = StoredScriptSource.parse(BytesReference.bytes(builder), XContentType.JSON); + StoredScriptSource source = new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap()); + + assertThat(parsed, equalTo(source)); + assertWarnings("empty templates should no longer be used"); + } + } + @Override protected StoredScriptSource createTestInstance() { return new StoredScriptSource( @@ -219,7 +253,7 @@ protected Writeable.Reader instanceReader() { @Override protected StoredScriptSource doParseInstance(XContentParser parser) { - return StoredScriptSource.fromXContent(parser); + return StoredScriptSource.fromXContent(parser, false); } @Override From 396bd6d51ba6c2b9c0bf6e459f4f63baab915b9e Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 26 Apr 2018 16:15:48 -0700 Subject: [PATCH 2/3] Remove commented out tests. --- .../test/java/org/elasticsearch/script/StoredScriptTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java b/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java index ae811cb9826f9..79e3195f3d923 100644 --- a/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java +++ b/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java @@ -206,7 +206,7 @@ public void testSourceParsingErrors() throws Exception { } public void testEmptyTemplateDeprecations() throws IOException { - /*try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { + try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { builder.startObject().endObject(); StoredScriptSource parsed = StoredScriptSource.parse(BytesReference.bytes(builder), XContentType.JSON); @@ -224,7 +224,7 @@ public void testEmptyTemplateDeprecations() throws IOException { assertThat(parsed, equalTo(source)); assertWarnings("empty templates should no longer be used"); - }*/ + } try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { builder.startObject().field("script").startObject().field("lang", "mustache") From a286256759ec7d6f718fea3af706604b7d97c3d1 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Mon, 14 May 2018 08:53:53 -0700 Subject: [PATCH 3/3] Rename ignore to ignoreEmpty. --- .../script/StoredScriptSource.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java b/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java index 374ce00ff1fcc..da6dad1dff384 100644 --- a/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java +++ b/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java @@ -139,10 +139,11 @@ private void setOptions(Map options) { /** * Validates the parameters and creates an {@link StoredScriptSource}. * - * @param ignore Specify as {@code true} to ignore the empty source check. This allows - * empty templates to be loaded for backwards compatibility. + * @param ignoreEmpty Specify as {@code true} to ignoreEmpty the empty source check. + * This allow empty templates to be loaded for backwards compatibility. + * This allow empty templates to be loaded for backwards compatibility. */ - private StoredScriptSource build(boolean ignore) { + private StoredScriptSource build(boolean ignoreEmpty) { if (lang == null) { throw new IllegalArgumentException("must specify lang for stored script"); } else if (lang.isEmpty()) { @@ -150,7 +151,7 @@ private StoredScriptSource build(boolean ignore) { } if (source == null) { - if (ignore || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + if (ignoreEmpty || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); } else { @@ -160,7 +161,7 @@ private StoredScriptSource build(boolean ignore) { throw new IllegalArgumentException("must specify source for stored script"); } } else if (source.isEmpty()) { - if (ignore || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { + if (ignoreEmpty || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) { DEPRECATION_LOGGER.deprecated("empty templates should no longer be used"); } else { @@ -361,11 +362,11 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon * Note that the "source" parameter can also handle template parsing including from * a complex JSON object. * - * @param ignore Specify as {@code true} to ignore the empty source check. This allows - * empty templates to be loaded for backwards compatibility. + * @param ignoreEmpty Specify as {@code true} to ignoreEmpty the empty source check. + * This allows empty templates to be loaded for backwards compatibility. */ - public static StoredScriptSource fromXContent(XContentParser parser, boolean ignore) { - return PARSER.apply(parser, null).build(ignore); + public static StoredScriptSource fromXContent(XContentParser parser, boolean ignoreEmpty) { + return PARSER.apply(parser, null).build(ignoreEmpty); } /**