From 5dd481bfe3ef81a59217f7809ca51dc4f31b893e Mon Sep 17 00:00:00 2001
From: Isabel Drost-Fromm <isabel.drostfromm@elasticsearch.com>
Date: Thu, 24 Mar 2016 15:55:21 +0100
Subject: [PATCH] Disallow reverse option and check it throws an exception

This adds tests to check that using reverse as sort option is disallowed
and throws an exception.
---
 .../search/sort/FieldSortBuilder.java         |  4 +--
 .../search/sort/GeoDistanceSortBuilder.java   | 10 +++++++
 .../search/sort/ScoreSortBuilder.java         |  1 +
 .../search/sort/FieldSortBuilderTests.java    | 28 +++++++++++++++++++
 .../sort/GeoDistanceSortBuilderTests.java     | 24 +++++++++++++++-
 .../search/sort/ScoreSortBuilderTests.java    | 21 ++++++++++++++
 6 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
index f75ea37f854..414062c0cd2 100644
--- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
@@ -372,14 +372,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);
                 }
             }
         }
diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java
index 4712799c658..c6a63d5f085 100644
--- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java
+++ b/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;
@@ -72,6 +73,7 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
     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");
 
     public static final GeoDistanceSortBuilder PROTOTYPE = new GeoDistanceSortBuilder("_na_", -1, -1);
 
@@ -465,6 +467,14 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
                     sortMode = SortMode.fromString(parser.text());
                 } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) {
                     nestedPath = parser.text();
+                } else if (parseFieldMatcher.match(currentName, REVERSE_FORBIDDEN)) {
+                    // explicitly filter for reverse in json: used to be a valid sort option, forbidden now,
+                    // if not filtered here it will be treated as a reference to a field in the index with
+                    // geo coordinates given as geohash string
+                    throw new ParsingException(
+                            parser.getTokenLocation(),
+                            "Sort option [{}] no longer supported.",
+                            REVERSE_FORBIDDEN.getPreferredName());
                 } else {
                     GeoPoint point = new GeoPoint();
                     point.resetFromString(parser.text());
diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java
index 9124540f881..c222634ca0c 100644
--- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java
+++ b/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 ScoreSortBuilder PROTOTYPE = new ScoreSortBuilder();
     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);
 
diff --git a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
index 2c1a4a65c7b..f28ec8797c2 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
+++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
@@ -20,6 +20,12 @@ 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;
 
@@ -103,4 +109,26 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
             assertEquals(builder.getFieldName(), sortField.getField());
         }
     }
+    
+    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.PROTOTYPE.fromXContent(context, "");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            // all good
+        }
+    }
+
 }
diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java
index 7544e644b0a..e63cb5ad299 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java
+++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java
@@ -21,6 +21,7 @@ package org.elasticsearch.search.sort;
 
 
 import org.apache.lucene.search.SortField;
+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 +203,28 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
           }
     }
 
+    public void testReverseOptionFails() 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.PROTOTYPE.fromXContent(context, "");
+          fail("adding reverse sorting option should fail with an exception");
+        } catch (ParsingException e) {
+            assertEquals("Sort option [reverse] no longer supported.", e.getMessage());
+        }
+    }
+    
     public void testSortModeSumIsRejectedInJSON() throws IOException {
         String json = "{\n" +
                 "  \"testname\" : [ {\n" +
@@ -210,7 +233,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" +
diff --git a/core/src/test/java/org/elasticsearch/search/sort/ScoreSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/ScoreSortBuilderTests.java
index 225ae49ad56..6d605fd6258 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/ScoreSortBuilderTests.java
+++ b/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.PROTOTYPE.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());