From 17a420e6aa7745c93bab7b0e2d6c08b203527508 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christoph=20B=C3=BCscher?= <christoph@elastic.co>
Date: Mon, 14 Mar 2016 15:21:39 +0100
Subject: [PATCH] Adressing review comments, adding parsing tests

---
 .../search/sort/ScriptSortBuilder.java        |  28 +++-
 .../search/sort/RandomSortDataGenerator.java  |  13 +-
 .../search/sort/ScriptSortBuilderTests.java   | 139 +++++++++++++++++-
 .../search/sort/SortModeTest.java             |   8 +
 4 files changed, 172 insertions(+), 16 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
index 6254d5b1e41..9b51eeca31d 100644
--- a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java
@@ -21,6 +21,7 @@ package org.elasticsearch.search.sort;
 
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.io.stream.NamedWriteable;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -59,8 +60,7 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
 
     private ScriptSortType type;
 
-    // TODO make this an enum
-    private String sortMode;
+    private SortMode sortMode;
 
     private QueryBuilder<?> nestedFilter;
 
@@ -109,7 +109,8 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
      * Defines which distance to use for sorting in the case a document contains multiple geo points.
      * Possible values: min and max
      */
-    public ScriptSortBuilder sortMode(String sortMode) {
+    public ScriptSortBuilder sortMode(SortMode sortMode) {
+        Objects.requireNonNull(sortMode, "sort mode cannot be null.");
         this.sortMode = sortMode;
         return this;
     }
@@ -117,7 +118,7 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
     /**
      * Get the sort mode.
      */
-    public String sortMode() {
+    public SortMode sortMode() {
         return this.sortMode;
     }
 
@@ -179,7 +180,7 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
         ParseFieldMatcher parseField = context.parseFieldMatcher();
         Script script = null;
         ScriptSortType type = null;
-        String sortMode = null;
+        SortMode sortMode = null;
         SortOrder order = null;
         QueryBuilder<?> nestedFilter = null;
         String nestedPath = null;
@@ -197,6 +198,8 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
                     params = parser.map();
                 } else if (parseField.match(currentName, NESTED_FILTER_FIELD)) {
                     nestedFilter = context.parseInnerQueryBuilder();
+                } else {
+                    throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]");
                 }
             } else if (token.isValue()) {
                 if (parseField.match(currentName, ORDER_FIELD)) {
@@ -206,10 +209,14 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
                 } else if (parseField.match(currentName, TYPE_FIELD)) {
                     type = ScriptSortType.fromString(parser.text());
                 } else if (parseField.match(currentName, SORTMODE_FIELD)) {
-                    sortMode = parser.text();
+                    sortMode = SortMode.fromString(parser.text());
                 } else if (parseField.match(currentName, NESTED_PATH_FIELD)) {
                     nestedPath = parser.text();
+                } else {
+                    throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]");
                 }
+            } else {
+                throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] unexpected token [" + token + "]");
             }
         }
 
@@ -266,7 +273,10 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
         script.writeTo(out);
         type.writeTo(out);
         order.writeTo(out);
-        out.writeOptionalString(sortMode);
+        out.writeBoolean(sortMode != null);
+        if (sortMode != null) {
+            sortMode.writeTo(out);
+        }
         out.writeOptionalString(nestedPath);
         boolean hasNestedFilter = nestedFilter != null;
         out.writeBoolean(hasNestedFilter);
