Adds deprecation logging to ScriptDocValues#getValues.

docs/reference/aggregations/bucket/diversified-sampler-aggregation.asciidoc

@@ -108,7 +108,7 @@ POST /stackoverflow/_search?size=0
                 "max_docs_per_value" : 3,
                 "script" : {
                     "lang": "painless",
-                    "source": "doc['tags'].values.hashCode()"
+                    "source": "doc['tags'].hashCode()"
                 }
             },
             "aggs": {

docs/reference/modules/scripting/fields.asciidoc

@@ -144,7 +144,7 @@ access `text` fields from scripts.
 
 _Stored fields_ -- fields explicitly marked as
 <<mapping-store,`"store": true`>> -- can be accessed using the
-`_fields['field_name'].value` or `_fields['field_name'].values` syntax.
+`_fields['field_name'].value` or `_fields['field_name']` syntax.
 
 The document <<mapping-source-field,`_source`>>, which is really just a
 special stored field,  can be accessed using the `_source.field_name` syntax.

modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml

@@ -370,7 +370,7 @@
                 script_fields:
                     foobar:
                         script: 
-                            source: "doc['f'].values.size()"
+                            source: "doc['f'].size()"
                             lang: painless
 
 

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

@@ -19,22 +19,27 @@
 
 package org.elasticsearch.index.fielddata;
 
+import org.apache.logging.log4j.LogManager;
 import org.apache.lucene.index.SortedNumericDocValues;
 import org.apache.lucene.util.ArrayUtil;
 import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.BytesRefBuilder;
 import org.elasticsearch.common.geo.GeoHashUtils;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.script.JodaCompatibleZonedDateTime;
 
 import java.io.IOException;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.time.Instant;
 import java.time.ZoneOffset;
 import java.util.AbstractList;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
+import java.util.function.BiConsumer;
 import java.util.function.UnaryOperator;
 
 /**
@@ -48,6 +53,25 @@
  */
 public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
+    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(ScriptDocValues.class));
+    /**
+     * Callback for deprecated fields. In production this should always point to
+     * {@link #deprecationLogger} but tests will override it so they can test
+     * that we use the required permissions when calling it.
+     */
+    private final BiConsumer<String, String> deprecationCallback;
+
+    public ScriptDocValues() {
+        deprecationCallback = deprecationLogger::deprecatedAndMaybeLog;
+    }
+
+    /**
+     * Constructor for testing deprecation callback.
+     */
+    ScriptDocValues(BiConsumer<String, String> deprecationCallback) {
+        this.deprecationCallback = deprecationCallback;
+    }
+
     /**
      * Set the current doc ID.
      */
@@ -57,6 +81,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
      * Return a copy of the list of the values for the current document.
      */
     public final List<T> getValues() {
+        deprecated("ScriptDocValues#getValues", "Deprecated getValues used, the field is a list and should be accessed directly."
+                + " For example, use doc['foo'] instead of doc['foo'].values.");
         return this;
     }
 
@@ -86,6 +112,21 @@ public final void sort(Comparator<? super T> c) {
         throw new UnsupportedOperationException("doc values are unmodifiable");
     }
 
