Merge pull request #17282 from MaineC/deprecation/sort-option-reverse-removal

Remove deprecated reverse option from sorting

core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java

@@ -47,7 +47,6 @@ public class FieldSortBuilder extends SortBuilder<FieldSortBuilder> {
     public static final ParseField NESTED_FILTER = new ParseField("nested_filter");
     public static final ParseField MISSING = new ParseField("missing");
     public static final ParseField ORDER = new ParseField("order");
-    public static final ParseField REVERSE = new ParseField("reverse");
     public static final ParseField SORT_MODE = new ParseField("mode");
     public static final ParseField UNMAPPED_TYPE = new ParseField("unmapped_type");
 
@@ -350,11 +349,6 @@ public class FieldSortBuilder extends SortBuilder<FieldSortBuilder> {
                     nestedPath = parser.text();
                 } else if (context.parseFieldMatcher().match(currentFieldName, MISSING)) {
                     missing = parser.objectText();
-                } else if (context.parseFieldMatcher().match(currentFieldName, REVERSE)) {
-                    if (parser.booleanValue()) {
-                        order = SortOrder.DESC;
-                    }
-                    // else we keep the default ASC
                 } else if (context.parseFieldMatcher().match(currentFieldName, ORDER)) {
                     String sortOrder = parser.text();
                     if ("asc".equals(sortOrder)) {
@@ -362,14 +356,14 @@ public class FieldSortBuilder extends SortBuilder<FieldSortBuilder> {
                     } else if ("desc".equals(sortOrder)) {
                         order = SortOrder.DESC;
                     } else {
-                        throw new IllegalStateException("Sort order " + sortOrder + " not supported.");
+                        throw new ParsingException(parser.getTokenLocation(), "Sort order [{}] not supported.", sortOrder);
                     }
                 } else if (context.parseFieldMatcher().match(currentFieldName, SORT_MODE)) {
                     sortMode = SortMode.fromString(parser.text());
                 } else if (context.parseFieldMatcher().match(currentFieldName, UNMAPPED_TYPE)) {
                     unmappedType = parser.text();
                 } else {
-                    throw new IllegalArgumentException("Option " + currentFieldName + " not supported.");
+                    throw new ParsingException(parser.getTokenLocation(), "Option [{}] not supported.", currentFieldName);
                 }
             }
         }

core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java

@@ -29,6 +29,7 @@ import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.geo.GeoDistance;
 import org.elasticsearch.common.geo.GeoDistance.FixedSourceDistance;
 import org.elasticsearch.common.geo.GeoPoint;
@@ -38,6 +39,7 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.DistanceUnit;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentParser.Token;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested;
 import org.elasticsearch.index.fielddata.IndexGeoPointFieldData;
@@ -66,13 +68,13 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
     public static final boolean DEFAULT_COERCE = false;
     public static final boolean DEFAULT_IGNORE_MALFORMED = false;
     public static final ParseField UNIT_FIELD = new ParseField("unit");
-    public static final ParseField REVERSE_FIELD = new ParseField("reverse");
     public static final ParseField DISTANCE_TYPE_FIELD = new ParseField("distance_type");
     public static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize");
     public static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed");
     public static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode");
     public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path");
     public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter");
+    public static final ParseField REVERSE_FORBIDDEN = new ParseField("reverse");
 
     private final String fieldName;
     private final List<GeoPoint> points = new ArrayList<>();
@@ -432,15 +434,20 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
                     nestedFilter = context.parseInnerQueryBuilder();
                 } else {
                     // the json in the format of -> field : { lat : 30, lon : 12 }
+                    if (fieldName != null && fieldName.equals(currentName) == false) {
+                        throw new ParsingException(
+                                parser.getTokenLocation(),
+                                "Trying to reset fieldName to [{}], already set to [{}].",
+                                currentName,
+                                fieldName);
+                    }
                     fieldName = currentName;
                     GeoPoint point = new GeoPoint();
                     GeoUtils.parseGeoPoint(parser, point);
                     geoPoints.add(point);
                 }
             } else if (token.isValue()) {
-                if (parseFieldMatcher.match(currentName, REVERSE_FIELD)) {
+                if (parseFieldMatcher.match(currentName, ORDER_FIELD)) {
-                    order = parser.booleanValue() ? SortOrder.DESC : SortOrder.ASC;
-                } else if (parseFieldMatcher.match(currentName, ORDER_FIELD)) {
                     order = SortOrder.fromString(parser.text());
                 } else if (parseFieldMatcher.match(currentName, UNIT_FIELD)) {
                     unit = DistanceUnit.fromString(parser.text());
@@ -460,11 +467,24 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
                     sortMode = SortMode.fromString(parser.text());
                 } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) {
                     nestedPath = parser.text();
-                } else {
+                } else if (token == Token.VALUE_STRING){
+                    if (fieldName != null && fieldName.equals(currentName) == false) {
+                        throw new ParsingException(
+                                parser.getTokenLocation(),
+                                "Trying to reset fieldName to [{}], already set to [{}].",
+                                currentName,
+                                fieldName);
+                    }
+
                     GeoPoint point = new GeoPoint();
                     point.resetFromString(parser.text());
                     geoPoints.add(point);
                     fieldName = currentName;
+                } else {
+                    throw new ParsingException(
+                            parser.getTokenLocation(),
+                            "Only geohashes of type string supported for field [{}]",
+                            currentName);
                 }
             }
         }
