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@106dcdf61a

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));

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));

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;

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