Handle missing values in painless

Throw an exception for `doc['field'].value` if this document is missing a value for the `field`. For 7.0: This is the default behaviour from 7.0 For 6.x: To enable this behavior from 6.x, a user can set a jvm.option: `-Des.script.exception_for_missing_value=true` on a node. If a user does not enable this behavior, a deprecation warning is logged on start up. Closes #29286
elastic · Jun 4, 2018 · ca9f0a5 · ca9f0a5
1 parent 30a8f9d
commit ca9f0a5
Show file tree

Hide file tree

Showing 13 changed files with 115 additions and 23 deletions.
diff --git a/docs/painless/painless-getting-started.asciidoc b/docs/painless/painless-getting-started.asciidoc
@@ -119,6 +119,27 @@ GET hockey/_search
 ----------------------------------------------------------------
 // CONSOLE
 
+
+[float]
+===== Missing values
+
+If you request the value from a field `field` that isn’t in
+the document, `doc['field'].value` for this document returns:
+
+- `0` if a `field` has a numeric datatype (long, double etc.)
+- `false` is a `field` has a boolean datatype
+- epoch date if a `field` has a date datatype
+- `null` if a `field` has a string datatype
+- `null` if a `field` has a geo datatype
+- `""` if a `field` has a binary datatype
+
+IMPORTANT: Starting in 7.0, `doc['field'].value` throws an exception if
+the field is missing in a document. To enable this behavior now,
+set a <<jvm-options,`jvm.option`>>
+`-Des.script.exception_for_missing_value=true` on a node. If you do not enable
+this behavior, a deprecation warning is logged on start up.
+
+
 [float]
 ==== Updating Fields with Painless
 

diff --git a/modules/lang-painless/build.gradle b/modules/lang-painless/build.gradle
@@ -17,7 +17,7 @@
  * under the License.
  */
 