+    /**
+     * Log a deprecation log, with the server's permissions and not the permissions
+     * of the script calling this method. We need to do this to prevent errors
+     * when rolling the log file.
+     */
+    private void deprecated(String key, String message) {
+        AccessController.doPrivileged(new PrivilegedAction<Void>() {
+            @Override
+            public Void run() {
+                deprecationCallback.accept(key, message);
+                return null;
+            }
+        });
+    }
+
     public static final class Longs extends ScriptDocValues<Long> {
         private final SortedNumericDocValues in;
         private long[] values = new long[0];
@@ -98,6 +139,14 @@ public Longs(SortedNumericDocValues in) {
             this.in = in;
         }
 
+        /**
+         * Constructor for testing deprecation callback.
+         */
+        Longs(SortedNumericDocValues in, BiConsumer<String, String> deprecationCallback) {
+            super(deprecationCallback);
+            this.in = in;
+        }
+
         @Override
         public void setNextDocId(int docId) throws IOException {
             if (in.advanceExact(docId)) {
@@ -155,6 +204,14 @@ public Dates(SortedNumericDocValues in) {
             this.in = in;
         }
 
+        /**
+         * Constructor for testing deprecation callback.
+         */
+        Dates(SortedNumericDocValues in, BiConsumer<String, String> deprecationCallback) {
+            super(deprecationCallback);
+            this.in = in;
+        }
+
         /**
          * Fetch the first field value or 0 millis after epoch if there are no
          * in.
@@ -273,6 +330,14 @@ public GeoPoints(MultiGeoPointValues in) {
             this.in = in;
         }
 
+        /**
+         * Constructor for testing deprecation callback.
+         */
+        GeoPoints(MultiGeoPointValues in, BiConsumer<String, String> deprecationCallback) {
+            super(deprecationCallback);
+            this.in =  in;
+        }
+
         @Override
         public void setNextDocId(int docId) throws IOException {
             if (in.advanceExact(docId)) {

server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java

@@ -0,0 +1,93 @@
+/*
+ * 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.index.fielddata;
+
+import org.elasticsearch.index.fielddata.ScriptDocValues.Dates;
+import org.elasticsearch.test.ESTestCase;
+
+import java.security.AccessControlContext;
+import java.security.AccessController;
+import java.security.PermissionCollection;
+import java.security.Permissions;
+import java.security.PrivilegedAction;
+import java.security.ProtectionDomain;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.BiConsumer;
+
+import static org.hamcrest.Matchers.contains;
+
+public class ScriptDocValuesDatesTests extends ESTestCase {
+
+    public void testGetValues() {
+        Set<String> keys = new HashSet<>();
+        Set<String> warnings = new HashSet<>();
+
+        Dates dates = biconsumerWrap((deprecationKey, deprecationMessage) -> {
+            keys.add(deprecationKey);
+            warnings.add(deprecationMessage);
+
+            // Create a temporary directory to prove we are running with the server's permissions.
+            createTempDir();
+        });
+
+        /*
+         * Invoke getValues() without any permissions to verify it still works.
+         * This is done using the callback created above, which creates a temp
+         * directory, which is not possible with "noPermission".
+         */
+        PermissionCollection noPermissions = new Permissions();
+        AccessControlContext noPermissionsAcc = new AccessControlContext(
+            new ProtectionDomain[] {
+                new ProtectionDomain(null, noPermissions)
+            }
+        );
+        AccessController.doPrivileged(new PrivilegedAction<Void>(){
+            public Void run() {
+                dates.getValues();
+                return null;
+            }
+        }, noPermissionsAcc);
+
+        assertThat(warnings, contains(
+            "Deprecated getValues used, the field is a list and should be accessed directly."
+           + " For example, use doc['foo'] instead of doc['foo'].values."));
+        assertThat(keys, contains("ScriptDocValues#getValues"));
+
+
+    }
+
+    private Dates biconsumerWrap(BiConsumer<String, String> deprecationHandler) {
+        return new Dates(new AbstractSortedNumericDocValues() {
+            @Override
+            public boolean advanceExact(int doc) {
+                return true;
+            }
+            @Override
+            public int docValueCount() {
+                return 0;
+            }
+            @Override
+            public long nextValue() {
+                return 0L;
+            }
+        }, deprecationHandler);
+    }
+}

server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java

@@ -21,10 +21,22 @@
 
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
+import org.elasticsearch.index.fielddata.ScriptDocValues.GeoPoints;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
+import java.security.AccessControlContext;
+import java.security.AccessController;
+import java.security.PermissionCollection;
+import java.security.Permissions;
+import java.security.PrivilegedAction;
+import java.security.ProtectionDomain;
 import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.BiConsumer;
+
+import static org.hamcrest.Matchers.contains;
 
 public class ScriptDocValuesGeoPointsTests extends ESTestCase {
 
@@ -70,8 +82,18 @@ public void testGeoGetLatLon() throws IOException {
         final double lat2 = randomLat();
         final double lon1 = randomLon();
         final double lon2 = randomLon();
+
+        Set<String> warnings = new HashSet<>();
+        Set<String> keys = new HashSet<>();
+
         final MultiGeoPointValues values = wrap(new GeoPoint(lat1, lon1), new GeoPoint(lat2, lon2));
-        final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values);
+        final ScriptDocValues.GeoPoints script = geoPointsWrap(values, (deprecationKey, deprecationMessage)  -> {
+            keys.add(deprecationKey);
+            warnings.add(deprecationMessage);
+
+            // Create a temporary directory to prove we are running with the server's permissions.
+            createTempDir();
+        });
         script.setNextDocId(1);
         assertEquals(true, script.isEmpty());
         script.setNextDocId(0);
@@ -82,6 +104,30 @@ public void testGeoGetLatLon() throws IOException {
         assertEquals(lon1, script.getLon(), 0);
         assertTrue(Arrays.equals(new double[] {lat1, lat2}, script.getLats()));
         assertTrue(Arrays.equals(new double[] {lon1, lon2}, script.getLons()));
+
+        /*
+         * Invoke getValues() without any permissions to verify it still works.
+         * This is done using the callback created above, which creates a temp
+         * directory, which is not possible with "noPermission".
+         */
+        PermissionCollection noPermissions = new Permissions();
+        AccessControlContext noPermissionsAcc = new AccessControlContext(
+            new ProtectionDomain[] {
+                new ProtectionDomain(null, noPermissions)
+            }
+        );
+        AccessController.doPrivileged(new PrivilegedAction<Void>(){
+            public Void run() {
+                script.getValues();
+                return null;
+            }
+        }, noPermissionsAcc);
+
+        assertThat(warnings, contains(
+            "Deprecated getValues used, the field is a list and should be accessed directly."
+           + " For example, use doc['foo'] instead of doc['foo'].values."));
+        assertThat(keys, contains("ScriptDocValues#getValues"));
+
     }
 
     public void testGeoDistance() throws IOException {
@@ -110,4 +156,8 @@ public void testGeoDistance() throws IOException {
         assertEquals(42, emptyScript.planeDistanceWithDefault(otherLat, otherLon, 42), 0);
     }
 
+    private GeoPoints geoPointsWrap(MultiGeoPointValues in, BiConsumer<String, String> deprecationHandler) {
+        return new GeoPoints(in, deprecationHandler);
+    }
+
 }