[ML] Move all Job validation to builder and make Job private (elastic/x-pack-elasticsearch#585)

All validation should be done in the builder and the Job object should just be a dumb, immutable blob.

Original commit: elastic/x-pack-elasticsearch@a78e6edbe9

plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Job.java

@@ -17,6 +17,7 @@ import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser.Token;
 import org.elasticsearch.xpack.ml.job.messages.Messages;
+import org.elasticsearch.xpack.ml.utils.ExceptionsHelper;
 import org.elasticsearch.xpack.ml.utils.MlStrings;
 import org.elasticsearch.xpack.ml.utils.time.TimeUtils;
 
@@ -134,32 +135,11 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContent
     private final String resultsIndexName;
     private final boolean deleted;
 
-    public Job(String jobId, String description, Date createTime, Date finishedTime, Date lastDataTime,
+    private Job(String jobId, String description, Date createTime, Date finishedTime, Date lastDataTime,
                AnalysisConfig analysisConfig, AnalysisLimits analysisLimits, DataDescription dataDescription,
                ModelDebugConfig modelDebugConfig, Long renormalizationWindowDays, Long backgroundPersistInterval,
                Long modelSnapshotRetentionDays, Long resultsRetentionDays, Map<String, Object> customSettings,
                String modelSnapshotId, String resultsIndexName, boolean deleted) {
-        if (analysisConfig == null) {
-            throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_MISSING_ANALYSISCONFIG));
-        }
-
-        checkValueNotLessThan(0, "renormalizationWindowDays", renormalizationWindowDays);
-        checkValueNotLessThan(MIN_BACKGROUND_PERSIST_INTERVAL, "backgroundPersistInterval", backgroundPersistInterval);
-        checkValueNotLessThan(0, "modelSnapshotRetentionDays", modelSnapshotRetentionDays);
-        checkValueNotLessThan(0, "resultsRetentionDays", resultsRetentionDays);
-
-        if (!MlStrings.isValidId(jobId)) {
-            throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_ID, ID.getPreferredName(), jobId));
-        }
-        if (jobId.length() > MAX_JOB_ID_LENGTH) {
-            throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_ID_TOO_LONG, MAX_JOB_ID_LENGTH));
-        }
-
-        if (Strings.isNullOrEmpty(resultsIndexName)) {
-            resultsIndexName = jobId;
-        } else if (!MlStrings.isValidId(resultsIndexName)) {
-            throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_ID, RESULTS_INDEX_NAME.getPreferredName()));
-        }
 
         this.jobId = jobId;
         this.description = description;
@@ -564,7 +544,7 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContent
         }
 
         public void setAnalysisConfig(AnalysisConfig.Builder configBuilder) {
-            analysisConfig = configBuilder.build();
+            analysisConfig = ExceptionsHelper.requireNonNull(configBuilder, ANALYSIS_CONFIG.getPreferredName()).build();
         }
 
         public void setAnalysisLimits(AnalysisLimits analysisLimits) {
@@ -597,7 +577,7 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContent
         }
 
         public void setDataDescription(DataDescription.Builder description) {
-            dataDescription = description.build();
+            dataDescription = ExceptionsHelper.requireNonNull(description, DATA_DESCRIPTION.getPreferredName()).build();
         }
 
         public void setModelDebugConfig(ModelDebugConfig modelDebugConfig) {
@@ -659,6 +639,28 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContent
                 modelSnapshotId = this.modelSnapshotId;
             }
 
