Fix off by one error when handling null values in range fields (elast…

…ic#107977)
kkrik-es · Apr 30, 2024 · a810f87 · a810f87
1 parent 71b1b30
commit a810f87
Show file tree

Hide file tree

Showing 7 changed files with 171 additions and 87 deletions.
diff --git a/docs/changelog/107977.yaml b/docs/changelog/107977.yaml
@@ -0,0 +1,6 @@
+pr: 107977
+summary: Fix off by one error when handling null values in range fields
+area: Mapping
+type: bug
+issues:
+ - 107282
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/range/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/range/10_basic.yml
@@ -445,3 +445,78 @@ setup:
         body: { "size" : 0, "query" : { "range" : { "date_range" : { "lte": "2019-12-15||/d", "relation": "within" } } } }
 
   - match: { hits.total: 2 }
+
+---
+"Null bounds":
+
+  - requires:
+      cluster_features: ["mapper.range.null_values_off_by_one_fix"]
+      reason: fixed in 8.15.0
+
+  - do:
+      index:
+        index: test
+        id: "1"
+        body: { "long_range" : { "gt": null, "lte": 5 } }
+
+  - do:
+      index:
+        index: test
+        id: "2"
+        body: { "long_range" : { "gte": null, "lte": 5 } }
+
+  - do:
+      index:
+        index: test
+        id: "3"
+        body: { "long_range" : { "lte": 5 } }
+
+  - do:
+      index:
+        index: test
+        id: "4"
+        body: { "long_range" : { "gte": 10, "lt": null } }
+
+  - do:
+      index:
+        index: test
+        id: "5"
+        body: { "long_range" : { "gte": 10, "lte": null } }
+
+  - do:
+      index:
+        index: test
+        id: "6"
+        body: { "long_range" : { "gte": 10 } }
+
+
+  - do:
+      indices.refresh: {}
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body: { "size" : 0, "query" : { "term" : { "long_range" : { "value": -9223372036854775808 } } } }
+
+  - match: { hits.total: 2 }
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body: { "size" : 0, "query" : { "term" : { "long_range" : { "value": -9223372036854775807 } } } }
+
+  - match: { hits.total: 3 }
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body: { "size" : 0, "query" : { "term" : { "long_range" : { "value": 9223372036854775807 } } } }
+
+  - match: { hits.total: 2 }
+
+  - do:
+      search:
+        rest_total_hits_as_int: true
+        body: { "size" : 0, "query" : { "term" : { "long_range" : { "value": 9223372036854775806 } } } }
+
+  - match: { hits.total: 3 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
@@ -19,6 +19,6 @@
 public class MapperFeatures implements FeatureSpecification {
     @Override
     public Set<NodeFeature> getFeatures() {
-        return Set.of(IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE);
+        return Set.of(IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE, RangeFieldMapper.NULL_VALUES_OFF_BY_ONE_FIX);
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java
@@ -20,6 +20,7 @@
 import org.elasticsearch.common.time.DateMathParser;
 import org.elasticsearch.common.util.LocaleUtils;
 import org.elasticsearch.core.Tuple;
+import org.elasticsearch.features.NodeFeature;
 import org.elasticsearch.index.fielddata.FieldDataContext;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.plain.BinaryIndexFieldData;
@@ -49,6 +50,8 @@
 
 /** A {@link FieldMapper} for indexing numeric and date ranges, and creating queries */
 public class RangeFieldMapper extends FieldMapper {
+    public static final NodeFeature NULL_VALUES_OFF_BY_ONE_FIX = new NodeFeature("mapper.range.null_values_off_by_one_fix");
+
     public static final boolean DEFAULT_INCLUDE_UPPER = true;
     public static final boolean DEFAULT_INCLUDE_LOWER = true;
 
@@ -381,66 +384,77 @@ protected boolean supportsParsingObject() {
 
     @Override
     protected void parseCreateField(DocumentParserContext context) throws IOException {
-        Range range;
         XContentParser parser = context.parser();
-        final XContentParser.Token start = parser.currentToken();
-        if (start == XContentParser.Token.VALUE_NULL) {
+        if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
             return;
-        } else if (start == XContentParser.Token.START_OBJECT) {
-            RangeFieldType fieldType = fieldType();
-            RangeType rangeType = fieldType.rangeType;
-            String fieldName = null;
-            Object from = rangeType.minValue();
-            Object to = rangeType.maxValue();
-            boolean includeFrom = DEFAULT_INCLUDE_LOWER;
-            boolean includeTo = DEFAULT_INCLUDE_UPPER;
-            XContentParser.Token token;
-            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
-                if (token == XContentParser.Token.FIELD_NAME) {
-                    fieldName = parser.currentName();
-                } else {
-                    if (fieldName.equals(GT_FIELD.getPreferredName())) {
-                        includeFrom = false;
-                        if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
-                            from = rangeType.parseFrom(fieldType, parser, coerce.value(), includeFrom);
-                        }
-                    } else if (fieldName.equals(GTE_FIELD.getPreferredName())) {
-                        includeFrom = true;
-                        if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
-                            from = rangeType.parseFrom(fieldType, parser, coerce.value(), includeFrom);
-                        }
-                    } else if (fieldName.equals(LT_FIELD.getPreferredName())) {
-                        includeTo = false;
-                        if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
-                            to = rangeType.parseTo(fieldType, parser, coerce.value(), includeTo);
-                        }
-                    } else if (fieldName.equals(LTE_FIELD.getPreferredName())) {
-                        includeTo = true;
-                        if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
-                            to = rangeType.parseTo(fieldType, parser, coerce.value(), includeTo);
-                        }
-                    } else {
-                        throw new DocumentParsingException(
-                            parser.getTokenLocation(),
-                            "error parsing field [" + name() + "], with unknown parameter [" + fieldName + "]"
-                        );
-                    }
-                }
-            }
-            range = new Range(rangeType, from, to, includeFrom, includeTo);
-        } else if (fieldType().rangeType == RangeType.IP && start == XContentParser.Token.VALUE_STRING) {
-            range = parseIpRangeFromCidr(parser);
-        } else {
+        }
+
+        Range range = parseRange(parser);
+        context.doc().addAll(fieldType().rangeType.createFields(context, name(), range, index, hasDocValues, store));
+
+        if (hasDocValues == false && (index || store)) {
+            context.addToFieldNames(fieldType().name());
+        }
+    }
+
+    private Range parseRange(XContentParser parser) throws IOException {
+        final XContentParser.Token start = parser.currentToken();
+        if (fieldType().rangeType == RangeType.IP && start == XContentParser.Token.VALUE_STRING) {
+            return parseIpRangeFromCidr(parser);
+        }
+
+        if (start != XContentParser.Token.START_OBJECT) {
             throw new DocumentParsingException(
                 parser.getTokenLocation(),
                 "error parsing field [" + name() + "], expected an object but got " + parser.currentName()
             );
         }
-        context.doc().addAll(fieldType().rangeType.createFields(context, name(), range, index, hasDocValues, store));
 
-        if (hasDocValues == false && (index || store)) {
-            context.addToFieldNames(fieldType().name());
+        RangeFieldType fieldType = fieldType();
+        RangeType rangeType = fieldType.rangeType;
+        String fieldName = null;
+        Object parsedFrom = null;
+        Object parsedTo = null;
+        boolean includeFrom = DEFAULT_INCLUDE_LOWER;
+        boolean includeTo = DEFAULT_INCLUDE_UPPER;
+        XContentParser.Token token;
+        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+            if (token == XContentParser.Token.FIELD_NAME) {
+                fieldName = parser.currentName();
+            } else {
+                if (fieldName.equals(GT_FIELD.getPreferredName())) {
+                    includeFrom = false;
+                    if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
+                        parsedFrom = rangeType.parseFrom(fieldType, parser, coerce.value(), includeFrom);
+                    }
+                } else if (fieldName.equals(GTE_FIELD.getPreferredName())) {
+                    includeFrom = true;
+                    if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
+                        parsedFrom = rangeType.parseFrom(fieldType, parser, coerce.value(), includeFrom);
+                    }
+                } else if (fieldName.equals(LT_FIELD.getPreferredName())) {
+                    includeTo = false;
+                    if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
+                        parsedTo = rangeType.parseTo(fieldType, parser, coerce.value(), includeTo);
+                    }
+                } else if (fieldName.equals(LTE_FIELD.getPreferredName())) {
+                    includeTo = true;
+                    if (parser.currentToken() != XContentParser.Token.VALUE_NULL) {
+                        parsedTo = rangeType.parseTo(fieldType, parser, coerce.value(), includeTo);
+                    }
+                } else {
+                    throw new DocumentParsingException(
+                        parser.getTokenLocation(),
+                        "error parsing field [" + name() + "], with unknown parameter [" + fieldName + "]"
+                    );
+                }
+            }
         }
