From ba4a3db03b7519624eee7c9acccaefeb4480a5fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 11 Oct 2016 21:11:58 +0200 Subject: [PATCH] Use ConstructingObjectParser for parsing DirectCandidateGenerator When refactoring DirectCandidateGeneratorBuilder recently, the ConstructingObjectParser that we have today was not available. Instead we used some workaround, but it is better to remove this now and use ConstructingObjectParser instead. --- .../DirectCandidateGeneratorBuilder.java | 70 +++++-------------- .../phrase/DirectCandidateGeneratorTests.java | 7 +- 2 files changed, 22 insertions(+), 55 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java b/core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java index bf9158f9b87..200a2119a0d 100644 --- a/core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java @@ -29,10 +29,9 @@ import org.apache.lucene.search.spell.SuggestMode; import org.apache.lucene.util.automaton.LevenshteinAutomata; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.mapper.MapperService; @@ -41,10 +40,8 @@ import org.elasticsearch.search.suggest.SortBy; import org.elasticsearch.search.suggest.phrase.PhraseSuggestionBuilder.CandidateGenerator; import java.io.IOException; -import java.util.HashSet; import java.util.Locale; import java.util.Objects; -import java.util.Set; import java.util.function.Consumer; public final class DirectCandidateGeneratorBuilder implements CandidateGenerator { @@ -89,30 +86,6 @@ public final class DirectCandidateGeneratorBuilder implements CandidateGenerator this.field = field; } - /** - * Quasi copy-constructor that takes all values from the generator - * passed in, but uses different field name. Needed by parser because we - * need to buffer the field name but read all other properties to a - * temporary object. - */ - private static DirectCandidateGeneratorBuilder replaceField(String field, DirectCandidateGeneratorBuilder other) { - DirectCandidateGeneratorBuilder generator = new DirectCandidateGeneratorBuilder(field); - generator.preFilter = other.preFilter; - generator.postFilter = other.postFilter; - generator.suggestMode = other.suggestMode; - generator.accuracy = other.accuracy; - generator.size = other.size; - generator.sort = other.sort; - generator.stringDistance = other.stringDistance; - generator.maxEdits = other.maxEdits; - generator.maxInspections = other.maxInspections; - generator.maxTermFreq = other.maxTermFreq; - generator.prefixLength = other.prefixLength; - generator.minWordLength = other.minWordLength; - generator.minDocFreq = other.minDocFreq; - return generator; - } - /** * Read from a stream. */ @@ -358,35 +331,28 @@ public final class DirectCandidateGeneratorBuilder implements CandidateGenerator } } - private static ObjectParser, DirectCandidateGeneratorBuilder>, QueryParseContext> PARSER = new ObjectParser<>(TYPE); + private static ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + TYPE, args -> new DirectCandidateGeneratorBuilder((String) args[0])); static { - PARSER.declareString((tp, s) -> tp.v1().add(s), FIELDNAME_FIELD); - PARSER.declareString((tp, s) -> tp.v2().preFilter(s), PREFILTER_FIELD); - PARSER.declareString((tp, s) -> tp.v2().postFilter(s), POSTFILTER_FIELD); - PARSER.declareString((tp, s) -> tp.v2().suggestMode(s), SUGGESTMODE_FIELD); - PARSER.declareFloat((tp, f) -> tp.v2().minDocFreq(f), MIN_DOC_FREQ_FIELD); - PARSER.declareFloat((tp, f) -> tp.v2().accuracy(f), ACCURACY_FIELD); - PARSER.declareInt((tp, i) -> tp.v2().size(i), SIZE_FIELD); - PARSER.declareString((tp, s) -> tp.v2().sort(s), SORT_FIELD); - PARSER.declareString((tp, s) -> tp.v2().stringDistance(s), STRING_DISTANCE_FIELD); - PARSER.declareInt((tp, i) -> tp.v2().maxInspections(i), MAX_INSPECTIONS_FIELD); - PARSER.declareFloat((tp, f) -> tp.v2().maxTermFreq(f), MAX_TERM_FREQ_FIELD); - PARSER.declareInt((tp, i) -> tp.v2().maxEdits(i), MAX_EDITS_FIELD); - PARSER.declareInt((tp, i) -> tp.v2().minWordLength(i), MIN_WORD_LENGTH_FIELD); - PARSER.declareInt((tp, i) -> tp.v2().prefixLength(i), PREFIX_LENGTH_FIELD); + PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELDNAME_FIELD); + PARSER.declareString(DirectCandidateGeneratorBuilder::preFilter, PREFILTER_FIELD); + PARSER.declareString(DirectCandidateGeneratorBuilder::postFilter, POSTFILTER_FIELD); + PARSER.declareString(DirectCandidateGeneratorBuilder::suggestMode, SUGGESTMODE_FIELD); + PARSER.declareFloat(DirectCandidateGeneratorBuilder::minDocFreq, MIN_DOC_FREQ_FIELD); + PARSER.declareFloat(DirectCandidateGeneratorBuilder::accuracy, ACCURACY_FIELD); + PARSER.declareInt(DirectCandidateGeneratorBuilder::size, SIZE_FIELD); + PARSER.declareString(DirectCandidateGeneratorBuilder::sort, SORT_FIELD); + PARSER.declareString(DirectCandidateGeneratorBuilder::stringDistance, STRING_DISTANCE_FIELD); + PARSER.declareInt(DirectCandidateGeneratorBuilder::maxInspections, MAX_INSPECTIONS_FIELD); + PARSER.declareFloat(DirectCandidateGeneratorBuilder::maxTermFreq, MAX_TERM_FREQ_FIELD); + PARSER.declareInt(DirectCandidateGeneratorBuilder::maxEdits, MAX_EDITS_FIELD); + PARSER.declareInt(DirectCandidateGeneratorBuilder::minWordLength, MIN_WORD_LENGTH_FIELD); + PARSER.declareInt(DirectCandidateGeneratorBuilder::prefixLength, PREFIX_LENGTH_FIELD); } public static DirectCandidateGeneratorBuilder fromXContent(QueryParseContext parseContext) throws IOException { - DirectCandidateGeneratorBuilder tempGenerator = new DirectCandidateGeneratorBuilder("_na_"); - // bucket for the field name, needed as constructor arg later - Set tmpFieldName = new HashSet<>(1); - PARSER.parse(parseContext.parser(), new Tuple<>(tmpFieldName, tempGenerator), - parseContext); - if (tmpFieldName.size() != 1) { - throw new IllegalArgumentException("[" + TYPE + "] expects exactly one field parameter, but found " + tmpFieldName); - } - return replaceField(tmpFieldName.iterator().next(), tempGenerator); + return PARSER.apply(parseContext.parser(), parseContext); } @Override diff --git a/core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java b/core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java index 846d3193f6d..da15c9c1b8d 100644 --- a/core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java +++ b/core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java @@ -35,6 +35,7 @@ import org.elasticsearch.search.suggest.phrase.PhraseSuggestionContext.DirectCan import org.elasticsearch.test.ESTestCase; import java.io.IOException; + import static org.hamcrest.Matchers.equalTo; public class DirectCandidateGeneratorTests extends ESTestCase{ @@ -151,12 +152,12 @@ public class DirectCandidateGeneratorTests extends ESTestCase{ // test missing fieldname String directGenerator = "{ }"; assertIllegalXContent(directGenerator, IllegalArgumentException.class, - "[direct_generator] expects exactly one field parameter, but found []"); + "Required [field]"); // test two fieldnames directGenerator = "{ \"field\" : \"f1\", \"field\" : \"f2\" }"; - assertIllegalXContent(directGenerator, IllegalArgumentException.class, - "[direct_generator] expects exactly one field parameter, but found [f2, f1]"); + assertIllegalXContent(directGenerator, ParsingException.class, + "[direct_generator] failed to parse field [field]"); // test unknown field directGenerator = "{ \"unknown_param\" : \"f1\" }";