+            if (analysisConfig == null) {
+                throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_MISSING_ANALYSISCONFIG));
+            }
+
+            checkValueNotLessThan(0, "renormalizationWindowDays", renormalizationWindowDays);
+            checkValueNotLessThan(MIN_BACKGROUND_PERSIST_INTERVAL, "backgroundPersistInterval", backgroundPersistInterval);
+            checkValueNotLessThan(0, "modelSnapshotRetentionDays", modelSnapshotRetentionDays);
+            checkValueNotLessThan(0, "resultsRetentionDays", resultsRetentionDays);
+
+            if (!MlStrings.isValidId(id)) {
+                throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_ID, ID.getPreferredName(), id));
+            }
+            if (id.length() > MAX_JOB_ID_LENGTH) {
+                throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_ID_TOO_LONG, MAX_JOB_ID_LENGTH));
+            }
+
+            if (Strings.isNullOrEmpty(resultsIndexName)) {
+                resultsIndexName = id;
+            } else if (!MlStrings.isValidId(resultsIndexName)) {
+                throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_ID, RESULTS_INDEX_NAME.getPreferredName()));
+            }
+
             return new Job(
                     id, description, createTime, finishedTime, lastDataTime, analysisConfig, analysisLimits,
                     dataDescription, modelDebugConfig, renormalizationWindowDays, backgroundPersistInterval,

plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsActionResponseTests.java

@@ -36,10 +36,10 @@ public class GetJobsActionResponseTests extends AbstractStreamableTestCase<GetJo
             Date finishedTime = randomBoolean() ? new Date(randomNonNegativeLong()) : null;
             Date lastDataTime = randomBoolean() ? new Date(randomNonNegativeLong()) : null;
             long timeout = randomNonNegativeLong();
-            AnalysisConfig analysisConfig = new AnalysisConfig.Builder(
+            AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(
-                    Collections.singletonList(new Detector.Builder("metric", "some_field").build())).build();
+                    Collections.singletonList(new Detector.Builder("metric", "some_field").build()));
             AnalysisLimits analysisLimits = new AnalysisLimits(randomNonNegativeLong(), randomNonNegativeLong());
-            DataDescription dataDescription = randomBoolean() ? new DataDescription.Builder().build() : null;
+            DataDescription.Builder dataDescription = new DataDescription.Builder();
             ModelDebugConfig modelDebugConfig = randomBoolean() ? new ModelDebugConfig(randomDouble(), randomAsciiOfLength(10)) : null;
             Long normalizationWindowDays = randomBoolean() ? Long.valueOf(randomIntBetween(0, 365)) : null;
             Long backgroundPersistInterval = randomBoolean() ? Long.valueOf(randomIntBetween(3600, 86400)) : null;
@@ -48,11 +48,26 @@ public class GetJobsActionResponseTests extends AbstractStreamableTestCase<GetJo
             Map<String, Object> customConfig = randomBoolean() ? Collections.singletonMap(randomAsciiOfLength(10), randomAsciiOfLength(10))
                     : null;
             String modelSnapshotId = randomBoolean() ? randomAsciiOfLength(10) : null;
-            String indexName = randomBoolean() ? "index" + j : null;
+            String indexName =  "index" + j;
-            Job job = new Job(jobId, description, createTime, finishedTime, lastDataTime,
+            Job.Builder builder = new Job.Builder();
-                    analysisConfig, analysisLimits, dataDescription,
+            builder.setId(jobId);
-                    modelDebugConfig, normalizationWindowDays, backgroundPersistInterval,
+            builder.setDescription(description);
-                    modelSnapshotRetentionDays, resultsRetentionDays, customConfig, modelSnapshotId, indexName, randomBoolean());
+            builder.setCreateTime(createTime);
+            builder.setFinishedTime(finishedTime);
+            builder.setLastDataTime(lastDataTime);
+            builder.setAnalysisConfig(analysisConfig);
+            builder.setAnalysisLimits(analysisLimits);
+            builder.setDataDescription(dataDescription);
+            builder.setModelDebugConfig(modelDebugConfig);
+            builder.setRenormalizationWindowDays(normalizationWindowDays);
+            builder.setBackgroundPersistInterval(backgroundPersistInterval);
+            builder.setModelSnapshotRetentionDays(modelSnapshotRetentionDays);
+            builder.setResultsRetentionDays(resultsRetentionDays);
+            builder.setCustomSettings(customConfig);
+            builder.setModelSnapshotId(modelSnapshotId);
+            builder.setResultsIndexName(indexName);
+            builder.setDeleted(randomBoolean());
+            Job job = builder.build();
 
             jobList.add(job);
         }