-import org.apache.tools.ant.types.Path
+import org.elasticsearch.gradle.test.RestIntegTestTask
 
 esplugin {
   description 'An easy, safe and fast scripting language for Elasticsearch'
@@ -28,6 +28,15 @@ integTestCluster {
   module project.project(':modules:mapper-extras')
 }
 
+Task additionalClusterTest = tasks.create(
+        name: "additionalClusterTest", type: RestIntegTestTask)
+additionalClusterTestCluster {
+  systemProperty 'es.script.exception_for_missing_value', 'true'
+  distribution = 'integ-test-zip'
+  module project // add the lang-painless module itself
+  module project.project(':modules:mapper-extras')
+}
+
 dependencies {
   compile 'org.antlr:antlr4-runtime:4.5.3'
   compile 'org.ow2.asm:asm-debug-all:5.1'

diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml
@@ -88,14 +88,13 @@ setup:
 
 ---
 "Scripted Field with a null safe dereference (null)":
-    # Change this to ?: once we have it implemented
     - do:
         search:
             body:
                 script_fields:
                     bar:
                         script:
-                            source: "(doc['missing'].value?.length() ?: 0) + params.x;"
+                            source: "(doc['missing'].size() == 0 ? 0 : doc['missing'].value?.length()) + params.x;"
                             params:
                                 x: 5
 

diff --git a/modules/mapper-extras/build.gradle b/modules/mapper-extras/build.gradle
@@ -21,3 +21,6 @@ esplugin {
     description 'Adds advanced field mappers'
     classname 'org.elasticsearch.index.mapper.MapperExtrasPlugin'
 }
+test.configure {
+    systemProperty 'es.script.exception_for_missing_value', 'true'
+}
diff --git a/modules/parent-join/build.gradle b/modules/parent-join/build.gradle
@@ -22,3 +22,6 @@ esplugin {
   classname 'org.elasticsearch.join.ParentJoinPlugin'
   hasClientJar = true
 }
+test.configure {
+  systemProperty 'es.script.exception_for_missing_value', 'true'
+}
diff --git a/modules/percolator/build.gradle b/modules/percolator/build.gradle
@@ -36,3 +36,7 @@ dependencyLicenses {
 
 compileJava.options.compilerArgs << "-Xlint:-deprecation,-rawtypes"
 compileTestJava.options.compilerArgs << "-Xlint:-deprecation,-rawtypes"
+
+test.configure {
+    systemProperty 'es.script.exception_for_missing_value', 'true'
+}
diff --git a/server/build.gradle b/server/build.gradle
@@ -338,3 +338,7 @@ if (isEclipse == false || project.path == ":server-tests") {
   check.dependsOn integTest
   integTest.mustRunAfter test
 }
+
+test.configure {
+  systemProperty 'es.script.exception_for_missing_value', 'true'
+}
diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.logging.ESLoggerFactory;
+import org.elasticsearch.script.ScriptModule;
 import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
 import org.joda.time.MutableDateTime;
@@ -128,6 +129,10 @@ protected void resize(int newSize) {
 
         public long getValue() {
             if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
                 return 0L;
             }
             return values[0];
@@ -170,6 +175,10 @@ public Dates(SortedNumericDocValues in) {
          */
         public ReadableDateTime getValue() {
             if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
                 return EPOCH;
             }
             return get(0);
@@ -271,6 +280,10 @@ public SortedNumericDoubleValues getInternalValues() {
 
         public double getValue() {
             if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
                 return 0d;
             }
             return values[0];
@@ -327,6 +340,10 @@ protected void resize(int newSize) {
 
         public GeoPoint getValue() {
             if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
                 return null;
             }
             return values[0];
@@ -439,7 +456,14 @@ protected void resize(int newSize) {
         }
 
         public boolean getValue() {
-            return count != 0 && values[0];
+            if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
+                return false;
+            }
+            return values[0];
         }
 
         @Override
@@ -522,7 +546,14 @@ public String get(int index) {
         }
 
         public String getValue() {
-            return count == 0 ? null : get(0);
+            if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
+                return null;
+            }
+            return get(0);
         }
     }
 
@@ -543,7 +574,14 @@ public BytesRef get(int index) {
         }
 
         public BytesRef getValue() {
-            return count == 0 ? new BytesRef() : get(0);
+            if (count == 0) {
+                if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
+                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
+                }
+                return new BytesRef();
+            }
+            return get(0);
         }
 
     }

diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java
@@ -31,6 +31,9 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.plugins.ScriptPlugin;
 import org.elasticsearch.search.aggregations.pipeline.movfn.MovingFunctionScript;
+import org.elasticsearch.common.Booleans;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 
 /**
  * Manages building {@link ScriptService}.
@@ -57,6 +60,11 @@ public class ScriptModule {
         ).collect(Collectors.toMap(c -> c.name, Function.identity()));
     }
 
+    public static final boolean EXCEPTION_FOR_MISSING_VALUE =
+        Booleans.parseBoolean(System.getProperty("es.script.exception_for_missing_value", "false"));
+
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ScriptModule.class));
+
     private final ScriptService scriptService;
 
     public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
@@ -80,6 +88,10 @@ public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
                 }
             }
         }
+        if (EXCEPTION_FOR_MISSING_VALUE == false)
+            DEPRECATION_LOGGER.deprecated("Script: returning default values for missing document values is deprecated. " +
+                    "Set system property '-Des.script.exception_for_missing_value=true' " +
+                    "to make behaviour compatible with future major versions.");
         scriptService = new ScriptService(settings, Collections.unmodifiableMap(engines), Collections.unmodifiableMap(contexts));
     }
 

diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java
@@ -54,7 +54,13 @@ public void test() throws IOException {
         for (int round = 0; round < 10; round++) {
             int d = between(0, values.length - 1);
             dates.setNextDocId(d);
-            assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getValue());
+            if (expectedDates[d].length > 0) {
+                assertEquals(expectedDates[d][0] , dates.getValue());
+            } else {
+                Exception e = expectThrows(IllegalStateException.class, () -> dates.getValue());
+                assertEquals("A document doesn't have a value for a field! " +
+                    "Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
+            }
 
             assertEquals(values[d].length, dates.size());
             for (int i = 0; i < values[d].length; i++) {

diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java
@@ -21,22 +21,8 @@
 
 import org.elasticsearch.index.fielddata.ScriptDocValues.Longs;
 import org.elasticsearch.test.ESTestCase;
-import org.joda.time.DateTime;
-import org.joda.time.DateTimeZone;
-import org.joda.time.ReadableDateTime;
 
 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.Consumer;
-
-import static org.hamcrest.Matchers.containsInAnyOrder;
 
 public class ScriptDocValuesLongsTests extends ESTestCase {
     public void testLongs() throws IOException {
@@ -52,8 +38,13 @@ public void testLongs() throws IOException {
         for (int round = 0; round < 10; round++) {
             int d = between(0, values.length - 1);
             longs.setNextDocId(d);
-            assertEquals(values[d].length > 0 ? values[d][0] : 0, longs.getValue());
-
+            if (values[d].length > 0) {
+                assertEquals(values[d][0], longs.getValue());
+            } else {
+                Exception e = expectThrows(IllegalStateException.class, () -> longs.getValue());
+                assertEquals("A document doesn't have a value for a field! " +
+                    "Use doc[<field>].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++) {

diff --git a/test/framework/build.gradle b/test/framework/build.gradle
@@ -73,4 +73,5 @@ precommit.dependsOn namingConventionsMain
 test.configure {
   systemProperty 'tests.gradle_index_compat_versions', bwcVersions.indexCompatible.join(',')
   systemProperty 'tests.gradle_wire_compat_versions', bwcVersions.wireCompatible.join(',')
+  systemProperty 'es.script.exception_for_missing_value', 'true'
 }
diff --git a/x-pack/plugin/security/build.gradle b/x-pack/plugin/security/build.gradle
@@ -225,6 +225,7 @@ test {
      * other if we allow them to set the number of available processors as it's set-once in Netty.
      */
     systemProperty 'es.set.netty.runtime.available.processors', 'false'
+    systemProperty 'es.script.exception_for_missing_value', 'true'
 }
 
 // xpack modules are installed in real clusters as the meta plugin, so