Range Validation

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add getWrappedScorer method to ProfileScorer for plugin access to wrapped scorers ([#20548](https://github.com/opensearch-project/OpenSearch/issues/20548))
 - Support expected cluster name with validation in CCS Sniff mode ([#20532](https://github.com/opensearch-project/OpenSearch/pull/20532))
 - Add security policy to allow `accessUnixDomainSocket` in `transport-grpc` module ([#20463](https://github.com/opensearch-project/OpenSearch/pull/20463))
+- Add range validations in query builder and field mapper ([#20497](https://github.com/opensearch-project/OpenSearch/issues/20497))
 - [Workload Management] Enhance Scroll API support for autotagging ([#20151](https://github.com/opensearch-project/OpenSearch/pull/20151))
 
 ### Changed

server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java

@@ -470,93 +470,106 @@ protected RangeFieldMapper clone() {
 
     @Override
     protected void parseCreateField(ParseContext context) throws IOException {
-        Range range;
-        if (context.externalValueSet()) {
-            range = context.parseExternalValue(Range.class);
-        } else {
-            XContentParser parser = context.parser();
-            final XContentParser.Token start = parser.currentToken();
-            if (start == 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;
-                boolean rangeIsMalformed = false;
-                while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
-                    if (token == XContentParser.Token.FIELD_NAME) {
-                        fieldName = parser.currentName();
-                    } else {
-                        try {
-                            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 MapperParsingException(
-                                    "error parsing field [" + name() + "], with unknown parameter [" + fieldName + "]"
-                                );
-                            }
-                        } catch (final IllegalArgumentException e) {
-                            // We have to consume the JSON object in full
-                            if (ignoreMalformed().value()) {
-                                rangeIsMalformed = true;
-                            } else {
-                                throw e;
-                            }
-                        }
-                    }
-                }
+        Range range = parseRange(context);
+        if (range == null) {
+            return;
+        }
+        context.doc().addAll(fieldType().rangeType.createFields(context, name(), range, index, hasDocValues, store));
 
-                if (rangeIsMalformed) {
+        if (hasDocValues == false && (index || store)) {
+            createFieldNamesField(context);
+        }
+    }
+
+    private Range parseRange(ParseContext context) throws IOException {
+        if (context.externalValueSet()) {
+            return context.parseExternalValue(Range.class);
+        }
+        XContentParser parser = context.parser();
+        final XContentParser.Token start = parser.currentToken();
+        if (start == XContentParser.Token.VALUE_NULL) {
+            return null;
+        }
+        if (fieldType().rangeType == RangeType.IP && start == XContentParser.Token.VALUE_STRING) {
+            try {
+                return parseIpRangeFromCidr(parser);
+            } catch (IllegalArgumentException e) {
+                if (ignoreMalformed().value()) {
                     context.addIgnoredField(fieldType().name());
-                    return;
+                    return null;
+                } else {
+                    throw e;
+                }
+            }
+        }
+        if (start == XContentParser.Token.START_OBJECT) {
+            RangeFieldType fieldType = fieldType();
+            RangeType rangeType = fieldType.rangeType;
+            String fieldName = null;
+            Object from = null;
+            Object to = null;
+            boolean includeFrom = DEFAULT_INCLUDE_LOWER;
+            boolean includeTo = DEFAULT_INCLUDE_UPPER;
+            XContentParser.Token token;
+            boolean rangeIsMalformed = false;
+            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+                if (token == XContentParser.Token.FIELD_NAME) {
+                    fieldName = parser.currentName();
+                    continue; // only field name is required in this iteration
+                }
+                if (fieldName == null) {
+                    throw new MapperParsingException("error parsing field [" + name() + "], parameter cannot be null");
+                }
+                if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
+                    // null values are currently allowed, disallowing it will result in breaking changes
+                    // current implementation, skips the null values, therefore 'continue'
+                    continue;
                 }
-
-                range = new Range(rangeType, from, to, includeFrom, includeTo);
-            } else if (fieldType().rangeType == RangeType.IP && start == XContentParser.Token.VALUE_STRING) {
                 try {
-                    range = parseIpRangeFromCidr(parser);
-                } catch (IllegalArgumentException e) {
+                    if (fieldName.equals(GT_FIELD.getPreferredName()) || fieldName.equals(GTE_FIELD.getPreferredName())) {
+                        if (from != null) {
+                            throw new MapperParsingException("error parsing field [" + name() + "], invalid lower bound (gt/gte)");
+                        }
+                        includeFrom = fieldName.equals(GTE_FIELD.getPreferredName());
+                        from = rangeType.parseFrom(fieldType, parser, coerce.value(), includeFrom);
+                    } else if (fieldName.equals(LT_FIELD.getPreferredName()) || fieldName.equals(LTE_FIELD.getPreferredName())) {
+                        if (to != null) {
+                            throw new MapperParsingException("error parsing field [" + name() + "], invalid upper bound (lt/lte)");
+                        }
+                        includeTo = fieldName.equals(LTE_FIELD.getPreferredName());
+                        to = rangeType.parseTo(fieldType, parser, coerce.value(), includeTo);
+                    } else {
+                        throw new MapperParsingException(
+                            "error parsing field [" + name() + "], with unknown parameter [" + fieldName + "]"
+                        );
+                    }
+                } catch (final IllegalArgumentException e) {
+                    // We have to consume the JSON object in full
                     if (ignoreMalformed().value()) {
-                        context.addIgnoredField(fieldType().name());
-                        return;
+                        rangeIsMalformed = true;
                     } else {
                         throw e;
                     }
                 }
-            } else {
-                throw new MapperParsingException(
-                    "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)) {
-            createFieldNamesField(context);
+            if (rangeIsMalformed) {
+                context.addIgnoredField(fieldType().name());
+                return null;
+            }
+
+            // default values for unset range bounds
+            if (from == null) {
+                from = rangeType.minValue();
+            }
+            if (to == null) {
+                to = rangeType.maxValue();
+            }
+
+            return new Range(rangeType, from, to, includeFrom, includeTo);
         }
+        // all valid options exhausted
+        throw new MapperParsingException("error parsing field [" + name() + "], expected an object but got " + parser.currentName());
     }
 
     private static Range parseIpRangeFromCidr(final XContentParser parser) throws IOException {

server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java

@@ -43,6 +43,7 @@
 import org.opensearch.core.common.Strings;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
+import org.opensearch.core.xcontent.DeprecationHandler;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.core.xcontent.XContentParser;
 import org.opensearch.index.mapper.FieldNamesFieldMapper;
@@ -81,6 +82,9 @@ public class RangeQueryBuilder extends AbstractQueryBuilder<RangeQueryBuilder>
     private static final ParseField FORMAT_FIELD = new ParseField("format");
     private static final ParseField RELATION_FIELD = new ParseField("relation");
 
+    private static final String INVALID_LOWER_BOUND_MESSAGE = "invalid lower bound for [range] query";
+    private static final String INVALID_UPPER_BOUND_MESSAGE = "invalid upper bound for [range] query";
+
     private final String fieldName;
     private Object from;
     private Object to;
@@ -358,8 +362,8 @@ public static RangeQueryBuilder fromXContent(XContentParser parser) throws IOExc
         String fieldName = null;
         Object from = null;
         Object to = null;
-        boolean includeLower = RangeQueryBuilder.DEFAULT_INCLUDE_LOWER;
-        boolean includeUpper = RangeQueryBuilder.DEFAULT_INCLUDE_UPPER;
+        Boolean includeLower = null;
+        Boolean includeUpper = null;
         String timeZone = null;
         float boost = AbstractQueryBuilder.DEFAULT_BOOST;
         String queryName = null;
@@ -377,43 +381,53 @@ public static RangeQueryBuilder fromXContent(XContentParser parser) throws IOExc
                 while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
                     if (token == XContentParser.Token.FIELD_NAME) {
                         currentFieldName = parser.currentName();
-                    } else {
-                        if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            from = maybeConvertToBytesRef(parser.objectBytes());
-                        } else if (TO_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            to = maybeConvertToBytesRef(parser.objectBytes());
-                        } else if (INCLUDE_LOWER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            includeLower = parser.booleanValue();
-                        } else if (INCLUDE_UPPER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            includeUpper = parser.booleanValue();
-                        } else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            boost = parser.floatValue();
-                        } else if (GT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            from = maybeConvertToBytesRef(parser.objectBytes());
-                            includeLower = false;
-                        } else if (GTE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            from = maybeConvertToBytesRef(parser.objectBytes());
-                            includeLower = true;
-                        } else if (LT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            to = maybeConvertToBytesRef(parser.objectBytes());
-                            includeUpper = false;
-                        } else if (LTE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            to = maybeConvertToBytesRef(parser.objectBytes());
-                            includeUpper = true;
-                        } else if (TIME_ZONE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            timeZone = parser.text();
-                        } else if (FORMAT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            format = parser.text();
-                        } else if (RELATION_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            relation = parser.text();
-                        } else if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                            queryName = parser.text();
-                        } else {
-                            throw new ParsingException(
-                                parser.getTokenLocation(),
-                                "[range] query does not support [" + currentFieldName + "]"
-                            );
+                        continue; // only field name is required in this iteration
+                    }
+                    DeprecationHandler depHandler = parser.getDeprecationHandler();
+                    if (FROM_FIELD.match(currentFieldName, depHandler)) {
+                        if (from != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_LOWER_BOUND_MESSAGE);
+                        }
+                        from = maybeConvertToBytesRef(parser.objectBytes());
+                    } else if (TO_FIELD.match(currentFieldName, depHandler)) {
+                        if (to != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_UPPER_BOUND_MESSAGE);
+                        }
+                        to = maybeConvertToBytesRef(parser.objectBytes());
+                    } else if (INCLUDE_LOWER_FIELD.match(currentFieldName, depHandler)) {
+                        if (includeLower != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_LOWER_BOUND_MESSAGE);
+                        }
+                        includeLower = parser.booleanValue();
+                    } else if (INCLUDE_UPPER_FIELD.match(currentFieldName, depHandler)) {
+                        if (includeUpper != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_UPPER_BOUND_MESSAGE);
                         }
+                        includeUpper = parser.booleanValue();
+                    } else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, depHandler)) {
+                        boost = parser.floatValue();
+                    } else if (GT_FIELD.match(currentFieldName, depHandler) || GTE_FIELD.match(currentFieldName, depHandler)) {
+                        if (from != null || includeLower != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_LOWER_BOUND_MESSAGE);
+                        }
+                        from = maybeConvertToBytesRef(parser.objectBytes());
+                        includeLower = GTE_FIELD.match(currentFieldName, depHandler);
+                    } else if (LT_FIELD.match(currentFieldName, depHandler) || LTE_FIELD.match(currentFieldName, depHandler)) {
+                        if (to != null || includeUpper != null) {
+                            throw new ParsingException(parser.getTokenLocation(), INVALID_UPPER_BOUND_MESSAGE);
+                        }
+                        to = maybeConvertToBytesRef(parser.objectBytes());
+                        includeUpper = LTE_FIELD.match(currentFieldName, depHandler);
+                    } else if (TIME_ZONE_FIELD.match(currentFieldName, depHandler)) {
+                        timeZone = parser.text();
+                    } else if (FORMAT_FIELD.match(currentFieldName, depHandler)) {
+                        format = parser.text();
+                    } else if (RELATION_FIELD.match(currentFieldName, depHandler)) {
+                        relation = parser.text();
+                    } else if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, depHandler)) {
+                        queryName = parser.text();
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[range] query does not support [" + currentFieldName + "]");
                     }
                 }
             } else if (token.isValue()) {
@@ -424,8 +438,12 @@ public static RangeQueryBuilder fromXContent(XContentParser parser) throws IOExc
         RangeQueryBuilder rangeQuery = new RangeQueryBuilder(fieldName);
         rangeQuery.from(from);
         rangeQuery.to(to);
-        rangeQuery.includeLower(includeLower);
-        rangeQuery.includeUpper(includeUpper);
+        if (includeLower != null) {
+            rangeQuery.includeLower(includeLower);
+        }
+        if (includeUpper != null) {
+            rangeQuery.includeUpper(includeUpper);
+        }
         if (timeZone != null) {
             rangeQuery.timeZone(timeZone);
         }