From 4d8667077266fe48cb376661fd50c305ce6716c9 Mon Sep 17 00:00:00 2001
From: Zachary Tong <polyfractal@elastic.co>
Date: Wed, 7 Dec 2016 12:48:33 -0500
Subject: [PATCH] Make jobId and from/size mutually exclusive options
 (elastic/elasticsearch#477)

Make jobId and from/size mutually exclusive options.

This approach has the main properties of not allowing an invalid Request to be built, and alerting the user if they set an incorrect configuration. It has the downside that PageParams can be null so the consumer will have to check for it. Since jobId could be null before, this seemed acceptable.

Original commit: elastic/x-pack-elasticsearch@106dcdf61aace4c35e8f6f04cc883eb5dc707da9
---
 .../xpack/prelert/action/GetJobsAction.java   | 22 +++++---
 .../prelert/rest/job/RestGetJobsAction.java   | 14 ++++--
 .../action/GetJobActionRequestTests.java      |  3 +-
 .../rest-api-spec/test/jobs_crud.yaml         | 50 +++++++++++++++++++
 4 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetJobsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetJobsAction.java
index 12867013cfe..b8d68eec3c8 100644
--- a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetJobsAction.java
+++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/action/GetJobsAction.java
@@ -93,8 +93,6 @@ public class GetJobsAction extends Action<GetJobsAction.Request, GetJobsAction.R
         public static final ObjectParser<Request, ParseFieldMatcherSupplier> PARSER = new ObjectParser<>(NAME, Request::new);
         public static final ParseField METRIC = new ParseField("metric");
 
-
-
         static {
             PARSER.declareString(Request::setJobId, Job.ID);
             PARSER.declareObject(Request::setPageParams, PageParams.PARSER, PageParams.PAGE);
@@ -104,20 +102,23 @@ public class GetJobsAction extends Action<GetJobsAction.Request, GetJobsAction.R
             }, METRIC);
         }
 
-        private String jobId;
+        private String jobId = null;
+        private PageParams pageParams = null;
         private boolean config;
         private boolean dataCounts;
         private boolean modelSizeStats;
         private boolean schedulerStatus;
         private boolean status;
-        private PageParams pageParams;
+
 
         public Request() {
-            pageParams = new PageParams();
             config = true;
         }
 
         public void setJobId(String jobId) {
+            if (pageParams != null) {
+                throw new IllegalArgumentException("Cannot set [from, size] when getting a single job.");
+            }
             this.jobId = jobId;
         }
 
@@ -130,6 +131,9 @@ public class GetJobsAction extends Action<GetJobsAction.Request, GetJobsAction.R
         }
 
         public void setPageParams(PageParams pageParams) {
+            if (jobId != null) {
+                throw new IllegalArgumentException("Cannot set [jobId] when getting multiple jobs.");
+            }
             this.pageParams = ExceptionsHelper.requireNonNull(pageParams, PageParams.PAGE.getPreferredName());
         }
 
@@ -508,9 +512,11 @@ public class GetJobsAction extends Action<GetJobsAction.Request, GetJobsAction.R
                         request.getJobId(), jobConfig, dataCounts, modelSizeStats, schedulerStatus, jobStatus);
                 response = new QueryPage<>(Collections.singletonList(jobInfo), 1, Job.RESULTS_FIELD);
 
-            } else {
+            } else if (request.getPageParams() != null) {
                 // Multiple Jobs
-                QueryPage<Job> jobsPage = jobManager.getJobs(request.pageParams.getFrom(), request.pageParams.getSize(), state);
+
+                PageParams pageParams = request.getPageParams();
+                QueryPage<Job> jobsPage = jobManager.getJobs(pageParams.getFrom(), pageParams.getSize(), state);
                 List<Response.JobInfo> jobInfoList = new ArrayList<>();
                 for (Job job : jobsPage.results()) {
                     Job jobConfig = request.config() ? job : null;
@@ -523,6 +529,8 @@ public class GetJobsAction extends Action<GetJobsAction.Request, GetJobsAction.R
                     jobInfoList.add(jobInfo);
                 }
                 response = new QueryPage<>(jobInfoList, jobsPage.count(), Job.RESULTS_FIELD);
+            } else {
+                throw new IllegalStateException("Both jobId and pageParams are null");
             }
 
             listener.onResponse(new Response(response));
diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/job/RestGetJobsAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/job/RestGetJobsAction.java
index d5e5d3f957a..7148a2a5059 100644
--- a/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/job/RestGetJobsAction.java
+++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/prelert/rest/job/RestGetJobsAction.java
@@ -57,13 +57,21 @@ public class RestGetJobsAction extends BaseRestHandler {
             XContentParser parser = XContentFactory.xContent(bodyBytes).createParser(bodyBytes);
             request = GetJobsAction.Request.PARSER.apply(parser, () -> parseFieldMatcher);
         } else {
+            String jobId = restRequest.param(Job.ID.getPreferredName());
             request = new GetJobsAction.Request();
-            request.setJobId(restRequest.param(Job.ID.getPreferredName()));
+            if (jobId != null && !jobId.isEmpty()) {
+                request.setJobId(jobId);
+            }
+            if (restRequest.hasParam(PageParams.FROM.getPreferredName())
+                    || restRequest.hasParam(PageParams.SIZE.getPreferredName())
+                    || jobId == null) {
+                request.setPageParams(new PageParams(restRequest.paramAsInt(PageParams.FROM.getPreferredName(), PageParams.DEFAULT_FROM),
+                        restRequest.paramAsInt(PageParams.SIZE.getPreferredName(), PageParams.DEFAULT_SIZE)));
+            }
             Set<String> stats = Strings.splitStringByCommaToSet(
                     restRequest.param(GetJobsAction.Request.METRIC.getPreferredName(), "config"));
             request.setStats(stats);
-            request.setPageParams(new PageParams(restRequest.paramAsInt(PageParams.FROM.getPreferredName(), PageParams.DEFAULT_FROM),
-                    restRequest.paramAsInt(PageParams.SIZE.getPreferredName(), PageParams.DEFAULT_SIZE)));
+
         }
 
         return channel -> transportGetJobAction.execute(request, new RestStatusToXContentListener<>(channel));
diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/prelert/action/GetJobActionRequestTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/prelert/action/GetJobActionRequestTests.java
index 8fbeb0ed7e3..009c38b9428 100644
--- a/elasticsearch/src/test/java/org/elasticsearch/xpack/prelert/action/GetJobActionRequestTests.java
+++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/prelert/action/GetJobActionRequestTests.java
@@ -24,8 +24,7 @@ public class GetJobActionRequestTests extends AbstractStreamableTestCase<GetJobs
             int maxSize = PageParams.MAX_FROM_SIZE_SUM - from;
             int size = randomInt(maxSize);
             instance.setPageParams(new PageParams(from, size));
-        }
-        if (randomBoolean()) {
+        } else {
             instance.setJobId(randomAsciiOfLengthBetween(1, 20));
         }
         return instance;
diff --git a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml
index b529a271628..10d57d63339 100644
--- a/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml
+++ b/elasticsearch/src/test/resources/rest-api-spec/test/jobs_crud.yaml
@@ -36,6 +36,11 @@
   - match: { count: 1 }
   - match: { jobs.0.config.job_id: "farequote" }
 
+  - do:
+      xpack.prelert.get_jobs: {}
+  - match: { count: 1 }
+  - match: { jobs.0.config.job_id: "farequote" }
+
   - do:
       xpack.prelert.delete_job:
         job_id: "farequote"
@@ -95,3 +100,48 @@
                 "time_format":"yyyy-MM-dd HH:mm:ssX"
             }
           }
+
+---
+"Test jobid + from and/or size":
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        job_id: "job-stats-test"
+        from: 0
+
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        job_id: "job-stats-test"
+        size: 100
+
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        job_id: "job-stats-test"
+        from: 0
+        size: 100
+
+---
+"Test jobid + from and/or size (via body)":
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        body:
+          job_id: "job-stats-test"
+          from: 0
+
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        body:
+          job_id: "job-stats-test"
+          size: 100
+
+  - do:
+      catch: request
+      xpack.prelert.get_jobs:
+        body:
+          job_id: "job-stats-test"
+          from: 0
+          size: 100