@@ -495,10 +515,16 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
         if (!indexCreatedBeforeV2_0 && !ignoreMalformed) {
             for (GeoPoint point : localPoints) {
                 if (GeoUtils.isValidLatitude(point.lat()) == false) {
-                    throw new ElasticsearchParseException("illegal latitude value [{}] for [GeoDistanceSort]", point.lat());
+                    throw new ElasticsearchParseException(
+                            "illegal latitude value [{}] for [GeoDistanceSort] for field [{}].", 
+                            point.lat(),
+                            fieldName);
                 }
                 if (GeoUtils.isValidLongitude(point.lon()) == false) {
-                    throw new ElasticsearchParseException("illegal longitude value [{}] for [GeoDistanceSort]", point.lon());
+                    throw new ElasticsearchParseException(
+                            "illegal longitude value [{}] for [GeoDistanceSort] for field [{}].",
+                            point.lon(),
+                            fieldName);
                 }
             }
         }

core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java

@@ -41,6 +41,7 @@ public class ScoreSortBuilder extends SortBuilder<ScoreSortBuilder> {
     public static final String NAME = "_score";
     public static final ParseField REVERSE_FIELD = new ParseField("reverse");
     public static final ParseField ORDER_FIELD = new ParseField("order");
+    private static final ParseField REVERSE_FORBIDDEN = new ParseField("reverse");
     private static final SortField SORT_SCORE = new SortField(null, SortField.Type.SCORE);
     private static final SortField SORT_SCORE_REVERSE = new SortField(null, SortField.Type.SCORE, true);
 
@@ -94,12 +95,7 @@ public class ScoreSortBuilder extends SortBuilder<ScoreSortBuilder> {
             if (token == XContentParser.Token.FIELD_NAME) {
                 currentName = parser.currentName();
             } else if (token.isValue()) {
-                if (matcher.match(currentName, REVERSE_FIELD)) {
+                if (matcher.match(currentName, ORDER_FIELD)) {
-                    if (parser.booleanValue()) {
-                        result.order(SortOrder.ASC);
-                    }
-                    // else we keep the default DESC
-                } else if (matcher.match(currentName, ORDER_FIELD)) {
                     result.order(SortOrder.fromString(parser.text()));
                 } else {
                     throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]");

core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java

@@ -207,7 +207,7 @@ public abstract class AbstractSortTestCase<T extends SortBuilder<T>> extends EST
         }
     }
 
-    private QueryShardContext createMockShardContext() {
+    protected QueryShardContext createMockShardContext() {
         Index index = new Index(randomAsciiOfLengthBetween(1, 10), "_na_");
         IndexSettings idxSettings = IndexSettingsModule.newIndexSettings(index, Settings.EMPTY);
         IndicesFieldDataCache cache = new IndicesFieldDataCache(Settings.EMPTY, null);

core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java

@@ -20,6 +20,11 @@ x * Licensed to Elasticsearch under one or more contributor
 package org.elasticsearch.search.sort;
 
 import org.apache.lucene.search.SortField;
+import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.ParsingException;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.query.QueryParseContext;
 
 import java.io.IOException;
@@ -105,6 +110,28 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
         }
     }
     
+    public void testReverseOptionFails() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+        String json = "{ \"post_date\" : {\"reverse\" : true} },\n";
+
+        XContentParser parser = XContentFactory.xContent(json).createParser(json);
+        // need to skip until parser is located on second START_OBJECT
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+
+        try {
+          FieldSortBuilder.fromXContent(context, "");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            // all good
+        }
+    }
+
+
     @Override
     protected FieldSortBuilder fromXContent(QueryParseContext context, String fieldName) throws IOException {
         return FieldSortBuilder.fromXContent(context, fieldName);

core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java

@@ -21,6 +21,8 @@ package org.elasticsearch.search.sort;
 
 
 import org.apache.lucene.search.SortField;
+import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.geo.GeoDistance;
 import org.elasticsearch.common.geo.GeoPoint;
@@ -202,6 +204,71 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
           }
     }
 
