Completion Suggester: Fix CompletionFieldMapper to correctly parse we…

…ight - Allows weight to be defined as a string representation of a positive integer closes #8090
elastic · Oct 28, 2014 · ba33c0a · ba33c0a
1 parent ebc15b1
commit ba33c0a
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 6 deletions.
diff --git a/docs/reference/search/suggesters/completion-suggest.asciidoc b/docs/reference/search/suggesters/completion-suggest.asciidoc
@@ -119,8 +119,9 @@ The following parameters are supported:
     might not yield any results, if `input` and `output` differ strongly).
 
 `weight`::
-    A positive integer, which defines a weight and allows you to
-    rank your suggestions. This field is optional.
+    A positive integer or a string containing a positive integer,
+    which defines a weight and allows you to rank your suggestions.
+    This field is optional.
 
 NOTE: Even though you will lose most of the features of the
 completion suggest, you can choose to use the following shorthand form.

diff --git a/src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/CompletionFieldMapper.java
@@ -295,16 +295,24 @@ public void parse(ParseContext context) throws IOException {
                     if (Fields.CONTENT_FIELD_NAME_INPUT.equals(currentFieldName)) {
                         inputs.add(parser.text());
                     }
+                    if (Fields.CONTENT_FIELD_NAME_WEIGHT.equals(currentFieldName)) {
+                        Number weightValue;
+                        try {
+                            weightValue = Long.parseLong(parser.text());
+                        } catch (NumberFormatException e) {
+                            throw new ElasticsearchIllegalArgumentException("Weight must be a string representing a numeric value, but was [" + parser.text() + "]");
+                        }
+                        weight = weightValue.longValue(); // always parse a long to make sure we don't get overflow
+                        checkWeight(weight);
+                    }
                 } else if (token == XContentParser.Token.VALUE_NUMBER) {
                     if (Fields.CONTENT_FIELD_NAME_WEIGHT.equals(currentFieldName)) {
                         NumberType numberType = parser.numberType();
                         if (NumberType.LONG != numberType && NumberType.INT != numberType) {
                             throw new ElasticsearchIllegalArgumentException("Weight must be an integer, but was [" + parser.numberValue() + "]");
                         }
-                        weight = parser.longValue(); // always parse a long to make sure we don't get the overflow value
-                        if (weight < 0 || weight > Integer.MAX_VALUE) {
-                            throw new ElasticsearchIllegalArgumentException("Weight must be in the interval [0..2147483647], but was [" + weight + "]");
-                        }
+                        weight = parser.longValue(); // always parse a long to make sure we don't get overflow
+                        checkWeight(weight);
                     }
                 } else if (token == XContentParser.Token.START_ARRAY) {
                     if (Fields.CONTENT_FIELD_NAME_INPUT.equals(currentFieldName)) {
@@ -341,6 +349,12 @@ public void parse(ParseContext context) throws IOException {
         }
     }
 
+    private void checkWeight(long weight) {
+        if (weight < 0 || weight > Integer.MAX_VALUE) {
+            throw new ElasticsearchIllegalArgumentException("Weight must be in the interval [0..2147483647], but was [" + weight + "]");
+        }
+    }
+
     /**
      * Get the context mapping associated with this completion field.
      */

diff --git a/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchTests.java b/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchTests.java
@@ -178,6 +178,68 @@ public void testThatWeightMustBeAnInteger() throws Exception {
         }
     }
 
+    @Test
+    public void testThatWeightCanBeAString() throws Exception {
+        createIndexAndMapping(completionMappingBuilder);
+
+        client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder()
+                        .startObject().startObject(FIELD)
+                        .startArray("input").value("testing").endArray()
+                        .field("weight", "10")
+                        .endObject().endObject()
+        ).get();
+
+        refresh();
+
+        SuggestResponse suggestResponse = client().prepareSuggest(INDEX).addSuggestion(
+                new CompletionSuggestionBuilder("testSuggestions").field(FIELD).text("test").size(10)
+        ).execute().actionGet();
+
+        assertSuggestions(suggestResponse, "testSuggestions", "testing");
+        Suggest.Suggestion.Entry.Option option = suggestResponse.getSuggest().getSuggestion("testSuggestions").getEntries().get(0).getOptions().get(0);
+        assertThat(option, is(instanceOf(CompletionSuggestion.Entry.Option.class)));
+        CompletionSuggestion.Entry.Option prefixOption = (CompletionSuggestion.Entry.Option) option;
+
+        assertThat(prefixOption.getText().string(), equalTo("testing"));
+        assertThat((long) prefixOption.getScore(), equalTo(10l));
+    }
+
+
+    @Test
+    public void testThatWeightMustNotBeANonNumberString() throws Exception {
+        createIndexAndMapping(completionMappingBuilder);
+
+        try {
+            client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder()
+                            .startObject().startObject(FIELD)
+                            .startArray("input").value("sth").endArray()
+                            .field("weight", "thisIsNotValid")
+                            .endObject().endObject()
+            ).get();
+            fail("Indexing with a non-number representing string as weight was successful, but should not be");
+        } catch (MapperParsingException e) {
+            assertThat(ExceptionsHelper.detailedMessage(e), containsString("thisIsNotValid"));
+        }
+    }
+
+    @Test
+    public void testThatWeightAsStringMustBeInt() throws Exception {
+        createIndexAndMapping(completionMappingBuilder);
+
+        String weight = String.valueOf(Long.MAX_VALUE - 4);
+        try {
+            client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder()
+                            .startObject().startObject(FIELD)
+                            .startArray("input").value("testing").endArray()
+                            .field("weight", weight)
+                            .endObject().endObject()
+            ).get();
+            fail("Indexing with weight string representing value > Int.MAX_VALUE was successful, but should not be");
+        } catch (MapperParsingException e) {
+            assertThat(ExceptionsHelper.detailedMessage(e), containsString(weight));
+        }
+    }
+
     @Test
     public void testThatInputCanBeAStringInsteadOfAnArray() throws Exception {
         createIndexAndMapping(completionMappingBuilder);