Correct bug in ScriptDocValues (#40488)

If a field `field_name` was missing in a document,
doc['field_name'].get(0) incorrectly retrieved
a value of the previously accessed document.
This happened because `get(int index)` function
was just accessing `values[index]` without
checking the number of values - `count`.

This PR fixes this.

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

@@ -112,15 +112,15 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
         }
 
         public long getValue() {
-            if (count == 0) {
+            return get(0);
-                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 values[0];
         }
 
         @Override
         public Long get(int index) {
+            if (count == 0) {
+                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 values[index];
         }
 
@@ -151,15 +151,15 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
          * in.
          */
         public JodaCompatibleZonedDateTime getValue() {
-            if (count == 0) {
-                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 get(0);
         }
 
         @Override
         public JodaCompatibleZonedDateTime get(int index) {
+            if (count == 0) {
+                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!");
+            }
             if (index >= count) {
                 throw new IndexOutOfBoundsException(
                         "attempted to fetch the [" + index + "] date when there are only ["
@@ -240,15 +240,15 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
         }
 
         public double getValue() {
-            if (count == 0) {
+            return get(0);
-               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 values[0];
         }
 
         @Override
         public Double get(int index) {
+            if (count == 0) {
+                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 values[index];
         }
 
@@ -297,11 +297,7 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
         }
 
         public GeoPoint getValue() {
-            if (count == 0) {
+            return get(0);
-                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 values[0];
         }
 
         public double getLat() {
@@ -330,6 +326,10 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         @Override
         public GeoPoint get(int index) {
+            if (count == 0) {
+                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!");
+            }
             final GeoPoint point = values[index];
             return new GeoPoint(point.lat(), point.lon());
         }
@@ -409,15 +409,15 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
         }
 
         public boolean getValue() {
-            if (count == 0) {
+            return get(0);
-                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 values[0];
         }
 
         @Override
         public Boolean get(int index) {
+            if (count == 0) {
+                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 values[index];
         }
 
@@ -492,14 +492,14 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         @Override
         public String get(int index) {
-            return values[index].get().utf8ToString();
-        }
-
-        public String getValue() {
             if (count == 0) {
                 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 values[index].get().utf8ToString();
+        }
+
+        public String getValue() {
             return get(0);
         }
     }
@@ -512,6 +512,10 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         @Override
         public BytesRef get(int index) {
+            if (count == 0) {
+                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!");
+            }
             /**
              * We need to make a copy here because {@link BinaryScriptDocValues} might reuse the
              * returned value and the same instance might be used to
@@ -521,10 +525,6 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
         }
 
         public BytesRef getValue() {
-            if (count == 0) {
-                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 get(0);
         }
 

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

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.index.fielddata;
 
+import org.elasticsearch.index.fielddata.ScriptDocValues.GeoPoints;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.test.ESTestCase;
@@ -28,31 +29,30 @@ import java.util.Arrays;
 
 public class ScriptDocValuesGeoPointsTests extends ESTestCase {
 
-    private static MultiGeoPointValues wrap(final GeoPoint... points) {
+    private static MultiGeoPointValues wrap(GeoPoint[][] points) {
         return new MultiGeoPointValues() {
-            int docID = -1;
+            GeoPoint[] current;
             int i;
 
             @Override
             public GeoPoint nextValue() {
-                if (docID != 0) {
+                return current[i++];
-                    fail();
-                }
-                return points[i++];
             }
 
             @Override
             public boolean advanceExact(int docId) {
-                docID = docId;
+                if (docId < points.length) {
-                return points.length > 0;
+                    current = points[docId];
+                } else {
+                    current = new GeoPoint[0];
+                }
+                i = 0;
+                return current.length > 0;
             }
 
             @Override
             public int docValueCount() {
-                if (docID != 0) {
+                return current.length;
-                    return 0;
-                }
-                return points.length;
             }
         };
     }
@@ -71,7 +71,8 @@ public class ScriptDocValuesGeoPointsTests extends ESTestCase {
         final double lon1 = randomLon();
         final double lon2 = randomLon();
 
-        final MultiGeoPointValues values = wrap(new GeoPoint(lat1, lon1), new GeoPoint(lat2, lon2));
+        GeoPoint[][] points = {{new GeoPoint(lat1, lon1), new GeoPoint(lat2, lon2)}};
+        final MultiGeoPointValues values = wrap(points);
         final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values);
 
         script.setNextDocId(1);
@@ -88,11 +89,13 @@ public class ScriptDocValuesGeoPointsTests extends ESTestCase {
     public void testGeoDistance() throws IOException {
         final double lat = randomLat();
         final double lon = randomLon();
-        final MultiGeoPointValues values = wrap(new GeoPoint(lat, lon));
+        GeoPoint[][] points = {{new GeoPoint(lat, lon)}};
+        final MultiGeoPointValues values = wrap(points);
         final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values);
         script.setNextDocId(0);
 
-        final ScriptDocValues.GeoPoints emptyScript = new ScriptDocValues.GeoPoints(wrap());
+        GeoPoint[][] points2 = {new GeoPoint[0]};
+        final ScriptDocValues.GeoPoints emptyScript = new ScriptDocValues.GeoPoints(wrap(points2));
         emptyScript.setNextDocId(0);
 
         final double otherLat = randomLat();
@@ -110,4 +113,31 @@ public class ScriptDocValuesGeoPointsTests extends ESTestCase {
                 script.planeDistanceWithDefault(otherLat, otherLon, 42) / 1000d, 0.01);
         assertEquals(42, emptyScript.planeDistanceWithDefault(otherLat, otherLon, 42), 0);
     }
+
+    public void testMissingValues() throws IOException {
+        GeoPoint[][] points = new GeoPoint[between(3, 10)][];
+        for (int d = 0; d < points.length; d++) {
+            points[d] = new GeoPoint[randomBoolean() ? 0 : between(1, 10)];
+        }
+        final ScriptDocValues.GeoPoints geoPoints = new GeoPoints(wrap(points));
+        for (int d = 0; d < points.length; d++) {
+            geoPoints.setNextDocId(d);
+            if (points[d].length > 0) {
+                assertEquals(points[d][0], geoPoints.getValue());
+            } else {
+                Exception e = expectThrows(IllegalStateException.class, () -> geoPoints.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());
+                e = expectThrows(IllegalStateException.class, () -> geoPoints.get(0));
+                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(points[d].length, geoPoints.size());
+            for (int i = 0; i < points[d].length; i++) {
+                assertEquals(points[d][i], geoPoints.get(i));
+            }
+        }
+    }
+
+
 }

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

@@ -42,10 +42,14 @@ public class ScriptDocValuesLongsTests extends ESTestCase {
             longs.setNextDocId(d);
             if (values[d].length > 0) {
                 assertEquals(values[d][0], longs.getValue());
+                assertEquals(values[d][0], (long) longs.get(0));
             } 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());
+                e = expectThrows(IllegalStateException.class, () -> longs.get(0));
+                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());
             for (int i = 0; i < values[d].length; i++) {