From 3fd1c90707fa3f09cf59d70c1a6b8478125e688b Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 17 Feb 2017 11:39:41 -0500 Subject: [PATCH] [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@a78e6edbe9db330908931b67872d7816cd6d7679 --- .../xpack/ml/job/config/Job.java | 50 ++++++++++--------- .../ml/action/GetJobsActionResponseTests.java | 31 +++++++++--- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Job.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Job.java index 46dfe2f2c7a..5680251284d 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Job.java +++ b/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 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 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 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 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 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, diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsActionResponseTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsActionResponseTests.java index dbbbe428e0f..36f8cee3d77 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsActionResponseTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsActionResponseTests.java @@ -36,10 +36,10 @@ public class GetJobsActionResponseTests extends AbstractStreamableTestCase customConfig = randomBoolean() ? Collections.singletonMap(randomAsciiOfLength(10), randomAsciiOfLength(10)) : null; String modelSnapshotId = randomBoolean() ? randomAsciiOfLength(10) : null; - String indexName = randomBoolean() ? "index" + j : null; - Job job = new Job(jobId, description, createTime, finishedTime, lastDataTime, - analysisConfig, analysisLimits, dataDescription, - modelDebugConfig, normalizationWindowDays, backgroundPersistInterval, - modelSnapshotRetentionDays, resultsRetentionDays, customConfig, modelSnapshotId, indexName, randomBoolean()); + String indexName = "index" + j; + Job.Builder builder = new Job.Builder(); + builder.setId(jobId); + builder.setDescription(description); + 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); }