@@ -279,7 +289,9 @@ public class ScriptSortBuilder extends SortBuilder<ScriptSortBuilder> implements
     public ScriptSortBuilder readFrom(StreamInput in) throws IOException {
         ScriptSortBuilder builder = new ScriptSortBuilder(Script.readScript(in), ScriptSortType.PROTOTYPE.readFrom(in));
         builder.order(SortOrder.readOrderFrom(in));
-        builder.sortMode = in.readOptionalString();
+        if (in.readBoolean()) {
+            builder.sortMode(SortMode.PROTOTYPE.readFrom(in));
+        }
         builder.nestedPath = in.readOptionalString();
         if (in.readBoolean()) {
             builder.nestedFilter = in.readQuery();
diff --git a/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java b/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java
index fcd5284119c..405c1c43e77 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java
+++ b/core/src/test/java/org/elasticsearch/search/sort/RandomSortDataGenerator.java
@@ -60,15 +60,14 @@ public class RandomSortDataGenerator {
         return nestedPath;
     }
 
-    public static String mode(String original) {
-        String[] modes = {"min", "max", "avg", "sum"};
-        String mode = ESTestCase.randomFrom(modes);
+    public static SortMode mode(SortMode original) {
+        SortMode mode = ESTestCase.randomFrom(SortMode.values());
         while (mode.equals(original)) {
-            mode = ESTestCase.randomFrom(modes);
+            mode = ESTestCase.randomFrom(SortMode.values());
         }
         return mode;
     }
-    
+
     public static Object missing(Object original) {
         Object missing = null;
         Object otherMissing = null;
@@ -95,12 +94,12 @@ public class RandomSortDataGenerator {
               break;
           default:
               throw new IllegalStateException("Unknown missing type.");
-              
+
           }
         }
         return missing;
     }
-    
+
     public static SortOrder order(SortOrder original) {
         SortOrder order = SortOrder.ASC;
         if (order.equals(original)) {
diff --git a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java
index 40798cbcb88..091a6c3002a 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java
+++ b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java
@@ -20,8 +20,17 @@
 package org.elasticsearch.search.sort;
 
 
+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 org.elasticsearch.script.Script;
+import org.elasticsearch.script.ScriptService.ScriptType;
 import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType;
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
 
 import java.io.IOException;
 
@@ -59,7 +68,9 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
                 result = new ScriptSortBuilder(script, type.equals(ScriptSortType.NUMBER) ? ScriptSortType.STRING : ScriptSortType.NUMBER);
             }
             result.order(original.order());
-            result.sortMode(original.sortMode());
+            if (original.sortMode() != null) {
+                result.sortMode(original.sortMode());
+            }
             result.setNestedFilter(original.getNestedFilter());
             result.setNestedPath(original.getNestedPath());
             return result;
@@ -86,6 +97,9 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
         return result;
     }
 
+    @Rule
+    public ExpectedException exceptionRule = ExpectedException.none();
+
     public void testScriptSortType() {
         // we rely on these ordinals in serialization, so changing them breaks bwc.
         assertEquals(0, ScriptSortType.STRING.ordinal());
@@ -101,4 +115,127 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase<ScriptSortBuild
         assertEquals(ScriptSortType.NUMBER, ScriptSortType.fromString("Number"));
         assertEquals(ScriptSortType.NUMBER, ScriptSortType.fromString("NUMBER"));
     }
+
+    public void testScriptSortTypeNull() {
+        exceptionRule.expect(NullPointerException.class);
+        exceptionRule.expectMessage("input string is null");
+        ScriptSortType.fromString(null);
+    }
+
+    public void testScriptSortTypeIllegalArgument() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Unknown ScriptSortType [xyz]");
+        ScriptSortType.fromString("xyz");
+    }
+
+    public void testParseJson() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+        String scriptSort = "{\n" +
+                "\"_script\" : {\n" +
+                    "\"type\" : \"number\",\n" +
+                    "\"script\" : {\n" +
+                        "\"inline\": \"doc['field_name'].value * factor\",\n" +
+                        "\"params\" : {\n" +
+                            "\"factor\" : 1.1\n" +
+                            "}\n" +
+                    "},\n" +
+                    "\"mode\" : \"max\",\n" +
+                    "\"order\" : \"asc\"\n" +
+                "} }\n";
+        XContentParser parser = XContentFactory.xContent(scriptSort).createParser(scriptSort);
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+        ScriptSortBuilder builder = ScriptSortBuilder.PROTOTYPE.fromXContent(context, null);
+        assertEquals("doc['field_name'].value * factor", builder.script().getScript());
+        assertNull(builder.script().getLang());
+        assertEquals(1.1, builder.script().getParams().get("factor"));
+        assertEquals(ScriptType.INLINE, builder.script().getType());
+        assertEquals(ScriptSortType.NUMBER, builder.type());
+        assertEquals(SortOrder.ASC, builder.order());
+        assertEquals(SortMode.MAX, builder.sortMode());
+        assertNull(builder.getNestedFilter());
+        assertNull(builder.getNestedPath());
+    }
+
+    public void testParseJsonOldStyle() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+        String scriptSort = "{\n" +
+                "\"_script\" : {\n" +
+                    "\"type\" : \"number\",\n" +
+                    "\"script\" : \"doc['field_name'].value * factor\",\n" +
+                    "\"params\" : {\n" +
+                        "\"factor\" : 1.1\n" +
+                    "},\n" +
+                    "\"mode\" : \"max\",\n" +
+                    "\"order\" : \"asc\"\n" +
+                "} }\n";
+        XContentParser parser = XContentFactory.xContent(scriptSort).createParser(scriptSort);
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+        ScriptSortBuilder builder = ScriptSortBuilder.PROTOTYPE.fromXContent(context, null);
+        assertEquals("doc['field_name'].value * factor", builder.script().getScript());
+        assertNull(builder.script().getLang());
+        assertEquals(1.1, builder.script().getParams().get("factor"));
+        assertEquals(ScriptType.INLINE, builder.script().getType());
+        assertEquals(ScriptSortType.NUMBER, builder.type());
+        assertEquals(SortOrder.ASC, builder.order());
+        assertEquals(SortMode.MAX, builder.sortMode());
+        assertNull(builder.getNestedFilter());
+        assertNull(builder.getNestedPath());
+    }
+
+    public void testParseBadFieldNameExceptions() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+        String scriptSort = "{\"_script\" : {" + "\"bad_field\" : \"number\"" + "} }";
+        XContentParser parser = XContentFactory.xContent(scriptSort).createParser(scriptSort);
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+        exceptionRule.expect(ParsingException.class);
+        exceptionRule.expectMessage("failed to parse field [bad_field]");
+        ScriptSortBuilder.PROTOTYPE.fromXContent(context, null);
+    }
+
+    public void testParseBadFieldNameExceptionsOnStartObject() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+
+        String scriptSort = "{\"_script\" : {" + "\"bad_field\" : { \"order\" : \"asc\" } } }";
+        XContentParser parser = XContentFactory.xContent(scriptSort).createParser(scriptSort);
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+        exceptionRule.expect(ParsingException.class);
+        exceptionRule.expectMessage("failed to parse field [bad_field]");
+        ScriptSortBuilder.PROTOTYPE.fromXContent(context, null);
+    }
+
+    public void testParseUnexpectedToken() throws IOException {
+        QueryParseContext context = new QueryParseContext(indicesQueriesRegistry);
+        context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
+
+        String scriptSort = "{\"_script\" : {" + "\"script\" : [ \"order\" : \"asc\" ] } }";
+        XContentParser parser = XContentFactory.xContent(scriptSort).createParser(scriptSort);
+        parser.nextToken();
+        parser.nextToken();
+        parser.nextToken();
+
+        context.reset(parser);
+        exceptionRule.expect(ParsingException.class);
+        exceptionRule.expectMessage("unexpected token [START_ARRAY]");
+        ScriptSortBuilder.PROTOTYPE.fromXContent(context, null);
+    }
 }
diff --git a/core/src/test/java/org/elasticsearch/search/sort/SortModeTest.java b/core/src/test/java/org/elasticsearch/search/sort/SortModeTest.java
index f7318ad0a59..ae159231443 100644
--- a/core/src/test/java/org/elasticsearch/search/sort/SortModeTest.java
+++ b/core/src/test/java/org/elasticsearch/search/sort/SortModeTest.java
@@ -46,7 +46,15 @@ public class SortModeTest extends ESTestCase {
             assertEquals(mode, SortMode.fromString(mode.toString()));
             assertEquals(mode, SortMode.fromString(mode.toString().toUpperCase()));
         }
+    }
 
+    public void testParseNull() {
+        exceptionRule.expect(NullPointerException.class);
+        exceptionRule.expectMessage("input string is null");
+        SortMode.fromString(null);
+    }
+
+    public void testIllegalArgument() {
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage("Unknown SortMode [xyz]");
         SortMode.fromString("xyz");