From a94e9500ac38b0f4604bc09dcb5601425ae67f7f Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Mon, 1 Apr 2019 06:25:06 -0400 Subject: [PATCH] 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. --- .../index/fielddata/ScriptDocValues.java | 64 +++++++++---------- .../ScriptDocValuesGeoPointsTests.java | 60 ++++++++++++----- .../fielddata/ScriptDocValuesLongsTests.java | 4 ++ 3 files changed, 81 insertions(+), 47 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 6aad80c4421..afd1d9e3684 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -112,15 +112,15 @@ public abstract class ScriptDocValues extends AbstractList { } public long getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].size()==0 to check if a document is missing a field!"); - } - return values[0]; + return get(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[].size()==0 to check if a document is missing a field!"); + } return values[index]; } @@ -151,15 +151,15 @@ public abstract class ScriptDocValues extends AbstractList { * in. */ public JodaCompatibleZonedDateTime getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].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[].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 extends AbstractList { } public double getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].size()==0 to check if a document is missing a field!"); - } - return values[0]; + return get(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[].size()==0 to check if a document is missing a field!"); + } return values[index]; } @@ -297,11 +297,7 @@ public abstract class ScriptDocValues extends AbstractList { } public GeoPoint getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].size()==0 to check if a document is missing a field!"); - } - return values[0]; + return get(0); } public double getLat() { @@ -330,6 +326,10 @@ public abstract class ScriptDocValues extends AbstractList { @Override public GeoPoint get(int index) { + if (count == 0) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].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 extends AbstractList { } public boolean getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].size()==0 to check if a document is missing a field!"); - } - return values[0]; + return get(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[].size()==0 to check if a document is missing a field!"); + } return values[index]; } @@ -492,14 +492,14 @@ public abstract class ScriptDocValues extends AbstractList { @Override public String get(int index) { + if (count == 0) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } 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[].size()==0 to check if a document is missing a field!"); - } return get(0); } } @@ -512,6 +512,10 @@ public abstract class ScriptDocValues extends AbstractList { @Override public BytesRef get(int index) { + if (count == 0) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].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 extends AbstractList { } public BytesRef getValue() { - if (count == 0) { - throw new IllegalStateException("A document doesn't have a value for a field! " + - "Use doc[].size()==0 to check if a document is missing a field!"); - } return get(0); } 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 72d890edc79..085b43a686b 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesGeoPointsTests.java +++ b/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) { - fail(); - } - return points[i++]; + return current[i++]; } @Override public boolean advanceExact(int docId) { - docID = docId; - return points.length > 0; + if (docId < points.length) { + current = points[docId]; + } else { + current = new GeoPoint[0]; + } + i = 0; + return current.length > 0; } @Override public int docValueCount() { - if (docID != 0) { - return 0; - } - return points.length; + return current.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[].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[].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)); + } + } + } + + } 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 a5674e4da7d..c74725d3774 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java +++ b/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[].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[].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++) {