From 6ef954e3f0fd272cfbad454405fc719c44440a1d Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Tue, 13 Dec 2016 11:37:46 -0500 Subject: [PATCH] Add mutually-exclusive param validation to GetCategories (elastic/elasticsearch#512) Also adds body parsing to the REST action, which seemed to be missing (but supported because of the POST) Closes elastic/elasticsearch#430 Original commit: elastic/x-pack-elasticsearch@e7455ffbb662bf92fe6e4a653a67e30220b2f821 --- .../action/GetCategoriesDefinitionAction.java | 29 +++++++++-- .../rest/results/RestGetCategoriesAction.java | 33 +++++++++--- .../test/jobs_get_result_categories.yaml | 51 +++++++++++++++++++ 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetCategoriesDefinitionAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetCategoriesDefinitionAction.java index 3ed8085a6c7..f0166788d90 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetCategoriesDefinitionAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetCategoriesDefinitionAction.java @@ -37,6 +37,8 @@ import org.elasticsearch.xpack.prelert.utils.ExceptionsHelper; import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.action.ValidateActions.addValidationError; + public class GetCategoriesDefinitionAction extends Action { @@ -81,7 +83,7 @@ Action result; if (request.categoryId != null ) { result = jobProvider.categoryDefinition(request.jobId, request.categoryId); + } else if (request.pageParams != null) { + result = jobProvider.categoryDefinitions(request.jobId, request.pageParams.getFrom(), request.pageParams.getSize()); } else { - result = jobProvider.categoryDefinitions(request.jobId, request.pageParams.getFrom(), - request.pageParams.getSize()); + throw new IllegalStateException("Both categoryId and pageParams are null"); } listener.onResponse(new Response(result)); } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/results/RestGetCategoriesAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/results/RestGetCategoriesAction.java index 8906fa9fc57..ab1fae76fed 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/results/RestGetCategoriesAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/results/RestGetCategoriesAction.java @@ -6,8 +6,12 @@ package org.elasticsearch.xpack.prelert.rest.results; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; @@ -44,17 +48,30 @@ public class RestGetCategoriesAction extends BaseRestHandler { @Override protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException { - Request request = new Request(restRequest.param(Job.ID.getPreferredName())); - + Request request; + String jobId = restRequest.param(Job.ID.getPreferredName()); String categoryId = restRequest.param(Request.CATEGORY_ID.getPreferredName()); - if (categoryId != null && !categoryId.isEmpty()) { + BytesReference bodyBytes = restRequest.content(); + + if (bodyBytes != null && bodyBytes.length() > 0) { + XContentParser parser = XContentFactory.xContent(bodyBytes).createParser(bodyBytes); + request = GetCategoriesDefinitionAction.Request.parseRequest(jobId, parser, () -> parseFieldMatcher); request.setCategoryId(categoryId); } else { - PageParams pageParams = new PageParams( - restRequest.paramAsInt(Request.FROM.getPreferredName(), 0), - restRequest.paramAsInt(Request.SIZE.getPreferredName(), 100) - ); - request.setPageParams(pageParams); + + request = new Request(jobId); + if (!Strings.isNullOrEmpty(categoryId)) { + request.setCategoryId(categoryId); + } + if (restRequest.hasParam(Request.FROM.getPreferredName()) + || restRequest.hasParam(Request.SIZE.getPreferredName()) + || Strings.isNullOrEmpty(categoryId)){ + + request.setPageParams(new PageParams( + restRequest.paramAsInt(Request.FROM.getPreferredName(), 0), + restRequest.paramAsInt(Request.SIZE.getPreferredName(), 100) + )); + } } return channel -> transportAction.execute(request, new RestToXContentListener<>(channel)); diff --git a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_get_result_categories.yaml b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_get_result_categories.yaml index a49e61cf00d..09ec5820eca 100644 --- a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_get_result_categories.yaml +++ b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_get_result_categories.yaml @@ -43,3 +43,54 @@ setup: - match: { categories.0.job_id: farequote } - match: { categories.0.category_id: 1 } + +--- +"Test with invalid param combinations": + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + from: 0 + + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + size: 1 + + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + from: 0 + size: 1 + +--- +"Test with invalid param combinations (via body)": + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + body: + from: 0 + + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + body: + size: 1 + + - do: + catch: request + xpack.prelert.get_categories: + job_id: "farequote" + category_id: "1" + body: + from: 0 + size: 1