From 96f1606cdc0fdb7a7e9f749c73ef9e0d326e64b2 Mon Sep 17 00:00:00 2001 From: Areek Zillur Date: Wed, 22 Oct 2014 13:44:33 -0400 Subject: [PATCH] Completion Suggester: Fix CompletionFieldMapper to correctly parse weight - Allows weight to be defined as a string representation of a positive integer closes #8090 --- .../suggesters/completion-suggest.asciidoc | 5 +- .../mapper/core/CompletionFieldMapper.java | 22 +++++-- .../suggest/CompletionSuggestSearchTests.java | 62 +++++++++++++++++++ 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/docs/reference/search/suggesters/completion-suggest.asciidoc b/docs/reference/search/suggesters/completion-suggest.asciidoc index 1cd6317b0b0..fbb87995d50 100644 --- 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 index da2fa61f12d..f69b79847ab 100644 --- 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 class CompletionFieldMapper extends AbstractFieldMapper { 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 class CompletionFieldMapper extends AbstractFieldMapper { } } + 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 index 7665c473b02..a0364d383ad 100644 --- 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 class CompletionSuggestSearchTests extends ElasticsearchIntegrationTest { } } + @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);