+
+        Object from = parsedFrom != null ? parsedFrom : rangeType.defaultFrom(includeFrom);
+        Object to = parsedTo != null ? parsedTo : rangeType.defaultTo(includeTo);
+
+        return new Range(rangeType, from, to, includeFrom, includeTo);
     }
 
     private static Range parseIpRangeFromCidr(final XContentParser parser) throws IOException {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java
@@ -844,6 +844,15 @@ public Object parseTo(RangeFieldMapper.RangeFieldType fieldType, XContentParser
         return included ? value : (Number) nextDown(value);
     }
 
+    public Object defaultFrom(boolean included) {
+        return included ? minValue() : nextUp(minValue());
+
+    }
+
+    public Object defaultTo(boolean included) {
+        return included ? maxValue() : nextDown(maxValue());
+    }
+
     public abstract Object minValue();
 
     public abstract Object maxValue();

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IpRangeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IpRangeFieldMapperTests.java
@@ -143,27 +143,13 @@ private Tuple<Object, Object> generateValue() {
             var to = rarely() ? null : InetAddresses.toAddrString(range.upperBound());
             input = (ToXContent) (builder, params) -> builder.startObject().field(fromKey, from).field(toKey, to).endObject();
 
-            // When ranges are stored, they are always normalized to include both ends.
-            // `includeFrom` and `includeTo` here refers to user input.
-            //
-            // Range values are not properly normalized for default values
-            // which results in off by one error here.
-            // So "gte": null and "gt": null both result in "gte": MIN_VALUE.
-            // This is a bug, see #107282.
-            if (from == null) {
-                output.put("gte", InetAddresses.toAddrString((InetAddress) rangeType().minValue()));
-            } else {
-                var rawFrom = range.lowerBound();
-                var adjustedFrom = includeFrom ? rawFrom : (InetAddress) RangeType.IP.nextUp(rawFrom);
-                output.put("gte", InetAddresses.toAddrString(adjustedFrom));
-            }
-            if (to == null) {
-                output.put("lte", InetAddresses.toAddrString((InetAddress) rangeType().maxValue()));
-            } else {
-                var rawTo = range.upperBound();
-                var adjustedTo = includeTo ? rawTo : (InetAddress) RangeType.IP.nextDown(rawTo);
-                output.put("lte", InetAddresses.toAddrString(adjustedTo));
-            }
+            var rawFrom = from != null ? range.lowerBound() : (InetAddress) rangeType().minValue();
+            var adjustedFrom = includeFrom ? rawFrom : (InetAddress) rangeType().nextUp(rawFrom);
+            output.put("gte", InetAddresses.toAddrString(adjustedFrom));
+
+            var rawTo = to != null ? range.upperBound() : (InetAddress) rangeType().maxValue();
+            var adjustedTo = includeTo ? rawTo : (InetAddress) rangeType().nextDown(rawTo);
+            output.put("lte", InetAddresses.toAddrString(adjustedTo));
         }
 
         return Tuple.tuple(input, output);

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java
@@ -339,24 +339,18 @@ Object toExpectedSyntheticSource() {
             // Also, "to" field always comes first.
             Map<String, Object> output = new LinkedHashMap<>();
 
-            // Range values are not properly normalized for default values
-            // which results in off by one error here.
-            // So "gte": null and "gt": null both result in "gte": MIN_VALUE.
-            // This is a bug, see #107282.
-            if (from == null) {
-                output.put("gte", rangeType().minValue());
-            } else if (includeFrom) {
-                output.put("gte", from);
+            var fromWithDefaults = from != null ? from : rangeType().minValue();
+            if (includeFrom) {
+                output.put("gte", fromWithDefaults);
             } else {
-                output.put("gte", type.nextUp(from));
+                output.put("gte", type.nextUp(fromWithDefaults));
             }
 
-            if (to == null) {
-                output.put("lte", rangeType().maxValue());
-            } else if (includeTo) {
-                output.put("lte", to);
+            var toWithDefaults = to != null ? to : rangeType().maxValue();
+            if (includeTo) {
+                output.put("lte", toWithDefaults);
             } else {
-                output.put("lte", type.nextDown(to));
+                output.put("lte", type.nextDown(toWithDefaults));
             }
 
             return output;