From c9128a1e20a915af5927c74a9d95244eb33db2b8 Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 8 May 2015 15:10:33 +0200 Subject: [PATCH] REST: Unify query_string parameters parsing There currently are small differences between search api and count, exists, validate query, explain api when it comes to reading query_string parameters. `analyze_wildcard`, `lowercase_expanded_terms` and `lenient` are only read by the search api and ignored by all other mentioned apis. Unified code to fix this and make sure it doesn't happen again. Also shared some code when it comes to printing out the query as part of SearchSourceBuilder conversion to ToXContent. Extended REST spec to include all the supported params (some that were already supported weren't listed), and added REST tests (also some basic tests for count and search_exists which weren't tested at all). Closes #11057 --- rest-api-spec/api/count.json | 30 ++++++ rest-api-spec/api/indices.validate_query.json | 26 ++++++ rest-api-spec/api/search_exists.json | 30 ++++++ rest-api-spec/test/count/10_basic.yaml | 37 ++++++++ rest-api-spec/test/count/20_query_string.yaml | 79 ++++++++++++++++ .../test/explain/30_query_string.yaml | 93 +++++++++++++++++++ .../20_query_string.yaml | 70 ++++++++++++++ .../test/search/60_query_string.yaml | 79 ++++++++++++++++ .../test/search_exists/10_basic.yaml | 38 ++++++++ .../test/search_exists/20_query_string.yaml | 82 ++++++++++++++++ .../action/support/QuerySourceBuilder.java | 9 +- .../rest/action/search/RestSearchAction.java | 37 +++----- .../rest/action/support/RestActions.java | 3 + .../search/builder/SearchSourceBuilder.java | 36 +++---- 14 files changed, 604 insertions(+), 45 deletions(-) create mode 100644 rest-api-spec/test/count/10_basic.yaml create mode 100644 rest-api-spec/test/count/20_query_string.yaml create mode 100644 rest-api-spec/test/explain/30_query_string.yaml create mode 100644 rest-api-spec/test/indices.validate_query/20_query_string.yaml create mode 100644 rest-api-spec/test/search/60_query_string.yaml create mode 100644 rest-api-spec/test/search_exists/10_basic.yaml create mode 100644 rest-api-spec/test/search_exists/20_query_string.yaml diff --git a/rest-api-spec/api/count.json b/rest-api-spec/api/count.json index 4275ba759ebb3..4a8dace5d5cd0 100644 --- a/rest-api-spec/api/count.json +++ b/rest-api-spec/api/count.json @@ -41,6 +41,36 @@ "routing": { "type" : "string", "description" : "Specific routing value" + }, + "q": { + "type" : "string", + "description" : "Query in the Lucene query string syntax" + }, + "analyzer": { + "type" : "string", + "description" : "The analyzer to use for the query string" + }, + "analyze_wildcard": { + "type" : "boolean", + "description" : "Specify whether wildcard and prefix queries should be analyzed (default: false)" + }, + "default_operator": { + "type" : "enum", + "options" : ["AND","OR"], + "default" : "OR", + "description" : "The default operator for query string query (AND or OR)" + }, + "df": { + "type" : "string", + "description" : "The field to use as default where no field prefix is given in the query string" + }, + "lenient": { + "type" : "boolean", + "description" : "Specify whether format-based query failures (such as providing text to a numeric field) should be ignored" + }, + "lowercase_expanded_terms": { + "type" : "boolean", + "description" : "Specify whether query terms should be lowercased" } } }, diff --git a/rest-api-spec/api/indices.validate_query.json b/rest-api-spec/api/indices.validate_query.json index 1ea9b667d4e21..2b8eb39694d31 100644 --- a/rest-api-spec/api/indices.validate_query.json +++ b/rest-api-spec/api/indices.validate_query.json @@ -40,6 +40,32 @@ "q": { "type" : "string", "description" : "Query in the Lucene query string syntax" + }, + "analyzer": { + "type" : "string", + "description" : "The analyzer to use for the query string" + }, + "analyze_wildcard": { + "type" : "boolean", + "description" : "Specify whether wildcard and prefix queries should be analyzed (default: false)" + }, + "default_operator": { + "type" : "enum", + "options" : ["AND","OR"], + "default" : "OR", + "description" : "The default operator for query string query (AND or OR)" + }, + "df": { + "type" : "string", + "description" : "The field to use as default where no field prefix is given in the query string" + }, + "lenient": { + "type" : "boolean", + "description" : "Specify whether format-based query failures (such as providing text to a numeric field) should be ignored" + }, + "lowercase_expanded_terms": { + "type" : "boolean", + "description" : "Specify whether query terms should be lowercased" } } }, diff --git a/rest-api-spec/api/search_exists.json b/rest-api-spec/api/search_exists.json index 4f52b27267351..a8970467d1c11 100644 --- a/rest-api-spec/api/search_exists.json +++ b/rest-api-spec/api/search_exists.json @@ -41,6 +41,36 @@ "routing": { "type" : "string", "description" : "Specific routing value" + }, + "q": { + "type" : "string", + "description" : "Query in the Lucene query string syntax" + }, + "analyzer": { + "type" : "string", + "description" : "The analyzer to use for the query string" + }, + "analyze_wildcard": { + "type" : "boolean", + "description" : "Specify whether wildcard and prefix queries should be analyzed (default: false)" + }, + "default_operator": { + "type" : "enum", + "options" : ["AND","OR"], + "default" : "OR", + "description" : "The default operator for query string query (AND or OR)" + }, + "df": { + "type" : "string", + "description" : "The field to use as default where no field prefix is given in the query string" + }, + "lenient": { + "type" : "boolean", + "description" : "Specify whether format-based query failures (such as providing text to a numeric field) should be ignored" + }, + "lowercase_expanded_terms": { + "type" : "boolean", + "description" : "Specify whether query terms should be lowercased" } } }, diff --git a/rest-api-spec/test/count/10_basic.yaml b/rest-api-spec/test/count/10_basic.yaml new file mode 100644 index 0000000000000..2495f2961210d --- /dev/null +++ b/rest-api-spec/test/count/10_basic.yaml @@ -0,0 +1,37 @@ +--- +"count with body": + - do: + indices.create: + index: test + - do: + index: + index: test + type: test + id: 1 + body: { foo: bar } + + - do: + indices.refresh: + index: [test] + + - do: + count: + index: test + type: test + body: + query: + match: + foo: bar + + - match: {count : 1} + + - do: + count: + index: test + type: test + body: + query: + match: + foo: test + + - match: {count : 0} diff --git a/rest-api-spec/test/count/20_query_string.yaml b/rest-api-spec/test/count/20_query_string.yaml new file mode 100644 index 0000000000000..933033761e9c1 --- /dev/null +++ b/rest-api-spec/test/count/20_query_string.yaml @@ -0,0 +1,79 @@ +--- +"count with query_string parameters": + - do: + indices.create: + index: test + body: + mappings: + test: + _all: + enabled: false + properties: + number: + type: integer + + - do: + index: + index: test + type: test + id: 1 + body: { field: foo bar} + + - do: + indices.refresh: + index: [test] + + - do: + count: + index: test + q: bar + df: field + + - match: {count : 1} + + - do: + count: + index: test + q: field:foo field:xyz + + - match: {count : 1} + + - do: + count: + index: test + q: field:foo field:xyz + default_operator: AND + + - match: {count : 0} + + - do: + count: + index: test + q: field:bars + analyzer: snowball + + - match: {count : 1} + + - do: + count: + index: test + q: field:BA* + lowercase_expanded_terms: false + + - match: {count : 0} + + - do: + count: + index: test + q: field:BA* + analyze_wildcard: true + + - match: {count : 1} + + - do: + count: + index: test + q: number:foo + lenient: true + + - match: {count : 0} diff --git a/rest-api-spec/test/explain/30_query_string.yaml b/rest-api-spec/test/explain/30_query_string.yaml new file mode 100644 index 0000000000000..30fe6cc55b621 --- /dev/null +++ b/rest-api-spec/test/explain/30_query_string.yaml @@ -0,0 +1,93 @@ +--- +"explain with query_string parameters": + - do: + indices.create: + index: test + body: + mappings: + test: + _all: + enabled: false + properties: + number: + type: integer + + - do: + index: + index: test + type: test + id: 1 + body: { field: foo bar} + + - do: + indices.refresh: + index: [test] + + - do: + explain: + index: test + type: test + id: 1 + q: bar + df: field + + - is_true: matched + + - do: + explain: + index: test + type: test + id: 1 + q: field:foo field:xyz + + - is_true: matched + + - do: + explain: + index: test + type: test + id: 1 + q: field:foo field:xyz + default_operator: AND + + - is_false: matched + + - do: + explain: + index: test + type: test + id: 1 + q: field:bars + analyzer: snowball + + - is_true: matched + + - do: + explain: + index: test + type: test + id: 1 + q: field:BA* + lowercase_expanded_terms: false + + - is_false: matched + + - do: + explain: + index: test + type: test + id: 1 + q: field:BA* + analyze_wildcard: true + + - is_true: matched + + - do: + explain: + index: test + type: test + id: 1 + q: number:foo + lenient: true + + - is_false: matched diff --git a/rest-api-spec/test/indices.validate_query/20_query_string.yaml b/rest-api-spec/test/indices.validate_query/20_query_string.yaml new file mode 100644 index 0000000000000..1207bc2a8ad96 --- /dev/null +++ b/rest-api-spec/test/indices.validate_query/20_query_string.yaml @@ -0,0 +1,70 @@ +--- +"validate_query with query_string parameters": + - do: + indices.create: + index: test + body: + mappings: + test: + _all: + enabled: false + properties: + field: + type: string + number: + type: integer + + - do: + indices.validate_query: + index: test + q: bar + df: field + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: field:foo field:xyz + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: field:foo field:xyz + default_operator: AND + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: field:bars + analyzer: snowball + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: field:BA* + lowercase_expanded_terms: false + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: field:BA* + analyze_wildcard: true + + - is_true: valid + + - do: + indices.validate_query: + index: test + q: number:foo + lenient: true + + - is_true: valid diff --git a/rest-api-spec/test/search/60_query_string.yaml b/rest-api-spec/test/search/60_query_string.yaml new file mode 100644 index 0000000000000..6fb93bb10449a --- /dev/null +++ b/rest-api-spec/test/search/60_query_string.yaml @@ -0,0 +1,79 @@ +--- +"search with query_string parameters": + - do: + indices.create: + index: test + body: + mappings: + test: + _all: + enabled: false + properties: + number: + type: integer + + - do: + index: + index: test + type: test + id: 1 + body: { field: foo bar} + + - do: + indices.refresh: + index: [test] + + - do: + search: + index: test + q: bar + df: field + + - match: {hits.total: 1} + + - do: + search: + index: test + q: field:foo field:xyz + + - match: {hits.total: 1} + + - do: + search: + index: test + q: field:foo field:xyz + default_operator: AND + + - match: {hits.total: 0} + + - do: + search: + index: test + q: field:bars + analyzer: snowball + + - match: {hits.total: 1} + + - do: + search: + index: test + q: field:BA* + lowercase_expanded_terms: false + + - match: {hits.total: 0} + + - do: + search: + index: test + q: field:BA* + analyze_wildcard: true + + - match: {hits.total: 1} + + - do: + search: + index: test + q: number:foo + lenient: true + + - match: {hits.total: 0} diff --git a/rest-api-spec/test/search_exists/10_basic.yaml b/rest-api-spec/test/search_exists/10_basic.yaml new file mode 100644 index 0000000000000..6045d9b55e470 --- /dev/null +++ b/rest-api-spec/test/search_exists/10_basic.yaml @@ -0,0 +1,38 @@ +--- +"search_exists with body": + - do: + indices.create: + index: test + - do: + index: + index: test + type: test + id: 1 + body: { foo: bar } + + - do: + indices.refresh: + index: [test] + + - do: + search_exists: + index: test + type: test + body: + query: + match: + foo: bar + + - is_true: exists + + - do: + catch: missing + search_exists: + index: test + type: test + body: + query: + match: + foo: test + + - is_false: exists diff --git a/rest-api-spec/test/search_exists/20_query_string.yaml b/rest-api-spec/test/search_exists/20_query_string.yaml new file mode 100644 index 0000000000000..11535fd6a262d --- /dev/null +++ b/rest-api-spec/test/search_exists/20_query_string.yaml @@ -0,0 +1,82 @@ +--- +"search_exists with query_string parameters": + - do: + indices.create: + index: test + body: + mappings: + test: + _all: + enabled: false + properties: + number: + type: integer + + - do: + index: + index: test + type: test + id: 1 + body: { field: foo bar} + + - do: + indices.refresh: + index: [test] + + - do: + search_exists: + index: test + q: bar + df: field + + - is_true: exists + + - do: + search_exists: + index: test + q: field:foo field:xyz + + - is_true: exists + + - do: + catch: missing + search_exists: + index: test + q: field:foo field:xyz + default_operator: AND + + - is_false: exists + + - do: + search_exists: + index: test + q: field:bars + analyzer: snowball + + - is_true: exists + + - do: + catch: missing + search_exists: + index: test + q: field:BA* + lowercase_expanded_terms: false + + - is_false: exists + + - do: + search_exists: + index: test + q: field:BA* + analyze_wildcard: true + + - is_true: exists + + - do: + catch: missing + search_exists: + index: test + q: number:foo + lenient: true + + - is_false: exists diff --git a/src/main/java/org/elasticsearch/action/support/QuerySourceBuilder.java b/src/main/java/org/elasticsearch/action/support/QuerySourceBuilder.java index 69fcda47e6c1b..bf2cd63905dc0 100644 --- a/src/main/java/org/elasticsearch/action/support/QuerySourceBuilder.java +++ b/src/main/java/org/elasticsearch/action/support/QuerySourceBuilder.java @@ -48,6 +48,12 @@ public QuerySourceBuilder setQuery(BytesReference queryBinary) { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); + innerToXContent(builder, params); + builder.endObject(); + return builder; + } + + public void innerToXContent(XContentBuilder builder, Params params) throws IOException { if (queryBuilder != null) { builder.field("query"); queryBuilder.toXContent(builder, params); @@ -60,9 +66,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("query_binary", queryBinary); } } - - builder.endObject(); - return builder; } public BytesReference buildAsBytes(XContentType contentType) throws SearchSourceBuilderException { diff --git a/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index a0e04a1af063e..b1a47dde83189 100644 --- a/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -23,14 +23,17 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.IndicesOptions; +import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.index.query.QueryStringQueryBuilder; -import org.elasticsearch.rest.*; +import org.elasticsearch.rest.BaseRestHandler; +import org.elasticsearch.rest.RestChannel; +import org.elasticsearch.rest.RestController; +import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.exists.RestExistsAction; +import org.elasticsearch.rest.action.support.RestActions; import org.elasticsearch.rest.action.support.RestStatusToXContentListener; import org.elasticsearch.search.Scroll; import org.elasticsearch.search.builder.SearchSourceBuilder; @@ -123,28 +126,10 @@ public static SearchRequest parseSearchRequest(RestRequest request) { public static SearchSourceBuilder parseSearchSource(RestRequest request) { SearchSourceBuilder searchSourceBuilder = null; - String queryString = request.param("q"); - if (queryString != null) { - QueryStringQueryBuilder queryBuilder = QueryBuilders.queryStringQuery(queryString); - queryBuilder.defaultField(request.param("df")); - queryBuilder.analyzer(request.param("analyzer")); - queryBuilder.analyzeWildcard(request.paramAsBoolean("analyze_wildcard", false)); - queryBuilder.lowercaseExpandedTerms(request.paramAsBoolean("lowercase_expanded_terms", true)); - queryBuilder.lenient(request.paramAsBoolean("lenient", null)); - String defaultOperator = request.param("default_operator"); - if (defaultOperator != null) { - if ("OR".equals(defaultOperator)) { - queryBuilder.defaultOperator(QueryStringQueryBuilder.Operator.OR); - } else if ("AND".equals(defaultOperator)) { - queryBuilder.defaultOperator(QueryStringQueryBuilder.Operator.AND); - } else { - throw new ElasticsearchIllegalArgumentException("Unsupported defaultOperator [" + defaultOperator + "], can either be [OR] or [AND]"); - } - } - if (searchSourceBuilder == null) { - searchSourceBuilder = new SearchSourceBuilder(); - } - searchSourceBuilder.query(queryBuilder); + QuerySourceBuilder querySourceBuilder = RestActions.parseQuerySource(request); + if (querySourceBuilder != null) { + searchSourceBuilder = new SearchSourceBuilder(); + searchSourceBuilder.query(querySourceBuilder); } int from = request.paramAsInt("from", -1); @@ -256,7 +241,7 @@ public static SearchSourceBuilder parseSearchSource(RestRequest request) { String suggestField = request.param("suggest_field"); if (suggestField != null) { - String suggestText = request.param("suggest_text", queryString); + String suggestText = request.param("suggest_text", request.param("q")); int suggestSize = request.paramAsInt("suggest_size", 5); if (searchSourceBuilder == null) { searchSourceBuilder = new SearchSourceBuilder(); diff --git a/src/main/java/org/elasticsearch/rest/action/support/RestActions.java b/src/main/java/org/elasticsearch/rest/action/support/RestActions.java index b3fe7a5d344fd..e39d22813a376 100644 --- a/src/main/java/org/elasticsearch/rest/action/support/RestActions.java +++ b/src/main/java/org/elasticsearch/rest/action/support/RestActions.java @@ -98,6 +98,9 @@ public static QuerySourceBuilder parseQuerySource(RestRequest request) { QueryStringQueryBuilder queryBuilder = QueryBuilders.queryStringQuery(queryString); queryBuilder.defaultField(request.param("df")); queryBuilder.analyzer(request.param("analyzer")); + queryBuilder.analyzeWildcard(request.paramAsBoolean("analyze_wildcard", false)); + queryBuilder.lowercaseExpandedTerms(request.paramAsBoolean("lowercase_expanded_terms", true)); + queryBuilder.lenient(request.paramAsBoolean("lenient", null)); String defaultOperator = request.param("default_operator"); if (defaultOperator != null) { if ("OR".equals(defaultOperator)) { diff --git a/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 1c4b3113cd501..f7b8d0758bd64 100644 --- a/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -26,6 +26,7 @@ import org.elasticsearch.ElasticsearchGenerationException; import org.elasticsearch.ElasticsearchIllegalArgumentException; import org.elasticsearch.ExceptionsHelper; +import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.client.Requests; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; @@ -78,9 +79,7 @@ public static HighlightBuilder highlight() { return new HighlightBuilder(); } - private QueryBuilder queryBuilder; - - private BytesReference queryBinary; + private QuerySourceBuilder querySourceBuilder; private FilterBuilder postFilterBuilder; @@ -136,13 +135,24 @@ public static HighlightBuilder highlight() { public SearchSourceBuilder() { } + /** + * Sets the query provided as a {@link QuerySourceBuilder} + */ + public SearchSourceBuilder query(QuerySourceBuilder querySourceBuilder) { + this.querySourceBuilder = querySourceBuilder; + return this; + } + /** * Constructs a new search source builder with a search query. * * @see org.elasticsearch.index.query.QueryBuilders */ public SearchSourceBuilder query(QueryBuilder query) { - this.queryBuilder = query; + if (this.querySourceBuilder == null) { + this.querySourceBuilder = new QuerySourceBuilder(); + } + this.querySourceBuilder.setQuery(query); return this; } @@ -164,7 +174,10 @@ public SearchSourceBuilder query(byte[] queryBinary, int queryBinaryOffset, int * Constructs a new search source builder with a raw search query. */ public SearchSourceBuilder query(BytesReference queryBinary) { - this.queryBinary = queryBinary; + if (this.querySourceBuilder == null) { + this.querySourceBuilder = new QuerySourceBuilder(); + } + this.querySourceBuilder.setQuery(queryBinary); return this; } @@ -772,17 +785,8 @@ public void innerToXContent(XContentBuilder builder, Params params) throws IOExc builder.field("terminate_after", terminateAfter); } - if (queryBuilder != null) { - builder.field("query"); - queryBuilder.toXContent(builder, params); - } - - if (queryBinary != null) { - if (XContentFactory.xContentType(queryBinary) == builder.contentType()) { - builder.rawField("query", queryBinary); - } else { - builder.field("query_binary", queryBinary); - } + if (querySourceBuilder != null) { + querySourceBuilder.innerToXContent(builder, params); } if (postFilterBuilder != null) {