From 19e1d12c3959193942c7d9c46eae125bb1f00101 Mon Sep 17 00:00:00 2001 From: liketic Date: Sat, 30 Sep 2017 09:26:35 +0800 Subject: [PATCH 1/2] Add support for parsing inline script (#23824) --- .../ingest/ConfigurationUtils.java | 24 +++++-- .../org/elasticsearch/ingest/Pipeline.java | 4 +- .../ingest/ConfigurationUtilsTests.java | 27 +++++++- .../ingest/common/ScriptProcessor.java | 69 ++++--------------- .../common/ScriptProcessorFactoryTests.java | 8 +-- .../50_script_processor_using_painless.yml | 1 - 6 files changed, 65 insertions(+), 68 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java b/core/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java index e2f67bf79c98b..78dc0ec6bfef1 100644 --- a/core/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java +++ b/core/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -294,13 +295,13 @@ public static ElasticsearchException newConfigurationException(String processorT return exception; } - public static List readProcessorConfigs(List>> processorConfigs, + public static List readProcessorConfigs(List> processorConfigs, Map processorFactories) throws Exception { Exception exception = null; List processors = new ArrayList<>(); if (processorConfigs != null) { - for (Map> processorConfigWithKey : processorConfigs) { - for (Map.Entry> entry : processorConfigWithKey.entrySet()) { + for (Map processorConfigWithKey : processorConfigs) { + for (Map.Entry entry : processorConfigWithKey.entrySet()) { try { processors.add(readProcessor(processorFactories, entry.getKey(), entry.getValue())); } catch (Exception e) { @@ -353,13 +354,28 @@ private static void addHeadersToException(ElasticsearchException exception, Stri } } + @SuppressWarnings("unchecked") + public static Processor readProcessor(Map processorFactories, + String type, Object config) throws Exception { + if (config instanceof Map) { + return readProcessor(processorFactories, type, (Map) config); + } else if (config instanceof String && "script".equals(type)) { + Map normalizedScript = new HashMap<>(1); + normalizedScript.put(ScriptType.INLINE.getName(), config); + return readProcessor(processorFactories, type, normalizedScript); + } else { + throw newConfigurationException(type, null, null, + "property isn't a map, but of type [" + config.getClass().getName() + "]"); + } + } + public static Processor readProcessor(Map processorFactories, String type, Map config) throws Exception { String tag = ConfigurationUtils.readOptionalStringProperty(null, null, config, TAG_KEY); Processor.Factory factory = processorFactories.get(type); if (factory != null) { boolean ignoreFailure = ConfigurationUtils.readBooleanProperty(null, null, config, "ignore_failure", false); - List>> onFailureProcessorConfigs = + List> onFailureProcessorConfigs = ConfigurationUtils.readOptionalList(null, null, config, Pipeline.ON_FAILURE_KEY); List onFailureProcessors = readProcessorConfigs(onFailureProcessorConfigs, processorFactories); diff --git a/core/src/main/java/org/elasticsearch/ingest/Pipeline.java b/core/src/main/java/org/elasticsearch/ingest/Pipeline.java index 4a705c43bac8d..473b555c05d22 100644 --- a/core/src/main/java/org/elasticsearch/ingest/Pipeline.java +++ b/core/src/main/java/org/elasticsearch/ingest/Pipeline.java @@ -118,9 +118,9 @@ public static final class Factory { public Pipeline create(String id, Map config, Map processorFactories) throws Exception { String description = ConfigurationUtils.readOptionalStringProperty(null, null, config, DESCRIPTION_KEY); Integer version = ConfigurationUtils.readIntProperty(null, null, config, VERSION_KEY, null); - List>> processorConfigs = ConfigurationUtils.readList(null, null, config, PROCESSORS_KEY); + List> processorConfigs = ConfigurationUtils.readList(null, null, config, PROCESSORS_KEY); List processors = ConfigurationUtils.readProcessorConfigs(processorConfigs, processorFactories); - List>> onFailureProcessorConfigs = + List> onFailureProcessorConfigs = ConfigurationUtils.readOptionalList(null, null, config, ON_FAILURE_KEY); List onFailureProcessors = ConfigurationUtils.readProcessorConfigs(onFailureProcessorConfigs, processorFactories); if (config.isEmpty() == false) { diff --git a/core/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java b/core/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java index 6817455f7a695..af863410f9f35 100644 --- a/core/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java +++ b/core/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java @@ -115,7 +115,7 @@ public void testReadProcessors() throws Exception { Map registry = Collections.singletonMap("test_processor", (factories, tag, config) -> processor); - List>> config = new ArrayList<>(); + List> config = new ArrayList<>(); Map emptyConfig = Collections.emptyMap(); config.add(Collections.singletonMap("test_processor", emptyConfig)); config.add(Collections.singletonMap("test_processor", emptyConfig)); @@ -135,7 +135,7 @@ public void testReadProcessors() throws Exception { assertThat(e.getHeader("processor_type"), equalTo(Collections.singletonList("unknown_processor"))); assertThat(e.getHeader("property_name"), is(nullValue())); - List>> config2 = new ArrayList<>(); + List> config2 = new ArrayList<>(); unknownTaggedConfig = new HashMap<>(); unknownTaggedConfig.put("tag", "my_unknown"); config2.add(Collections.singletonMap("unknown_processor", unknownTaggedConfig)); @@ -157,4 +157,27 @@ public void testReadProcessors() throws Exception { assertThat(e2.getHeader("property_name"), is(nullValue())); } + public void testReadProcessorFromObjectOrMap() throws Exception { + Processor processor = mock(Processor.class); + Map registry = + Collections.singletonMap("script", (processorFactories, tag, config) -> { + config.clear(); + return processor; + }); + + Object emptyConfig = Collections.emptyMap(); + Processor processor1 = ConfigurationUtils.readProcessor(registry, "script", emptyConfig); + assertThat(processor1, sameInstance(processor)); + + Object inlineScript = "test_script"; + Processor processor2 = ConfigurationUtils.readProcessor(registry, "script", inlineScript); + assertThat(processor2, sameInstance(processor)); + + Object invalidConfig = 12L; + + ElasticsearchParseException ex = expectThrows(ElasticsearchParseException.class, + () -> ConfigurationUtils.readProcessor(registry, "unknown_processor", invalidConfig)); + assertThat(ex.getMessage(), equalTo("property isn't a map, but of type [" + invalidConfig.getClass().getName() + "]")); + } + } diff --git a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java index ad574115208da..bc8aa6b04a950 100644 --- a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java +++ b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java @@ -19,10 +19,12 @@ package org.elasticsearch.ingest.common; -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.Strings; -import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.logging.ESLoggerFactory; +import com.fasterxml.jackson.core.JsonFactory; + +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.common.xcontent.json.JsonXContentParser; import org.elasticsearch.ingest.AbstractProcessor; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.Processor; @@ -31,15 +33,10 @@ import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.ScriptService; +import java.util.Arrays; import java.util.Map; -import static java.util.Collections.emptyMap; -import static org.elasticsearch.common.Strings.hasLength; import static org.elasticsearch.ingest.ConfigurationUtils.newConfigurationException; -import static org.elasticsearch.ingest.ConfigurationUtils.readOptionalMap; -import static org.elasticsearch.ingest.ConfigurationUtils.readOptionalStringProperty; -import static org.elasticsearch.script.ScriptType.INLINE; -import static org.elasticsearch.script.ScriptType.STORED; /** * Processor that evaluates a script with an ingest document in its context. @@ -47,6 +44,7 @@ public final class ScriptProcessor extends AbstractProcessor { public static final String TYPE = "script"; + private static final JsonFactory JSON_FACTORY = new JsonFactory(); private final Script script; private final ScriptService scriptService; @@ -87,9 +85,6 @@ Script getScript() { } public static final class Factory implements Processor.Factory { - private final Logger logger = ESLoggerFactory.getLogger(Factory.class); - private final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); - private final ScriptService scriptService; public Factory(ScriptService scriptService) { @@ -97,56 +92,20 @@ public Factory(ScriptService scriptService) { } @Override - @SuppressWarnings("unchecked") public ScriptProcessor create(Map registry, String processorTag, Map config) throws Exception { - String lang = readOptionalStringProperty(TYPE, processorTag, config, "lang"); - String source = readOptionalStringProperty(TYPE, processorTag, config, "source"); - String id = readOptionalStringProperty(TYPE, processorTag, config, "id"); - Map params = readOptionalMap(TYPE, processorTag, config, "params"); - - if (source == null) { - source = readOptionalStringProperty(TYPE, processorTag, config, "inline"); - if (source != null) { - deprecationLogger.deprecated("Specifying script source with [inline] is deprecated, use [source] instead."); - } - } - - boolean containsNoScript = !hasLength(id) && !hasLength(source); - if (containsNoScript) { - throw newConfigurationException(TYPE, processorTag, null, "Need [id] or [source] parameter to refer to scripts"); - } - - boolean moreThanOneConfigured = Strings.hasLength(id) && Strings.hasLength(source); - if (moreThanOneConfigured) { - throw newConfigurationException(TYPE, processorTag, null, "Only one of [id] or [source] may be configured"); - } - - if (lang == null) { - lang = Script.DEFAULT_SCRIPT_LANG; - } + XContentBuilder builder = XContentBuilder.builder(JsonXContent.jsonXContent).map(config); + JsonXContentParser parser = new JsonXContentParser(NamedXContentRegistry.EMPTY, + JSON_FACTORY.createParser(builder.bytes().streamInput())); + Script script = Script.parse(parser); - if (params == null) { - params = emptyMap(); - } - - final Script script; - String scriptPropertyUsed; - if (Strings.hasLength(source)) { - script = new Script(INLINE, lang, source, (Map)params); - scriptPropertyUsed = "source"; - } else if (Strings.hasLength(id)) { - script = new Script(STORED, null, id, (Map)params); - scriptPropertyUsed = "id"; - } else { - throw newConfigurationException(TYPE, processorTag, null, "Could not initialize script"); - } + Arrays.asList("id", "source", "inline", "lang", "params", "options").forEach(config::remove); // verify script is able to be compiled before successfully creating processor. try { scriptService.compile(script, ExecutableScript.INGEST_CONTEXT); } catch (ScriptException e) { - throw newConfigurationException(TYPE, processorTag, scriptPropertyUsed, e); + throw newConfigurationException(TYPE, processorTag, null, e); } return new ScriptProcessor(processorTag, script, scriptService); diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java index 07467f9f98825..e5214a2ac47a5 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java @@ -82,17 +82,17 @@ public void testFactoryValidationForMultipleScriptingTypes() throws Exception { ElasticsearchException exception = expectThrows(ElasticsearchException.class, () -> factory.create(null, randomAlphaOfLength(10), configMap)); - assertThat(exception.getMessage(), is("Only one of [id] or [source] may be configured")); + assertThat(exception.getMessage(), is("[script] failed to parse field [source]")); } public void testFactoryValidationAtLeastOneScriptingType() throws Exception { Map configMap = new HashMap<>(); configMap.put("lang", "mockscript"); - ElasticsearchException exception = expectThrows(ElasticsearchException.class, + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> factory.create(null, randomAlphaOfLength(10), configMap)); - assertThat(exception.getMessage(), is("Need [id] or [source] parameter to refer to scripts")); + assertThat(exception.getMessage(), is("must specify either [source] for an inline script or [id] for a stored script")); } public void testInlineBackcompat() throws Exception { @@ -100,7 +100,7 @@ public void testInlineBackcompat() throws Exception { configMap.put("inline", "code"); factory.create(null, randomAlphaOfLength(10), configMap); - assertWarnings("Specifying script source with [inline] is deprecated, use [source] instead."); + assertWarnings("Deprecated field [inline] used, expected [source] instead"); } public void testFactoryInvalidateWithInvalidCompiledScript() throws Exception { diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml index d89395d4d1939..8c6a94b4a5c49 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml @@ -104,7 +104,6 @@ ] } - match: { error.header.processor_type: "script" } - - match: { error.header.property_name: "source" } - match: { error.type: "script_exception" } - match: { error.reason: "compile error" } From 9a8822c9a64d4aa44199b7b941382d707f8f6caa Mon Sep 17 00:00:00 2001 From: liketic Date: Sun, 1 Oct 2017 12:20:30 +0800 Subject: [PATCH 2/2] Fix test --- .../elasticsearch/ingest/common/ScriptProcessorFactoryTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java index e5214a2ac47a5..f1a7add303f1b 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java @@ -112,7 +112,6 @@ public void testFactoryInvalidateWithInvalidCompiledScript() throws Exception { factory = new ScriptProcessor.Factory(mockedScriptService); Map configMap = new HashMap<>(); - configMap.put("lang", "mockscript"); configMap.put(randomType, "my_script"); ElasticsearchException exception = expectThrows(ElasticsearchException.class,