+    public void testReverseOptionFailsWhenNonStringField() throws IOException {
+        String json = "{\n" +
+                "  \"testname\" : [ {\n" +
+                "    \"lat\" : -6.046997540714173,\n" +
+                "    \"lon\" : -51.94128329747579\n" +
+                "  } ],\n" +
+                "  \"reverse\" : true\n" +
+                "}";
+        XContentParser itemParser = XContentHelper.createParser(new BytesArray(json));
+        itemParser.nextToken();
+
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.reset(itemParser);
+
+        try {
+          GeoDistanceSortBuilder.fromXContent(context, "");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            assertEquals("Only geohashes of type string supported for field [reverse]", e.getMessage());
+        }
+    }
+
+    public void testReverseOptionFailsWhenStringFieldButResetting() throws IOException {
+        String json = "{\n" +
+                "  \"testname\" : [ {\n" +
+                "    \"lat\" : -6.046997540714173,\n" +
+                "    \"lon\" : -51.94128329747579\n" +
+                "  } ],\n" +
+                "  \"reverse\" : \"true\"\n" +
+                "}";
+        XContentParser itemParser = XContentHelper.createParser(new BytesArray(json));
+        itemParser.nextToken();
+
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.reset(itemParser);
+
+        try {
+          GeoDistanceSortBuilder.fromXContent(context, "");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            assertEquals("Trying to reset fieldName to [reverse], already set to [testname].", e.getMessage());
+        }
+    }
+
+    public void testReverseOptionFailsBuildWhenInvalidGeoHashString() throws IOException {
+        String json = "{\n" +
+                "  \"reverse\" : \"false\"\n" +
+                "}";
+        XContentParser itemParser = XContentHelper.createParser(new BytesArray(json));
+        itemParser.nextToken();
+
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.reset(itemParser);
+
+        try {
+          GeoDistanceSortBuilder item = GeoDistanceSortBuilder.fromXContent(context, "");
+          item.ignoreMalformed(false);
+          item.build(createMockShardContext());
+          
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ElasticsearchParseException e) {
+            assertEquals("illegal latitude value [269.384765625] for [GeoDistanceSort] for field [reverse].", e.getMessage());
+        }
+    }
+
     public void testSortModeSumIsRejectedInJSON() throws IOException {
         String json = "{\n" +
                 "  \"testname\" : [ {\n" +
@@ -210,7 +277,6 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
                 "  } ],\n" +
                 "  \"unit\" : \"m\",\n" +
                 "  \"distance_type\" : \"sloppy_arc\",\n" +
-                "  \"reverse\" : true,\n" +
                 "  \"mode\" : \"SUM\",\n" +
                 "  \"coerce\" : false,\n" +
                 "  \"ignore_malformed\" : false\n" +
@@ -231,7 +297,6 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
                 "    \"ezu09wxw6v4c\", \"kc7s3515p6k6\", \"jgeuvjwrmfzn\", \"kcpcfj7ruyf8\" ],\n" +
                 "    \"unit\" : \"m\",\n" +
                 "    \"distance_type\" : \"sloppy_arc\",\n" +
-                "    \"reverse\" : true,\n" +
                 "    \"mode\" : \"MAX\",\n" +
                 "    \"nested_filter\" : {\n" +
                 "      \"ids\" : {\n" +

core/src/test/java/org/elasticsearch/search/sort/ScoreSortBuilderTests.java

@@ -22,6 +22,7 @@ package org.elasticsearch.search.sort;
 
 import org.apache.lucene.search.SortField;
 import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -81,6 +82,26 @@ public class ScoreSortBuilderTests extends AbstractSortTestCase<ScoreSortBuilder
         assertEquals(order, scoreSort.order());
     }
 
+    public void testReverseOptionFails() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+        String json = "{ \"_score\": { \"reverse\": true }}";
+        XContentParser parser = XContentFactory.xContent(json).createParser(json);
+        // need to skip until parser is located on second START_OBJECT
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+
+        try {
+          ScoreSortBuilder.fromXContent(context, "_score");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            // all good
+        }
+    }
+
     @Override
     protected void sortFieldAssertions(ScoreSortBuilder builder, SortField sortField) {
         assertEquals(SortField.Type.SCORE, sortField.getType());

docs/reference/migration/migrate_5_0/search.asciidoc

@@ -141,3 +141,8 @@ The term vectors APIs no longer persist unmapped fields in the mappings.
 
 The `dfs` parameter to the term vectors API has been removed completely. Term
 vectors don't support distributed document frequencies anymore.
+
+==== Sort
+
+The `reverse` parameter has been removed, in favour of explicitly
+specifying the sort order with the `order` option.

docs/reference/search/request/sort.asciidoc

@@ -346,7 +346,7 @@ When sorting on a field, scores are not computed. By setting
 {
     "track_scores": true,
     "sort" : [
-        { "post_date" : {"reverse" : true} },
+        { "post_date" : {"order" : "desc"} },
         { "name" : "desc" },
         { "age" : "desc" }
     ],