From 9a83ded55399c9e5af9de93f52125984e7e9c7cd Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 4 Oct 2016 20:45:07 -0400 Subject: [PATCH] Add did you mean to strict REST params This commit adds a did you mean feature to the strict REST params error message. This works by comparing any unconsumed parameters to all of the consumer parameters, comparing the Levenstein distance between those parameters, and taking any consumed parameters that are close to an unconsumed parameter as candiates for the did you mean. * Fix pluralization in strict REST params message This commit fixes the pluralization in the strict REST parameters error message so that the word "parameter" is not unconditionally written as "parameters" even when there is only one unrecognized parameter. * Strength strict REST params did you mean test This commit adds an unconsumed parameter that is too far from every consumed parameter to have any candidate suggestions. Relates #20747 --- .../elasticsearch/rest/BaseRestHandler.java | 44 +++++++++++-- .../org/elasticsearch/rest/RestRequest.java | 11 +++- .../admin/indices/RestAnalyzeAction.java | 5 +- .../rest/BaseRestHandlerTests.java | 62 +++++++++++++++++-- 4 files changed, 108 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index fd5ff846529..3bc91dc48e2 100644 --- a/core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -19,8 +19,11 @@ package org.elasticsearch.rest; +import org.apache.lucene.search.spell.LevensteinDistance; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; @@ -28,10 +31,13 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugins.ActionPlugin; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; import java.util.stream.Collectors; /** @@ -59,16 +65,44 @@ public abstract class BaseRestHandler extends AbstractComponent implements RestH final RestChannelConsumer action = prepareRequest(request, client); // validate unconsumed params, but we must exclude params used to format the response - final List unconsumedParams = - request.unconsumedParams().stream().filter(p -> !responseParams().contains(p)).collect(Collectors.toList()); + // use a sorted set so the unconsumed parameters appear in a reliable sorted order + final SortedSet unconsumedParams = + request.unconsumedParams().stream().filter(p -> !responseParams().contains(p)).collect(Collectors.toCollection(TreeSet::new)); // validate the non-response params if (!unconsumedParams.isEmpty()) { - final String message = String.format( + String message = String.format( Locale.ROOT, - "request [%s] contains unrecognized parameters: %s", + "request [%s] contains unrecognized parameter%s: ", request.path(), - unconsumedParams.toString()); + unconsumedParams.size() > 1 ? "s" : ""); + boolean first = true; + for (final String unconsumedParam : unconsumedParams) { + final LevensteinDistance ld = new LevensteinDistance(); + final List> scoredParams = new ArrayList<>(); + for (String consumedParam : request.consumedParams()) { + final float distance = ld.getDistance(unconsumedParam, consumedParam); + if (distance > 0.5f) { + scoredParams.add(new Tuple<>(distance, consumedParam)); + } + } + CollectionUtil.timSort(scoredParams, (a, b) -> { + // sort by distance in reverse order, then parameter name for equal distances + int compare = a.v1().compareTo(b.v1()); + if (compare != 0) return -compare; + else return a.v2().compareTo(b.v2()); + }); + if (first == false) { + message += ", "; + } + message += "[" + unconsumedParam + "]"; + final List keys = scoredParams.stream().map(Tuple::v2).collect(Collectors.toList()); + if (keys.isEmpty() == false) { + message += " -> did you mean " + (keys.size() == 1 ? "[" + keys.get(0) + "]": "any of " + keys.toString()) + "?"; + } + first = false; + } + throw new IllegalArgumentException(message); } diff --git a/core/src/main/java/org/elasticsearch/rest/RestRequest.java b/core/src/main/java/org/elasticsearch/rest/RestRequest.java index 5960fc8979e..3f0f32fff37 100644 --- a/core/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/core/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ToXContent; import java.net.SocketAddress; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -129,6 +128,16 @@ public abstract class RestRequest implements ToXContent.Params { return params; } + /** + * Returns a list of parameters that have been consumed. This method returns a copy, callers + * are free to modify the returned list. + * + * @return the list of currently consumed parameters. + */ + List consumedParams() { + return consumedParams.stream().collect(Collectors.toList()); + } + /** * Returns a list of parameters that have not yet been consumed. This method returns a copy, * callers are free to modify the returned list. diff --git a/core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeAction.java b/core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeAction.java index 4b3035e08b5..247df1a380e 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeAction.java @@ -73,8 +73,9 @@ public class RestAnalyzeAction extends BaseRestHandler { analyzeRequest.text(texts); analyzeRequest.analyzer(request.param("analyzer")); analyzeRequest.field(request.param("field")); - if (request.hasParam("tokenizer")) { - analyzeRequest.tokenizer(request.param("tokenizer")); + final String tokenizer = request.param("tokenizer"); + if (tokenizer != null) { + analyzeRequest.tokenizer(tokenizer); } for (String filter : request.paramAsStringArray("filter", Strings.EMPTY_ARRAY)) { analyzeRequest.addTokenFilter(filter); diff --git a/core/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java b/core/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java index fc5af192b74..8f9dd445369 100644 --- a/core/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java +++ b/core/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java @@ -33,14 +33,34 @@ import java.util.HashMap; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; -import static org.hamcrest.core.AnyOf.anyOf; import static org.hamcrest.core.StringContains.containsString; import static org.hamcrest.object.HasToString.hasToString; import static org.mockito.Mockito.mock; public class BaseRestHandlerTests extends ESTestCase { - public void testUnconsumedParameters() throws Exception { + public void testOneUnconsumedParameters() throws Exception { + final AtomicBoolean executed = new AtomicBoolean(); + BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { + @Override + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + request.param("consumed"); + return channel -> executed.set(true); + } + }; + + final HashMap params = new HashMap<>(); + params.put("consumed", randomAsciiOfLength(8)); + params.put("unconsumed", randomAsciiOfLength(8)); + RestRequest request = new FakeRestRequest.Builder().withParams(params).build(); + RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class))); + assertThat(e, hasToString(containsString("request [/] contains unrecognized parameter: [unconsumed]"))); + assertFalse(executed.get()); + } + + public void testMultipleUnconsumedParameters() throws Exception { final AtomicBoolean executed = new AtomicBoolean(); BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { @Override @@ -56,14 +76,44 @@ public class BaseRestHandlerTests extends ESTestCase { params.put("unconsumed-second", randomAsciiOfLength(8)); RestRequest request = new FakeRestRequest.Builder().withParams(params).build(); RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class))); + assertThat(e, hasToString(containsString("request [/] contains unrecognized parameters: [unconsumed-first], [unconsumed-second]"))); + assertFalse(executed.get()); + } + + public void testUnconsumedParametersDidYouMean() throws Exception { + final AtomicBoolean executed = new AtomicBoolean(); + BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { + @Override + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + request.param("consumed"); + request.param("field"); + request.param("tokenizer"); + request.param("very_close_to_parameter_1"); + request.param("very_close_to_parameter_2"); + return channel -> executed.set(true); + } + }; + + final HashMap params = new HashMap<>(); + params.put("consumed", randomAsciiOfLength(8)); + params.put("flied", randomAsciiOfLength(8)); + params.put("tokenzier", randomAsciiOfLength(8)); + params.put("very_close_to_parametre", randomAsciiOfLength(8)); + params.put("very_far_from_every_consumed_parameter", randomAsciiOfLength(8)); + RestRequest request = new FakeRestRequest.Builder().withParams(params).build(); + RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class))); assertThat( e, - // we can not rely on ordering of the unconsumed parameters here - anyOf( - hasToString(containsString("request [/] contains unrecognized parameters: [unconsumed-first, unconsumed-second]")), - hasToString(containsString("request [/] contains unrecognized parameters: [unconsumed-second, unconsumed-first]")))); + hasToString(containsString( + "request [/] contains unrecognized parameters: " + + "[flied] -> did you mean [field]?, " + + "[tokenzier] -> did you mean [tokenizer]?, " + + "[very_close_to_parametre] -> did you mean any of [very_close_to_parameter_1, very_close_to_parameter_2]?, " + + "[very_far_from_every_consumed_parameter]"))); assertFalse(executed.get()); }