Backports for _source bug fix in scripting (#57068)

* Update DeprecationMap to DynamicMap (#56149) This renames DeprecationMap to DynamicMap, and changes the deprecation messages Map to accept a Map of String (keys) to Functions (updated values) instead. This creates more flexibility in either logging or updating values from params within a script. This change is required to fix (#52103) in a future PR. * Fix Source Return Bug in Scripting (#56831) This change ensures that when a user returns _source directly no matter where accessed within scripting, the value is a Map of the converted source as opposed to a SourceLookup.
elastic · May 22, 2020 · 35c546b · 35c546b
1 parent 4cf49bc
commit 35c546b
Show file tree

Hide file tree

Showing 12 changed files with 243 additions and 183 deletions.
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
@@ -19,7 +19,9 @@
 
 package org.elasticsearch.ingest.common;
 
+import org.apache.logging.log4j.LogManager;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
@@ -30,17 +32,16 @@
 import org.elasticsearch.ingest.AbstractProcessor;
 import org.elasticsearch.ingest.IngestDocument;
 import org.elasticsearch.ingest.Processor;
-import org.elasticsearch.script.DeprecationMap;
+import org.elasticsearch.script.DynamicMap;
 import org.elasticsearch.script.IngestScript;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.ScriptException;
 import org.elasticsearch.script.ScriptService;
 
 import java.io.InputStream;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.Map;
+import java.util.function.Function;
 
 import static org.elasticsearch.ingest.ConfigurationUtils.newConfigurationException;
 
@@ -49,15 +50,14 @@
  */
 public final class ScriptProcessor extends AbstractProcessor {
 
-    private static final Map<String, String> DEPRECATIONS;
-    static {
-        Map<String, String> deprecations = new HashMap<>();
-        deprecations.put(
-                "_type",
-                "[types removal] Looking up doc types [_type] in scripts is deprecated."
-        );
-        DEPRECATIONS = Collections.unmodifiableMap(deprecations);
-    }
+    private static final DeprecationLogger deprecationLogger =
+            new DeprecationLogger(LogManager.getLogger(DynamicMap.class));
+    private static final Map<String, Function<Object, Object>> PARAMS_FUNCTIONS = org.elasticsearch.common.collect.Map.of(
+            "_type", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("script_processor",
+                        "[types removal] Looking up doc types [_type] in scripts is deprecated.");
+                return value;
+            });
 
     public static final String TYPE = "script";
 
@@ -86,7 +86,7 @@ public final class ScriptProcessor extends AbstractProcessor {
     public IngestDocument execute(IngestDocument document) {
         IngestScript.Factory factory = scriptService.compile(script, IngestScript.CONTEXT);
         factory.newInstance(script.getParams()).execute(
-                new DeprecationMap(document.getSourceAndMetadata(), DEPRECATIONS, "script_processor"));
+                new DynamicMap(document.getSourceAndMetadata(), PARAMS_FUNCTIONS));
         CollectionUtils.ensureNoSelfReferences(document.getSourceAndMetadata(), "ingest script");
         return document;
     }

diff --git a/...ang-painless/src/test/java/org/elasticsearch/painless/ScriptedMetricAggContextsTests.java b/...ang-painless/src/test/java/org/elasticsearch/painless/ScriptedMetricAggContextsTests.java
@@ -91,6 +91,33 @@ public void testMapBasic() throws IOException {
         assertEquals(1.0, state.get("testField"));
     }
 
+    public void testReturnSource() throws IOException {
+        ScriptedMetricAggContexts.MapScript.Factory factory = scriptEngine.compile("test",
+                "state._source = params._source", ScriptedMetricAggContexts.MapScript.CONTEXT, Collections.emptyMap());
+
+        Map<String, Object> params = new HashMap<>();
+        Map<String, Object> state = new HashMap<>();
+
+        MemoryIndex index = new MemoryIndex();
+        // we don't need a real index, just need to construct a LeafReaderContext which cannot be mocked
+        LeafReaderContext leafReaderContext = index.createSearcher().getIndexReader().leaves().get(0);
+
+        SearchLookup lookup = mock(SearchLookup.class);
+        LeafSearchLookup leafLookup = mock(LeafSearchLookup.class);
+        when(lookup.getLeafSearchLookup(leafReaderContext)).thenReturn(leafLookup);
+        SourceLookup sourceLookup = mock(SourceLookup.class);
+        when(leafLookup.asMap()).thenReturn(Collections.singletonMap("_source", sourceLookup));
+        when(sourceLookup.loadSourceIfNeeded()).thenReturn(Collections.singletonMap("test", 1));
+        ScriptedMetricAggContexts.MapScript.LeafFactory leafFactory = factory.newFactory(params, state, lookup);
+        ScriptedMetricAggContexts.MapScript script = leafFactory.newInstance(leafReaderContext);
+
+        script.execute();
+
+        assertTrue(state.containsKey("_source"));
+        assertTrue(state.get("_source") instanceof Map && ((Map)state.get("_source")).containsKey("test"));
+        assertEquals(1, ((Map)state.get("_source")).get("test"));
+    }
+
     public void testMapSourceAccess() throws IOException {
         ScriptedMetricAggContexts.MapScript.Factory factory = scriptEngine.compile("test",
             "state.testField = params._source.three", ScriptedMetricAggContexts.MapScript.CONTEXT, Collections.emptyMap());
@@ -107,13 +134,13 @@ public void testMapSourceAccess() throws IOException {
         when(lookup.getLeafSearchLookup(leafReaderContext)).thenReturn(leafLookup);
         SourceLookup sourceLookup = mock(SourceLookup.class);
         when(leafLookup.asMap()).thenReturn(Collections.singletonMap("_source", sourceLookup));
-        when(sourceLookup.get("three")).thenReturn(3);
+        when(sourceLookup.loadSourceIfNeeded()).thenReturn(Collections.singletonMap("three", 3));
         ScriptedMetricAggContexts.MapScript.LeafFactory leafFactory = factory.newFactory(params, state, lookup);
         ScriptedMetricAggContexts.MapScript script = leafFactory.newInstance(leafReaderContext);
 
         script.execute();
 
-        assert(state.containsKey("testField"));
+        assertTrue(state.containsKey("testField"));
         assertEquals(3, state.get("testField"));
     }
 

diff --git a/server/src/main/java/org/elasticsearch/ingest/ConditionalProcessor.java b/server/src/main/java/org/elasticsearch/ingest/ConditionalProcessor.java
@@ -19,37 +19,38 @@
 
 package org.elasticsearch.ingest;
 
+import org.apache.logging.log4j.LogManager;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.script.DynamicMap;
+import org.elasticsearch.script.IngestConditionalScript;
+import org.elasticsearch.script.Script;
+import org.elasticsearch.script.ScriptService;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiConsumer;
+import java.util.function.Function;
 import java.util.function.LongSupplier;
 import java.util.stream.Collectors;
 
-import org.elasticsearch.script.DeprecationMap;
-import org.elasticsearch.script.IngestConditionalScript;
-import org.elasticsearch.script.Script;
-import org.elasticsearch.script.ScriptService;
-
 public class ConditionalProcessor extends AbstractProcessor implements WrappingProcessor {
 
-    private static final Map<String, String> DEPRECATIONS;
-    static {
-        Map<String, String> deprecations = new HashMap<>();
-        deprecations.put(
-                "_type",
-                "[types removal] Looking up doc types [_type] in scripts is deprecated."
-        );
-        DEPRECATIONS = Collections.unmodifiableMap(deprecations);
-    }
+    private static final DeprecationLogger deprecationLogger =
+            new DeprecationLogger(LogManager.getLogger(DynamicMap.class));
+    private static final Map<String, Function<Object, Object>> FUNCTIONS = org.elasticsearch.common.collect.Map.of(
+            "_type", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("conditional-processor__type",
+                        "[types removal] Looking up doc types [_type] in scripts is deprecated.");
+                return value;
+            });
 
     static final String TYPE = "conditional";
 
@@ -110,8 +111,7 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
     boolean evaluate(IngestDocument ingestDocument) {
         IngestConditionalScript script =
             scriptService.compile(condition, IngestConditionalScript.CONTEXT).newInstance(condition.getParams());
-        return script.execute(new UnmodifiableIngestData(
-                new DeprecationMap(ingestDocument.getSourceAndMetadata(), DEPRECATIONS, "conditional-processor")));
+        return script.execute(new UnmodifiableIngestData(new DynamicMap(ingestDocument.getSourceAndMetadata(), FUNCTIONS)));
     }
 
     public Processor getInnerProcessor() {

diff --git a/server/src/main/java/org/elasticsearch/script/AbstractSortScript.java b/server/src/main/java/org/elasticsearch/script/AbstractSortScript.java
@@ -18,36 +18,41 @@
  */
 package org.elasticsearch.script;
 
-import java.io.IOException;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import org.apache.logging.log4j.LogManager;
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.search.Scorable;
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.lucene.ScorerAware;
 import org.elasticsearch.index.fielddata.ScriptDocValues;
 import org.elasticsearch.search.lookup.LeafSearchLookup;
 import org.elasticsearch.search.lookup.SearchLookup;
+import org.elasticsearch.search.lookup.SourceLookup;
 
-abstract class AbstractSortScript implements ScorerAware {
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Function;
 
-    private static final Map<String, String> DEPRECATIONS;
+abstract class AbstractSortScript implements ScorerAware {
 
-    static {
-        Map<String, String> deprecations = new HashMap<>();
-        deprecations.put(
-            "doc",
-            "Accessing variable [doc] via [params.doc] from within a sort-script " +
-                "is deprecated in favor of directly accessing [doc]."
-        );
-        deprecations.put(
-            "_doc",
-            "Accessing variable [doc] via [params._doc] from within a sort-script " +
-                "is deprecated in favor of directly accessing [doc]."
-        );
-        DEPRECATIONS = Collections.unmodifiableMap(deprecations);
-    }
+    private static final DeprecationLogger deprecationLogger =
+            new DeprecationLogger(LogManager.getLogger(DynamicMap.class));
+    private static final Map<String, Function<Object, Object>> PARAMS_FUNCTIONS = org.elasticsearch.common.collect.Map.of(
+            "doc", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("sort-script_doc",
+                        "Accessing variable [doc] via [params.doc] from within an sort-script "
+                                + "is deprecated in favor of directly accessing [doc].");
+                return value;
+            },
+            "_doc", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("sort-script__doc",
+                        "Accessing variable [doc] via [params._doc] from within an sort-script "
+                                + "is deprecated in favor of directly accessing [doc].");
+                return value;
+            },
+            "_source", value -> ((SourceLookup)value).loadSourceIfNeeded()
+    );
 
     /**
      * The generic runtime parameters for the script.
@@ -66,7 +71,7 @@ abstract class AbstractSortScript implements ScorerAware {
         this.leafLookup = lookup.getLeafSearchLookup(leafContext);
         Map<String, Object> parameters = new HashMap<>(params);
         parameters.putAll(leafLookup.asMap());
-        this.params = new DeprecationMap(parameters, DEPRECATIONS, "sort-script");
+        this.params = new DynamicMap(parameters, PARAMS_FUNCTIONS);
     }
 
     protected AbstractSortScript() {

diff --git a/server/src/main/java/org/elasticsearch/script/AggregationScript.java b/server/src/main/java/org/elasticsearch/script/AggregationScript.java
@@ -18,40 +18,45 @@
  */
 package org.elasticsearch.script;
 
-import java.io.IOException;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import org.apache.logging.log4j.LogManager;
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.search.Scorable;
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.lucene.ScorerAware;
 import org.elasticsearch.index.fielddata.ScriptDocValues;
 import org.elasticsearch.search.lookup.LeafSearchLookup;
 import org.elasticsearch.search.lookup.SearchLookup;
+import org.elasticsearch.search.lookup.SourceLookup;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Function;
 
 public abstract class AggregationScript implements ScorerAware {
 
     public static final String[] PARAMETERS = {};
 
     public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("aggs", Factory.class);
 
-    private static final Map<String, String> DEPRECATIONS;
-
-    static {
-        Map<String, String> deprecations = new HashMap<>();
-        deprecations.put(
-            "doc",
-            "Accessing variable [doc] via [params.doc] from within an aggregation-script " +
-                "is deprecated in favor of directly accessing [doc]."
-        );
-        deprecations.put(
-            "_doc",
-            "Accessing variable [doc] via [params._doc] from within an aggregation-script " +
-                "is deprecated in favor of directly accessing [doc]."
-        );
-        DEPRECATIONS = Collections.unmodifiableMap(deprecations);
-    }
+    private static final DeprecationLogger deprecationLogger =
+            new DeprecationLogger(LogManager.getLogger(DynamicMap.class));
+    private static final Map<String, Function<Object, Object>> PARAMS_FUNCTIONS = org.elasticsearch.common.collect.Map.of(
+            "doc", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("aggregation-script_doc",
+                        "Accessing variable [doc] via [params.doc] from within an aggregation-script "
+                                + "is deprecated in favor of directly accessing [doc].");
+                return value;
+            },
+            "_doc", value -> {
+                deprecationLogger.deprecatedAndMaybeLog("aggregation-script__doc",
+                        "Accessing variable [doc] via [params._doc] from within an aggregation-script "
+                                + "is deprecated in favor of directly accessing [doc].");
+                return value;
+            },
+            "_source", value -> ((SourceLookup)value).loadSourceIfNeeded()
+    );
 
     /**
      * The generic runtime parameters for the script.
@@ -71,7 +76,7 @@ public abstract class AggregationScript implements ScorerAware {
     private Object value;
 
     public AggregationScript(Map<String, Object> params, SearchLookup lookup, LeafReaderContext leafContext) {
-        this.params = new DeprecationMap(new HashMap<>(params), DEPRECATIONS, "aggregation-script");
+        this.params = new DynamicMap(new HashMap<>(params), PARAMS_FUNCTIONS);
         this.leafLookup = lookup.getLeafSearchLookup(leafContext);
         this.params.putAll(leafLookup.asMap());
     }

diff --git a/.../elasticsearch/script/DeprecationMap.java → .../org/elasticsearch/script/DynamicMap.java b/.../elasticsearch/script/DeprecationMap.java → .../org/elasticsearch/script/DynamicMap.java
@@ -19,28 +19,26 @@
 
 package org.elasticsearch.script;
 
-import org.apache.logging.log4j.LogManager;
-import org.elasticsearch.common.logging.DeprecationLogger;
-
 import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Function;
 
-public final class DeprecationMap implements Map<String, Object> {
-
-    private static final DeprecationLogger deprecationLogger =
-        new DeprecationLogger(LogManager.getLogger(DeprecationMap.class));
+/**
+ * DynamicMap is used to wrap a Map for a script parameter. A set of
+ * functions is provided for the overridden values where the function
+ * is applied to the existing value when one exists for the
+ * corresponding key.
+ */
+public final class DynamicMap implements Map<String, Object> {
 
     private final Map<String, Object> delegate;
 
-    private final Map<String, String> deprecations;
-
-    private final String logKeyPrefix;
+    private final Map<String, Function<Object, Object>> functions;
 
-    public DeprecationMap(Map<String, Object> delegate, Map<String, String> deprecations, String logKeyPrefix) {
+    public DynamicMap(Map<String, Object> delegate, Map<String, Function<Object, Object>> functions) {
         this.delegate = delegate;
-        this.deprecations = deprecations;
-        this.logKeyPrefix = logKeyPrefix;
+        this.functions = functions;
     }
 
     @Override
@@ -65,11 +63,12 @@ public boolean containsValue(final Object value) {
 
     @Override
     public Object get(final Object key) {
-        String deprecationMessage = deprecations.get(key);
-        if (deprecationMessage != null) {
-            deprecationLogger.deprecatedAndMaybeLog(logKeyPrefix + "_" + key, deprecationMessage);
+        Object value = delegate.get(key);
+        Function<Object, Object> function = functions.get(key);
+        if (function != null) {
+            value = function.apply(value);
         }
-        return delegate.get(key);
+        return value;
     }
 
     @Override