Handle missing values in painless (elastic#30975)

* 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 elastic#29286

Author: mayya-sharipova

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -691,6 +691,7 @@ class BuildPlugin implements Plugin<Project> {
             systemProperty 'tests.task', path
             systemProperty 'tests.security.manager', 'true'
             systemProperty 'jna.nosys', 'true'
+            systemProperty 'es.scripting.exception_for_missing_value', 'true'
             // TODO: remove setting logging level via system property
             systemProperty 'tests.logger.level', 'WARN'
             for (Map.Entry<String, String> property : System.properties.entrySet()) {

docs/painless/painless-getting-started.asciidoc

@@ -119,6 +119,30 @@ 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.scripting.exception_for_missing_value=true` on a node. If you do not enable
+this behavior, a deprecation warning is logged on start up.
+
+To check if a document is missing a value, you can call
+`doc['field'].size() == 0`.
+
+
 [float]
 ==== Updating Fields with Painless
 

modules/lang-painless/build.gradle

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-import org.apache.tools.ant.types.Path
+
 
 esplugin {
   description 'An easy, safe and fast scripting language for Elasticsearch'

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
 

server/build.gradle

@@ -329,7 +329,7 @@ if (isEclipse == false || project.path == ":server-tests") {
   task integTest(type: RandomizedTestingTask,
                  group: JavaBasePlugin.VERIFICATION_GROUP,
                  description: 'Multi-node tests',
-                 dependsOn: test.dependsOn) {
+                 dependsOn: test.dependsOn.collect()) {
     configure(BuildPlugin.commonTestConfig(project))
     classpath = project.test.classpath
     testClassesDirs = project.test.testClassesDirs
@@ -338,3 +338,13 @@ if (isEclipse == false || project.path == ":server-tests") {
   check.dependsOn integTest
   integTest.mustRunAfter test
 }
+
+// TODO: remove ScriptDocValuesMissingV6BehaviourTests in 7.0
+additionalTest('testScriptDocValuesMissingV6Behaviour'){
+  include '**/ScriptDocValuesMissingV6BehaviourTests.class'
+  systemProperty 'es.scripting.exception_for_missing_value', 'false'
+}
+test {
+  // these are tested explicitly in separate test tasks
+  exclude '**/*ScriptDocValuesMissingV6BehaviourTests.class'
+}

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;
@@ -125,6 +126,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];
@@ -167,6 +172,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);
@@ -268,6 +277,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];
@@ -324,6 +337,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];
@@ -436,7 +453,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
@@ -519,7 +543,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);
         }
     }
 
@@ -540,7 +571,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);
         }
 
     }

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}.
@@ -61,6 +64,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.scripting.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) {
@@ -84,6 +92,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.scripting.exception_for_missing_value=true' " +
+                    "to make behaviour compatible with future major versions.");
         scriptService = new ScriptService(settings, Collections.unmodifiableMap(engines), Collections.unmodifiableMap(contexts));
     }
 

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

@@ -44,7 +44,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++) {

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

@@ -24,7 +24,6 @@
 
 import java.io.IOException;
 
-
 public class ScriptDocValuesLongsTests extends ESTestCase {
     public void testLongs() throws IOException {
         long[][] values = new long[between(3, 10)][];
@@ -39,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++) {