From c5704a4de74e5c8e92e5da0d600c4884bc7d5537 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Wed, 3 Oct 2018 12:24:53 -0500 Subject: [PATCH 01/11] Adds deprecation logging to ScriptDocValues#getValues. First commit addressing issue #22919. `ScriptDocValues#getValues` was added for backwards compatibility but no longer needed. Scripts using the syntax `doc['foo'].values` when `doc['foo']` is a list should be using `doc['foo']` instead. --- .../index/fielddata/ScriptDocValues.java | 65 +++++++++++++ .../fielddata/ScriptDocValuesDatesTests.java | 94 +++++++++++++++++++ .../ScriptDocValuesGeoPointsTests.java | 52 +++++++++- .../fielddata/ScriptDocValuesLongsTests.java | 51 +++++++++- 4 files changed, 258 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index 0c3e634e35229..f2e7463c5a649 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -19,6 +19,7 @@ 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; @@ -26,15 +27,19 @@ 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 extends AbstractList { + 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 deprecationCallback; + + public ScriptDocValues() { + deprecationCallback = deprecationLogger::deprecatedAndMaybeLog; + } + + /** + * Constructor for testing deprecation callback. + */ + ScriptDocValues(BiConsumer deprecationCallback) { + this.deprecationCallback = deprecationCallback; + } + /** * Set the current doc ID. */ @@ -57,6 +81,8 @@ public abstract class ScriptDocValues extends AbstractList { * Return a copy of the list of the values for the current document. */ public final List 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 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() { + @Override + public Void run() { + deprecationCallback.accept(key, message); + return null; + } + }); + } + public static final class Longs extends ScriptDocValues { 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 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 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 deprecationCallback) { + super(deprecationCallback); + this.in = in; + } + @Override public void setNextDocId(int docId) throws IOException { if (in.advanceExact(docId)) { diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java new file mode 100644 index 0000000000000..ab8a70e6c65cf --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java @@ -0,0 +1,94 @@ +/* + * 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.Arrays; +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 keys = new HashSet<>(); + Set 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(){ + 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 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); + } +} diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java index 1f71a808ab58a..2924694c10274 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java +++ b/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 warnings = new HashSet<>(); + Set 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(){ + 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 deprecationHandler) { + return new GeoPoints(in, deprecationHandler); + } + } diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java index c22cb4919677a..fd2a18a1662f1 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java @@ -23,6 +23,17 @@ 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.HashSet; +import java.util.Set; +import java.util.function.BiConsumer; + +import static org.hamcrest.Matchers.contains; public class ScriptDocValuesLongsTests extends ESTestCase { public void testLongs() throws IOException { @@ -33,7 +44,17 @@ public void testLongs() throws IOException { values[d][i] = randomLong(); } } - Longs longs = wrap(values); + + Set warnings = new HashSet<>(); + Set keys = new HashSet<>(); + + Longs longs = wrap(values, (deprecationKey, deprecationMessage) -> { + keys.add(deprecationKey); + warnings.add(deprecationMessage); + + // Create a temporary directory to prove we are running with the server's permissions. + createTempDir(); + }); for (int round = 0; round < 10; round++) { int d = between(0, values.length - 1); @@ -55,9 +76,33 @@ public void testLongs() throws IOException { Exception e = expectThrows(UnsupportedOperationException.class, () -> longs.getValues().add(100L)); assertEquals("doc values are unmodifiable", e.getMessage()); } + + /* + * 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(){ + public Void run() { + longs.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 Longs wrap(long[][] values) { + private Longs wrap(long[][] values, BiConsumer deprecationCallback) { return new Longs(new AbstractSortedNumericDocValues() { long[] current; int i; @@ -76,6 +121,6 @@ public int docValueCount() { public long nextValue() { return current[i++]; } - }); + }, deprecationCallback); } } From 65c3f64ac46d409b046e6afbecb1ccb90a568c1c Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Fri, 2 Nov 2018 16:04:01 -0500 Subject: [PATCH 02/11] Fixes two build errors in #34279 * Removes unused import in ScriptDocValuesDatesTest * Removes used of `.values` in example in diversified-sampler-aggregation.asciidoc --- .../bucket/diversified-sampler-aggregation.asciidoc | 2 +- .../index/fielddata/ScriptDocValuesDatesTests.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/reference/aggregations/bucket/diversified-sampler-aggregation.asciidoc b/docs/reference/aggregations/bucket/diversified-sampler-aggregation.asciidoc index 970a2ffdc1e33..07d8261d200b4 100644 --- a/docs/reference/aggregations/bucket/diversified-sampler-aggregation.asciidoc +++ b/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": { diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java index ab8a70e6c65cf..42f5470ee89fc 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java @@ -28,7 +28,6 @@ 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; From 90062839b2eb2c0e4850084da47da2ca2a7b3ad3 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 5 Nov 2018 07:22:26 -0600 Subject: [PATCH 03/11] Removes use of .values from painless test. Part of #34279 --- .../test/resources/rest-api-spec/test/painless/30_search.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml index 9a43e1f9aa445..292fab938d9e3 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yml +++ b/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 From 0103405ea9464eacc0b98742c834abea9d511291 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 26 Nov 2018 17:12:15 -0600 Subject: [PATCH 04/11] Updates tests to use `doc[foo]` syntax rather than `doc[foo].values`. --- docs/reference/modules/scripting/fields.asciidoc | 2 +- .../aggregations/AggregationTestScriptsPlugin.java | 4 ++-- .../search/aggregations/bucket/IpTermsIT.java | 4 ++-- .../search/aggregations/bucket/MinDocCountIT.java | 8 ++++---- .../search/aggregations/bucket/RangeIT.java | 4 ++-- .../search/aggregations/metrics/CardinalityIT.java | 8 ++++---- .../aggregations/metrics/ExtendedStatsIT.java | 2 +- .../aggregations/metrics/HDRPercentileRanksIT.java | 2 +- .../aggregations/metrics/HDRPercentilesIT.java | 2 +- .../search/aggregations/metrics/MaxIT.java | 2 +- .../metrics/MedianAbsoluteDeviationIT.java | 2 +- .../search/aggregations/metrics/MinIT.java | 2 +- .../search/aggregations/metrics/StatsIT.java | 2 +- .../metrics/TDigestPercentileRanksIT.java | 2 +- .../aggregations/metrics/TDigestPercentilesIT.java | 2 +- .../search/fields/SearchFieldsIT.java | 14 +++++++------- .../elasticsearch/search/sort/SimpleSortIT.java | 4 ++-- 17 files changed, 33 insertions(+), 33 deletions(-) diff --git a/docs/reference/modules/scripting/fields.asciidoc b/docs/reference/modules/scripting/fields.asciidoc index bdd6c5785d81c..408707207f010 100644 --- a/docs/reference/modules/scripting/fields.asciidoc +++ b/docs/reference/modules/scripting/fields.asciidoc @@ -144,7 +144,7 @@ access `text` fields from scripts. _Stored fields_ -- fields explicitly marked as <> -- 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 <>, which is really just a special stored field, can be accessed using the `_source.field_name` syntax. diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregationTestScriptsPlugin.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregationTestScriptsPlugin.java index 38d9e62604c46..5d71176e79820 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregationTestScriptsPlugin.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregationTestScriptsPlugin.java @@ -37,7 +37,7 @@ public class AggregationTestScriptsPlugin extends MockScriptPlugin { // Equivalent to: // - // List values = doc['values'].values; + // List values = doc['values']; // double[] res = new double[values.size()]; // for (int i = 0; i < res.length; i++) { // res[i] = values.get(i) - dec; @@ -85,7 +85,7 @@ protected Map, Object>> pluginScripts() { return value.getValue() + inc; }); - scripts.put("doc['values'].values", vars -> { + scripts.put("doc['values']", vars -> { Map doc = (Map) vars.get("doc"); return doc.get("values"); }); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpTermsIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpTermsIT.java index c99ada821a5e3..dd09e2e79c541 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpTermsIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpTermsIT.java @@ -53,7 +53,7 @@ protected Map, Object>> pluginScripts() { return doc.get("ip"); }); - scripts.put("doc['ip'].values", vars -> { + scripts.put("doc['ip']", vars -> { Map doc = (Map) vars.get("doc"); return ((ScriptDocValues) doc.get("ip")).get(0); }); @@ -96,7 +96,7 @@ public void testScriptValues() throws Exception { client().prepareIndex("index", "type", "3").setSource("ip", "2001:db8::2:1")); Script script = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "doc['ip'].values", Collections.emptyMap()); + "doc['ip']", Collections.emptyMap()); SearchResponse response = client().prepareSearch("index").addAggregation( AggregationBuilders.terms("my_terms").script(script).executionHint(randomExecutionHint())).get(); assertSearchResponse(response); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java index eeb6e12161383..3013b59edcee1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java @@ -81,19 +81,19 @@ public static class CustomScriptPlugin extends AggregationTestScriptsPlugin { protected Map, Object>> pluginScripts() { Map, Object>> scripts = new HashMap<>(); - scripts.put("doc['d'].values", vars -> { + scripts.put("doc['d']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Doubles value = (ScriptDocValues.Doubles) doc.get("d"); return value.getValues(); }); - scripts.put("doc['l'].values", vars -> { + scripts.put("doc['l']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Longs value = (ScriptDocValues.Longs) doc.get("l"); return value.getValues(); }); - scripts.put("doc['s'].values", vars -> { + scripts.put("doc['s']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Strings value = (ScriptDocValues.Strings) doc.get("s"); return value.getValues(); @@ -154,7 +154,7 @@ TermsAggregationBuilder apply(TermsAggregationBuilder builder, String field) { @Override TermsAggregationBuilder apply(TermsAggregationBuilder builder, String field) { return builder.script(new org.elasticsearch.script.Script(ScriptType.INLINE, - CustomScriptPlugin.NAME, "doc['" + field + "'].values", Collections.emptyMap())); + CustomScriptPlugin.NAME, "doc['" + field + "']", Collections.emptyMap())); } }; abstract TermsAggregationBuilder apply(TermsAggregationBuilder builder, String field); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java index 894834882f9f9..7508cd4e83976 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java @@ -84,7 +84,7 @@ protected Map, Object>> pluginScripts() { return value.getValue(); }); - scripts.put("doc['" + MULTI_VALUED_FIELD_NAME + "'].values", vars -> { + scripts.put("doc['" + MULTI_VALUED_FIELD_NAME + "']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Longs value = (ScriptDocValues.Longs) doc.get(MULTI_VALUED_FIELD_NAME); return value.getValues(); @@ -693,7 +693,7 @@ public void testNoRangesInQuery() { public void testScriptMultiValued() throws Exception { Script script = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['" + MULTI_VALUED_FIELD_NAME + "'].values", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['" + MULTI_VALUED_FIELD_NAME + "']", Collections.emptyMap()); SearchResponse response = client() .prepareSearch("idx") diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java index cf155b8690d3c..b3213306da3d7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java @@ -72,7 +72,7 @@ protected Map, Object>> pluginScripts() { return doc.get("str_value"); }); - scripts.put("doc['str_values'].values", vars -> { + scripts.put("doc['str_values']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Strings strValue = (ScriptDocValues.Strings) doc.get("str_values"); return strValue.getValues(); @@ -83,7 +83,7 @@ protected Map, Object>> pluginScripts() { return doc.get(singleNumericField()); }); - scripts.put("doc[' + multiNumericField(false) + '].values", vars -> { + scripts.put("doc[' + multiNumericField(false) + ']", vars -> { Map doc =(Map) vars.get("doc"); return ((ScriptDocValues) doc.get(multiNumericField(false))).getValues(); }); @@ -322,7 +322,7 @@ public void testMultiValuedStringScript() throws Exception { .addAggregation( cardinality("cardinality") .precisionThreshold(precisionThreshold) - .script(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['str_values'].values", emptyMap()))) + .script(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['str_values']", emptyMap()))) .execute().actionGet(); assertSearchResponse(response); @@ -349,7 +349,7 @@ public void testSingleValuedNumericScript() throws Exception { public void testMultiValuedNumericScript() throws Exception { Script script = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc[' + multiNumericField(false) + '].values", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc[' + multiNumericField(false) + ']", Collections.emptyMap()); SearchResponse response = client().prepareSearch("idx").setTypes("type") .addAggregation(cardinality("cardinality").precisionThreshold(precisionThreshold).script(script)) .execute().actionGet(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java index 3daafb8684eb6..2f70c5ef19be2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java @@ -502,7 +502,7 @@ public void testScriptMultiValued() throws Exception { .addAggregation( extendedStats("stats") .script(new Script(ScriptType.INLINE, - AggregationTestScriptsPlugin.NAME, "doc['values'].values", Collections.emptyMap())) + AggregationTestScriptsPlugin.NAME, "doc['values']", Collections.emptyMap())) .sigma(sigma)) .execute().actionGet(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java index 5d0bbf0f853a8..b866992fe7af2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java @@ -457,7 +457,7 @@ public void testScriptMultiValued() throws Exception { int sigDigits = randomSignificantDigits(); final double[] pcts = randomPercents(minValues, maxValues); - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client() .prepareSearch("idx") diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java index 256717f809f3b..085eea565a29c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java @@ -416,7 +416,7 @@ public void testScriptMultiValued() throws Exception { final double[] pcts = randomPercentiles(); int sigDigits = randomSignificantDigits(); - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client() .prepareSearch("idx") diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxIT.java index 9c46c1db7ea9b..17bf3a9e619b8 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxIT.java @@ -294,7 +294,7 @@ public void testScriptMultiValued() throws Exception { .addAggregation( max("max") .script(new Script(ScriptType.INLINE, - AggregationTestScriptsPlugin.NAME, "doc['values'].values", Collections.emptyMap()))) + AggregationTestScriptsPlugin.NAME, "doc['values']", Collections.emptyMap()))) .get(); assertHitCount(searchResponse, 10); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationIT.java index 4e056ceb1b897..e04905a38ecfc 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationIT.java @@ -441,7 +441,7 @@ public void testScriptMultiValued() throws Exception { .script(new Script( ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, - "doc['values'].values", + "doc['values']", Collections.emptyMap()))) .execute() .actionGet(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinIT.java index 7bb0d23c4c2fa..55f509966ff21 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinIT.java @@ -305,7 +305,7 @@ public void testScriptSingleValuedWithParams() throws Exception { @Override public void testScriptMultiValued() throws Exception { - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client().prepareSearch("idx").setQuery(matchAllQuery()) .addAggregation(min("min").script(script)) .get(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsIT.java index a97982cccac3b..b0d9855a9fd00 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsIT.java @@ -392,7 +392,7 @@ public void testScriptSingleValuedWithParams() throws Exception { @Override public void testScriptMultiValued() throws Exception { - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client().prepareSearch("idx") .setQuery(matchAllQuery()) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksIT.java index 4a68cb6858213..0b8529ec07001 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksIT.java @@ -392,7 +392,7 @@ public void testScriptSingleValuedWithParams() throws Exception { @Override public void testScriptMultiValued() throws Exception { final double[] pcts = randomPercents(minValues, maxValues); - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client().prepareSearch("idx") .setQuery(matchAllQuery()) .addAggregation( diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java index 25e3435ea9724..d506aa60d7b71 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java @@ -371,7 +371,7 @@ public void testScriptSingleValuedWithParams() throws Exception { @Override public void testScriptMultiValued() throws Exception { final double[] pcts = randomPercentiles(); - Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values'].values", emptyMap()); + Script script = new Script(ScriptType.INLINE, AggregationTestScriptsPlugin.NAME, "doc['values']", emptyMap()); SearchResponse searchResponse = client().prepareSearch("idx") .setQuery(matchAllQuery()) diff --git a/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java b/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java index 1a86b3b1da283..72ea99df8ff58 100644 --- a/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java +++ b/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java @@ -131,12 +131,12 @@ protected Map, Object>> pluginScripts() { scripts.put("return null", vars -> null); - scripts.put("doc['l'].values", vars -> docScript(vars, "l")); - scripts.put("doc['ml'].values", vars -> docScript(vars, "ml")); - scripts.put("doc['d'].values", vars -> docScript(vars, "d")); - scripts.put("doc['md'].values", vars -> docScript(vars, "md")); - scripts.put("doc['s'].values", vars -> docScript(vars, "s")); - scripts.put("doc['ms'].values", vars -> docScript(vars, "ms")); + scripts.put("doc['l']", vars -> docScript(vars, "l")); + scripts.put("doc['ml']", vars -> docScript(vars, "ml")); + scripts.put("doc['d']", vars -> docScript(vars, "d")); + scripts.put("doc['md']", vars -> docScript(vars, "md")); + scripts.put("doc['s']", vars -> docScript(vars, "s")); + scripts.put("doc['ms']", vars -> docScript(vars, "ms")); return scripts; } @@ -925,7 +925,7 @@ public void testScriptFields() throws Exception { SearchRequestBuilder req = client().prepareSearch("index"); for (String field : Arrays.asList("s", "ms", "l", "ml", "d", "md")) { req.addScriptField(field, - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['" + field + "'].values", Collections.emptyMap())); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['" + field + "']", Collections.emptyMap())); } SearchResponse resp = req.get(); assertSearchResponse(resp); diff --git a/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java b/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java index 6668c1be0e439..82b0608046a74 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java @@ -85,7 +85,7 @@ protected Map, Object>> pluginScripts() { return ((ScriptDocValues.Strings) doc.get("id")).getValue(); }); - scripts.put("doc['id'].values[0]", vars -> { + scripts.put("doc['id'][0]", vars -> { Map doc = (Map) vars.get("doc"); return ((ScriptDocValues.Strings) doc.get("id")).getValues().get(0); }); @@ -399,7 +399,7 @@ public void testDocumentsWithNullValue() throws Exception { searchResponse = client().prepareSearch() .setQuery(matchAllQuery()) - .addScriptField("id", new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['id'].values[0]", Collections.emptyMap())) + .addScriptField("id", new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['id'][0]", Collections.emptyMap())) .addSort("svalue", SortOrder.ASC) .get(); From 5be0966325e2bf144f78e37d590e86eb74a25a41 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 3 Dec 2018 11:37:17 -0600 Subject: [PATCH 05/11] Removes use of `getValues()` and replaces use of `doc[foo].values` with `doc[foo]`. --- .../index/fielddata/ScriptDocValues.java | 79 +--------------- .../fielddata/ScriptDocValuesDatesTests.java | 93 ------------------- .../ScriptDocValuesGeoPointsTests.java | 52 +---------- .../fielddata/ScriptDocValuesLongsTests.java | 53 +---------- .../aggregations/bucket/MinDocCountIT.java | 6 +- .../search/aggregations/bucket/RangeIT.java | 2 +- .../aggregations/metrics/CardinalityIT.java | 4 +- .../search/fields/SearchFieldsIT.java | 2 +- .../functionscore/ExplainableScriptIT.java | 2 +- .../functionscore/RandomScoreFunctionIT.java | 2 +- .../search/sort/FieldSortIT.java | 4 +- .../search/sort/SimpleSortIT.java | 4 +- 12 files changed, 21 insertions(+), 282 deletions(-) delete mode 100644 server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index f2e7463c5a649..2633c6e036158 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -19,7 +19,6 @@ 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; @@ -27,25 +26,20 @@ 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; /** * Script level doc values, the assumption is that any implementation will - * implement a getValue and a getValues that return - * the relevant type that then can be used in scripts. + * implement a getValue. * * Implementations should not internally re-use objects for the values that they * return as a single {@link ScriptDocValues} instance can be reused to return @@ -53,39 +47,11 @@ */ public abstract class ScriptDocValues extends AbstractList { - 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 deprecationCallback; - - public ScriptDocValues() { - deprecationCallback = deprecationLogger::deprecatedAndMaybeLog; - } - - /** - * Constructor for testing deprecation callback. - */ - ScriptDocValues(BiConsumer deprecationCallback) { - this.deprecationCallback = deprecationCallback; - } - /** * Set the current doc ID. */ public abstract void setNextDocId(int docId) throws IOException; - /** - * Return a copy of the list of the values for the current document. - */ - public final List 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; - } - // Throw meaningful exceptions if someone tries to modify the ScriptDocValues. @Override public final void add(int index, T element) { @@ -112,21 +78,6 @@ public final void sort(Comparator 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() { - @Override - public Void run() { - deprecationCallback.accept(key, message); - return null; - } - }); - } - public static final class Longs extends ScriptDocValues { private final SortedNumericDocValues in; private long[] values = new long[0]; @@ -139,14 +90,6 @@ public Longs(SortedNumericDocValues in) { this.in = in; } - /** - * Constructor for testing deprecation callback. - */ - Longs(SortedNumericDocValues in, BiConsumer deprecationCallback) { - super(deprecationCallback); - this.in = in; - } - @Override public void setNextDocId(int docId) throws IOException { if (in.advanceExact(docId)) { @@ -204,14 +147,6 @@ public Dates(SortedNumericDocValues in) { this.in = in; } - /** - * Constructor for testing deprecation callback. - */ - Dates(SortedNumericDocValues in, BiConsumer deprecationCallback) { - super(deprecationCallback); - this.in = in; - } - /** * Fetch the first field value or 0 millis after epoch if there are no * in. @@ -330,14 +265,6 @@ public GeoPoints(MultiGeoPointValues in) { this.in = in; } - /** - * Constructor for testing deprecation callback. - */ - GeoPoints(MultiGeoPointValues in, BiConsumer deprecationCallback) { - super(deprecationCallback); - this.in = in; - } - @Override public void setNextDocId(int docId) throws IOException { if (in.advanceExact(docId)) { @@ -379,7 +306,7 @@ public double getLat() { } public double[] getLats() { - List points = getValues(); + List points = this; double[] lats = new double[points.size()]; for (int i = 0; i < points.size(); i++) { lats[i] = points.get(i).lat(); @@ -388,7 +315,7 @@ public double[] getLats() { } public double[] getLons() { - List points = getValues(); + List points = this; double[] lons = new double[points.size()]; for (int i = 0; i < points.size(); i++) { lons[i] = points.get(i).lon(); diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java deleted file mode 100644 index 42f5470ee89fc..0000000000000 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java +++ /dev/null @@ -1,93 +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.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 keys = new HashSet<>(); - Set 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(){ - 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 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); - } -} diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java index 2924694c10274..72d890edc795d 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java @@ -21,22 +21,10 @@ 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 { @@ -83,51 +71,18 @@ public void testGeoGetLatLon() throws IOException { final double lon1 = randomLon(); final double lon2 = randomLon(); - Set warnings = new HashSet<>(); - Set keys = new HashSet<>(); - final MultiGeoPointValues values = wrap(new GeoPoint(lat1, lon1), new GeoPoint(lat2, lon2)); - final ScriptDocValues.GeoPoints script = geoPointsWrap(values, (deprecationKey, deprecationMessage) -> { - keys.add(deprecationKey); - warnings.add(deprecationMessage); + final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values); - // 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); assertEquals(false, script.isEmpty()); assertEquals(new GeoPoint(lat1, lon1), script.getValue()); - assertEquals(Arrays.asList(new GeoPoint(lat1, lon1), new GeoPoint(lat2, lon2)), script.getValues()); assertEquals(lat1, script.getLat(), 0); 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(){ - 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 { @@ -155,9 +110,4 @@ public void testGeoDistance() throws IOException { script.planeDistanceWithDefault(otherLat, otherLon, 42) / 1000d, 0.01); assertEquals(42, emptyScript.planeDistanceWithDefault(otherLat, otherLon, 42), 0); } - - private GeoPoints geoPointsWrap(MultiGeoPointValues in, BiConsumer deprecationHandler) { - return new GeoPoints(in, deprecationHandler); - } - } diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java index fd2a18a1662f1..a5674e4da7d7d 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java @@ -23,17 +23,7 @@ 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.HashSet; -import java.util.Set; -import java.util.function.BiConsumer; -import static org.hamcrest.Matchers.contains; public class ScriptDocValuesLongsTests extends ESTestCase { public void testLongs() throws IOException { @@ -45,16 +35,7 @@ public void testLongs() throws IOException { } } - Set warnings = new HashSet<>(); - Set keys = new HashSet<>(); - - Longs longs = wrap(values, (deprecationKey, deprecationMessage) -> { - keys.add(deprecationKey); - warnings.add(deprecationMessage); - - // Create a temporary directory to prove we are running with the server's permissions. - createTempDir(); - }); + Longs longs = wrap(values); for (int round = 0; round < 10; round++) { int d = between(0, values.length - 1); @@ -67,42 +48,16 @@ public void testLongs() throws IOException { "Use doc[].size()==0 to check if a document is missing a field!", e.getMessage()); } assertEquals(values[d].length, longs.size()); - assertEquals(values[d].length, longs.getValues().size()); for (int i = 0; i < values[d].length; i++) { assertEquals(values[d][i], longs.get(i).longValue()); - assertEquals(values[d][i], longs.getValues().get(i).longValue()); } - Exception e = expectThrows(UnsupportedOperationException.class, () -> longs.getValues().add(100L)); + Exception e = expectThrows(UnsupportedOperationException.class, () -> longs.add(100L)); assertEquals("doc values are unmodifiable", e.getMessage()); } - - /* - * 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(){ - public Void run() { - longs.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 Longs wrap(long[][] values, BiConsumer deprecationCallback) { + private Longs wrap(long[][] values) { return new Longs(new AbstractSortedNumericDocValues() { long[] current; int i; @@ -121,6 +76,6 @@ public int docValueCount() { public long nextValue() { return current[i++]; } - }, deprecationCallback); + }); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java index 3013b59edcee1..3311a3507f79f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/MinDocCountIT.java @@ -84,19 +84,19 @@ protected Map, Object>> pluginScripts() { scripts.put("doc['d']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Doubles value = (ScriptDocValues.Doubles) doc.get("d"); - return value.getValues(); + return value; }); scripts.put("doc['l']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Longs value = (ScriptDocValues.Longs) doc.get("l"); - return value.getValues(); + return value; }); scripts.put("doc['s']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Strings value = (ScriptDocValues.Strings) doc.get("s"); - return value.getValues(); + return value; }); return scripts; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java index 7508cd4e83976..34d7c1a5cacce 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java @@ -87,7 +87,7 @@ protected Map, Object>> pluginScripts() { scripts.put("doc['" + MULTI_VALUED_FIELD_NAME + "']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Longs value = (ScriptDocValues.Longs) doc.get(MULTI_VALUED_FIELD_NAME); - return value.getValues(); + return value; }); return scripts; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java index b3213306da3d7..64acb3338a709 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityIT.java @@ -75,7 +75,7 @@ protected Map, Object>> pluginScripts() { scripts.put("doc['str_values']", vars -> { Map doc = (Map) vars.get("doc"); ScriptDocValues.Strings strValue = (ScriptDocValues.Strings) doc.get("str_values"); - return strValue.getValues(); + return strValue; }); scripts.put("doc[' + singleNumericField() + '].value", vars -> { @@ -85,7 +85,7 @@ protected Map, Object>> pluginScripts() { scripts.put("doc[' + multiNumericField(false) + ']", vars -> { Map doc =(Map) vars.get("doc"); - return ((ScriptDocValues) doc.get(multiNumericField(false))).getValues(); + return (ScriptDocValues) doc.get(multiNumericField(false)); }); return scripts; diff --git a/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java b/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java index 72ea99df8ff58..babd985fefa63 100644 --- a/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java +++ b/server/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java @@ -156,7 +156,7 @@ static Object sourceScript(Map vars, String path) { static Object docScript(Map vars, String fieldName) { Map doc = (Map) vars.get("doc"); ScriptDocValues values = (ScriptDocValues) doc.get(fieldName); - return values.getValues(); + return values; } } diff --git a/server/src/test/java/org/elasticsearch/search/functionscore/ExplainableScriptIT.java b/server/src/test/java/org/elasticsearch/search/functionscore/ExplainableScriptIT.java index c4b085e84cfb2..76b4f3e93469b 100644 --- a/server/src/test/java/org/elasticsearch/search/functionscore/ExplainableScriptIT.java +++ b/server/src/test/java/org/elasticsearch/search/functionscore/ExplainableScriptIT.java @@ -108,7 +108,7 @@ public Explanation explain(Explanation subQueryScore) throws IOException { @Override public double execute() { - return ((Number) ((ScriptDocValues) getDoc().get("number_field")).getValues().get(0)).doubleValue(); + return ((Number) ((ScriptDocValues) getDoc().get("number_field")).get(0)).doubleValue(); } } diff --git a/server/src/test/java/org/elasticsearch/search/functionscore/RandomScoreFunctionIT.java b/server/src/test/java/org/elasticsearch/search/functionscore/RandomScoreFunctionIT.java index 8203dac1a2dcd..fcd3f0bd7e9d5 100644 --- a/server/src/test/java/org/elasticsearch/search/functionscore/RandomScoreFunctionIT.java +++ b/server/src/test/java/org/elasticsearch/search/functionscore/RandomScoreFunctionIT.java @@ -85,7 +85,7 @@ protected Map, Object>> pluginScripts() { static Double scoringScript(Map vars, Function scoring) { Map doc = (Map) vars.get("doc"); - Double index = ((Number) ((ScriptDocValues) doc.get("index")).getValues().get(0)).doubleValue(); + Double index = ((Number) ((ScriptDocValues) doc.get("index")).get(0)).doubleValue(); Double score = scoring.apply((ScoreAccessor) vars.get("_score")).doubleValue(); Integer factor = (Integer) vars.get("factor"); return Math.log(index + (factor * score)); diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java index 352e27e4f4f44..fe370f6719bfa 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortIT.java @@ -95,13 +95,13 @@ protected Map, Object>> pluginScripts() { static Double sortDoubleScript(Map vars) { Map doc = (Map) vars.get("doc"); - Double index = ((Number) ((ScriptDocValues) doc.get("number")).getValues().get(0)).doubleValue(); + Double index = ((Number) ((ScriptDocValues) doc.get("number")).get(0)).doubleValue(); return index; } static String sortStringScript(Map vars) { Map doc = (Map) vars.get("doc"); - String value = ((String) ((ScriptDocValues) doc.get("keyword")).getValues().get(0)); + String value = ((String) ((ScriptDocValues) doc.get("keyword")).get(0)); return value; } } diff --git a/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java b/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java index 82b0608046a74..0476f6584755f 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/sort/SimpleSortIT.java @@ -87,7 +87,7 @@ protected Map, Object>> pluginScripts() { scripts.put("doc['id'][0]", vars -> { Map doc = (Map) vars.get("doc"); - return ((ScriptDocValues.Strings) doc.get("id")).getValues().get(0); + return ((ScriptDocValues.Strings) doc.get("id")).get(0); }); scripts.put("get min long", vars -> getMinValueScript(vars, Long.MAX_VALUE, "lvalue", l -> (Long) l)); @@ -108,7 +108,7 @@ static > T getMinValueScript(Map vars, T T retval = initialValue; Map doc = (Map) vars.get("doc"); ScriptDocValues values = (ScriptDocValues) doc.get(fieldName); - for (Object v : values.getValues()) { + for (Object v : values) { T value = converter.apply(v); retval = (value.compareTo(retval) < 0) ? value : retval; } From 7c4f8cc659338da1b3c9fd682f6fc721da619729 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 3 Dec 2018 12:12:10 -0600 Subject: [PATCH 06/11] Indentation fix. --- .../org/elasticsearch/index/fielddata/ScriptDocValues.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index 2633c6e036158..bb110cb254b1f 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -306,7 +306,7 @@ public double getLat() { } public double[] getLats() { - List points = this; + List points = this; double[] lats = new double[points.size()]; for (int i = 0; i < points.size(); i++) { lats[i] = points.get(i).lat(); @@ -315,7 +315,7 @@ public double[] getLats() { } public double[] getLons() { - List points = this; + List points = this; double[] lons = new double[points.size()]; for (int i = 0; i < points.size(); i++) { lons[i] = points.get(i).lon(); From 8e55a771d68d9cdf2f7bf96a1fdb50f15f565a02 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 3 Dec 2018 14:37:12 -0600 Subject: [PATCH 07/11] Remove unnecessary list construction at previous `getValues()` callsite in ScriptDocValues.GeoPoints. --- .../index/fielddata/ScriptDocValues.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index bb110cb254b1f..c99f2dafd9fcf 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -34,7 +34,6 @@ import java.util.AbstractList; import java.util.Arrays; import java.util.Comparator; -import java.util.List; import java.util.function.UnaryOperator; /** @@ -306,19 +305,17 @@ public double getLat() { } public double[] getLats() { - List points = this; - double[] lats = new double[points.size()]; - for (int i = 0; i < points.size(); i++) { - lats[i] = points.get(i).lat(); + double[] lats = new double[size()]; + for (int i = 0; i < size(); i++) { + lats[i] = get(i).lat(); } return lats; } public double[] getLons() { - List points = this; - double[] lons = new double[points.size()]; - for (int i = 0; i < points.size(); i++) { - lons[i] = points.get(i).lon(); + double[] lons = new double[size()]; + for (int i = 0; i < size(); i++) { + lons[i] = get(i).lon(); } return lons; } From 94b3958de284b470035f02190b8f1cfc0a787605 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Wed, 5 Dec 2018 09:24:58 -0600 Subject: [PATCH 08/11] Update migration doc and add link to `getValue` in ScriptDocValues javadoc. --- docs/reference/migration/migrate_7_0/scripting.asciidoc | 6 ++++++ .../org/elasticsearch/index/fielddata/ScriptDocValues.java | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/reference/migration/migrate_7_0/scripting.asciidoc b/docs/reference/migration/migrate_7_0/scripting.asciidoc index 01d8805c89667..99afca91e0119 100644 --- a/docs/reference/migration/migrate_7_0/scripting.asciidoc +++ b/docs/reference/migration/migrate_7_0/scripting.asciidoc @@ -29,3 +29,9 @@ To check if a document is missing a value, you can use Malformed scripts, either in search templates, ingest pipelines or search requests, return `400 - Bad request` while they would previously return `500 - Internal Server Error`. This also applies for stored scripts. + +[float] +==== getValues() removed + +The `ScriptDocValues#getValues()` method is deprecated in 6.6 and will +be removed in 7.0. Use `doc["foo"]` in place of `doc["foo"].values`. diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index c99f2dafd9fcf..96c46e6864831 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -38,7 +38,7 @@ /** * Script level doc values, the assumption is that any implementation will - * implement a getValue. + * implement a {@link #getValue}. * * Implementations should not internally re-use objects for the values that they * return as a single {@link ScriptDocValues} instance can be reused to return From ea14adf9cca488610bfb2eb7ea8d0891cf80a0ef Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 5 Dec 2018 11:23:30 -0500 Subject: [PATCH 09/11] Fix compile --- .../java/org/elasticsearch/index/fielddata/ScriptDocValues.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index 96c46e6864831..c65dc2e97a85d 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -38,7 +38,7 @@ /** * Script level doc values, the assumption is that any implementation will - * implement a {@link #getValue}. + * implement a {@link Longs#getValue getValue} method. * * Implementations should not internally re-use objects for the values that they * return as a single {@link ScriptDocValues} instance can be reused to return From 10424a0cc8aab8d5b98533fbd0be9f7323133ca3 Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Mon, 10 Dec 2018 16:11:55 -0600 Subject: [PATCH 10/11] Fix javadoc issue --- .../java/org/elasticsearch/index/fielddata/ScriptDocValues.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index 96c46e6864831..c65dc2e97a85d 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -38,7 +38,7 @@ /** * Script level doc values, the assumption is that any implementation will - * implement a {@link #getValue}. + * implement a {@link Longs#getValue getValue} method. * * Implementations should not internally re-use objects for the values that they * return as a single {@link ScriptDocValues} instance can be reused to return From d24aa1538a3a71f2b3190279775644181d52ca3c Mon Sep 17 00:00:00 2001 From: Jeff Hajewski Date: Tue, 11 Dec 2018 11:52:38 -0600 Subject: [PATCH 11/11] Removes ScriptDocValues#getValues usage from painless whitelist. --- .../elasticsearch/painless/spi/org.elasticsearch.txt | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt b/modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt index becf34c49eac9..41adcc28ae95c 100644 --- a/modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt +++ b/modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt @@ -67,13 +67,11 @@ class org.elasticsearch.common.geo.GeoPoint { class org.elasticsearch.index.fielddata.ScriptDocValues$Strings { String get(int) String getValue() - List getValues() } class org.elasticsearch.index.fielddata.ScriptDocValues$Longs { Long get(int) long getValue() - List getValues() } class org.elasticsearch.script.JodaCompatibleZonedDateTime { @@ -163,19 +161,16 @@ class org.elasticsearch.script.JodaCompatibleZonedDateTime { class org.elasticsearch.index.fielddata.ScriptDocValues$Dates { JodaCompatibleZonedDateTime get(int) JodaCompatibleZonedDateTime getValue() - List getValues() } class org.elasticsearch.index.fielddata.ScriptDocValues$Doubles { Double get(int) double getValue() - List getValues() } class org.elasticsearch.index.fielddata.ScriptDocValues$GeoPoints { org.elasticsearch.common.geo.GeoPoint get(int) org.elasticsearch.common.geo.GeoPoint getValue() - List getValues() double getLat() double getLon() double[] getLats() @@ -193,13 +188,11 @@ class org.elasticsearch.index.fielddata.ScriptDocValues$GeoPoints { class org.elasticsearch.index.fielddata.ScriptDocValues$Booleans { Boolean get(int) boolean getValue() - List getValues() } class org.elasticsearch.index.fielddata.ScriptDocValues$BytesRefs { BytesRef get(int) BytesRef getValue() - List getValues() } class org.apache.lucene.util.BytesRef { @@ -213,7 +206,6 @@ class org.apache.lucene.util.BytesRef { class org.elasticsearch.index.mapper.IpFieldMapper$IpFieldType$IpScriptDocValues { String get(int) String getValue() - List getValues() } class org.elasticsearch.search.lookup.FieldLookup { @@ -268,4 +260,4 @@ static_import { int staticAddIntsTest(int, int) from_class org.elasticsearch.painless.StaticTest float staticAddFloatsTest(float, float) from_class org.elasticsearch.painless.FeatureTest int testAddWithState(int, int, int, double) bound_to org.elasticsearch.painless.BindingTest -} \ No newline at end of file +}