From 18d15ef2d2ea93b4589496e46b64ed1cf91ffcfe Mon Sep 17 00:00:00 2001 From: David Kyle Date: Tue, 22 Aug 2017 13:33:40 +0100 Subject: [PATCH] Revert "[ML] Missing validations in analysis config (elastic/x-pack-elasticsearch#2103)" This reverts commit elastic/x-pack-elasticsearch@461be12f9f10b0df40ea140fbaf1f88da67cff02. Original commit: elastic/x-pack-elasticsearch@1ce719671005d3525164b24e80b7f64101b25a28 --- .../xpack/ml/job/config/AnalysisConfig.java | 22 +++------- .../xpack/ml/job/config/Detector.java | 20 +++------ .../xpack/ml/job/messages/Messages.java | 4 +- .../datafeed/DatafeedJobValidatorTests.java | 1 + .../ml/job/config/AnalysisConfigTests.java | 44 ++++++------------- .../xpack/ml/job/config/DetectorTests.java | 41 ++++------------- .../ml/job/process/ProcessCtrlTests.java | 1 - 7 files changed, 37 insertions(+), 96 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfig.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfig.java index ee3ad20796e..e3f6b0be735 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfig.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfig.java @@ -530,8 +530,6 @@ public class AnalysisConfig implements ToXContentObject, Writeable { *
  • Check that MULTIPLE_BUCKETSPANS are set appropriately
    • *
  • If Per Partition normalization is configured at least one detector * must have a partition field and no influences can be used
    • - *
  • Field names cannot be empty strings
    • - *
  • If multivariate by fields are enabled a detector must have a by field
    • * */ public AnalysisConfig build() { @@ -561,12 +559,6 @@ public class AnalysisConfig implements ToXContentObject, Writeable { verifyNoInconsistentNestedFieldNames(); verifyInfluencerNames(); - if (multivariateByFields != null && multivariateByFields) { - if (aDetectorHasAByField(detectors) == false) { - throw ExceptionsHelper.badRequestException(Messages.JOB_CONFIG_MULTIVARIATE_REQUIRES_BY_FIELD); - } - } - return new AnalysisConfig(bucketSpan, categorizationFieldName, categorizationFilters, latency, summaryCountFieldName, detectors, influencers, overlappingBuckets, resultFinalizationWindow, multivariateByFields, multipleBucketSpans, usePerPartitionNormalization); @@ -685,19 +677,15 @@ public class AnalysisConfig implements ToXContentObject, Writeable { private void verifyInfluencerNames() { for (String influencer : influencers) { + if (influencer == null || influencer.isEmpty()) { + throw ExceptionsHelper.badRequestException( + Messages.getMessage(Messages.JOB_CONFIG_INFLUENCER_CANNOT_BE_EMPTY)); + } + Detector.Builder.verifyFieldName(influencer); } } - private static boolean aDetectorHasAByField(List detectors) { - for (Detector detector : detectors) { - if (Strings.hasLength(detector.getByFieldName())) { - return true; - } - } - - return false; - } private static void checkDetectorsHavePartitionFields(List detectors) { for (Detector detector : detectors) { if (!Strings.isNullOrEmpty(detector.getPartitionFieldName())) { diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Detector.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Detector.java index 619970ccbbd..c167e75089a 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Detector.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Detector.java @@ -779,19 +779,13 @@ public class Detector implements ToXContentObject, Writeable { * @param field The field name to be validated */ public static void verifyFieldName(String field) throws ElasticsearchParseException { - if (field != null) { - if (field.isEmpty()) { - throw ExceptionsHelper.badRequestException( - Messages.getMessage(Messages.JOB_CONFIG_INFLUENCER_FIELD_NAME_CANNOT_BE_EMPTY)); - } - if (containsInvalidChar(field)) { - throw ExceptionsHelper.badRequestException( - Messages.getMessage(Messages.JOB_CONFIG_INVALID_FIELDNAME_CHARS, field, Detector.PROHIBITED)); - } - if (RecordWriter.CONTROL_FIELD_NAME.equals(field)) { - throw ExceptionsHelper.badRequestException( - Messages.getMessage(Messages.JOB_CONFIG_INVALID_FIELDNAME, field, RecordWriter.CONTROL_FIELD_NAME)); - } + if (field != null && containsInvalidChar(field)) { + throw ExceptionsHelper.badRequestException( + Messages.getMessage(Messages.JOB_CONFIG_INVALID_FIELDNAME_CHARS, field, Detector.PROHIBITED)); + } + if (RecordWriter.CONTROL_FIELD_NAME.equals(field)) { + throw ExceptionsHelper.badRequestException( + Messages.getMessage(Messages.JOB_CONFIG_INVALID_FIELDNAME, field, RecordWriter.CONTROL_FIELD_NAME)); } } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java index 7afb7fc708f..140ad637865 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java @@ -112,7 +112,7 @@ public final class Messages { public static final String JOB_CONFIG_FUNCTION_REQUIRES_OVERFIELD = "over_field_name must be set when the ''{0}'' function is used"; public static final String JOB_CONFIG_ID_ALREADY_TAKEN = "The job cannot be created with the Id ''{0}''. The Id is already used."; public static final String JOB_CONFIG_ID_TOO_LONG = "The job id cannot contain more than {0,number,integer} characters."; - public static final String JOB_CONFIG_INFLUENCER_FIELD_NAME_CANNOT_BE_EMPTY = "Influencers and field names cannot be empty strings"; + public static final String JOB_CONFIG_INFLUENCER_CANNOT_BE_EMPTY = "Influencer names cannot be empty strings"; public static final String JOB_CONFIG_INVALID_CREATE_SETTINGS = "The job is configured with fields [{0}] that are illegal to set at job creation"; public static final String JOB_CONFIG_INVALID_FIELDNAME_CHARS = @@ -124,8 +124,6 @@ public final class Messages { public static final String JOB_CONFIG_MISSING_DATA_DESCRIPTION = "A data_description must be set"; public static final String JOB_CONFIG_MULTIPLE_BUCKETSPANS_MUST_BE_MULTIPLE = "Multiple bucket_span ''{0}'' must be a multiple of the main bucket_span ''{1}''"; - public static final String JOB_CONFIG_MULTIVARIATE_REQUIRES_BY_FIELD = - "The multivariate_by_fields setting requires a detector with a by field"; public static final String JOB_CONFIG_NO_ANALYSIS_FIELD_NOT_COUNT = "Unless the function is 'count' one of field_name, by_field_name or over_field_name must be set"; public static final String JOB_CONFIG_NO_DETECTORS = "No detectors configured"; diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobValidatorTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobValidatorTests.java index bdc21fa95b4..92aaf1597ad 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobValidatorTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobValidatorTests.java @@ -86,6 +86,7 @@ public class DatafeedJobValidatorTests extends ESTestCase { DatafeedConfig.DOC_COUNT); Job.Builder builder = buildJobBuilder("foo"); AnalysisConfig.Builder ac = createAnalysisConfig(); + ac.setSummaryCountFieldName(""); ac.setBucketSpan(TimeValue.timeValueSeconds(1800)); builder.setAnalysisConfig(ac); Job job = builder.build(new Date()); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfigTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfigTests.java index ad26199a425..503749c3ce1 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfigTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/AnalysisConfigTests.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.ml.job.config; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentParser; @@ -41,9 +40,6 @@ public class AnalysisConfigTests extends AbstractSerializingTestCase { } } - public void testEmptyFieldValuesAreNotSet() { - Detector.Builder builder = new Detector.Builder("avg", ""); - expectThrows(ElasticsearchStatusException.class, builder::build); - builder.setFieldName("fname"); - builder.build(); - - builder.setByFieldName(""); - expectThrows(ElasticsearchStatusException.class, builder::build); - builder.setByFieldName("bname"); - builder.build(); - - builder.setOverFieldName(""); - expectThrows(ElasticsearchStatusException.class, builder::build); - builder.setOverFieldName("oname"); - builder.build(); - - builder.setPartitionFieldName(""); - expectThrows(ElasticsearchStatusException.class, builder::build); - builder.setPartitionFieldName("pname"); - builder.build(); - } - public void testVerifyFunctionForPreSummariedInput() { Collection testCaseArguments = getCharactersAndValidity(); for (Object [] args : testCaseArguments) { @@ -559,7 +536,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenSameByAndPartition() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("x"); detector.setPartitionFieldName("x"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -568,7 +545,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenSameByAndOver() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("x"); detector.setOverFieldName("x"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -577,7 +554,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenSameOverAndPartition() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setOverFieldName("x"); detector.setPartitionFieldName("x"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -586,7 +563,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenByIsCount() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("count"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -594,7 +571,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenOverIsCount() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setOverFieldName("count"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -602,7 +579,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenByIsBy() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("by"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -610,7 +587,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenOverIsBy() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setOverFieldName("by"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -618,7 +595,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenByIsOver() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("over"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); @@ -626,7 +603,7 @@ public class DetectorTests extends AbstractSerializingTestCase { } public void testVerify_GivenOverIsOver() { - Detector.Builder detector = new Detector.Builder("count", null); + Detector.Builder detector = new Detector.Builder("count", ""); detector.setOverFieldName("over"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/process/ProcessCtrlTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/process/ProcessCtrlTests.java index fa4168520ea..91ef2840f8b 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/process/ProcessCtrlTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/process/ProcessCtrlTests.java @@ -35,7 +35,6 @@ public class ProcessCtrlTests extends ESTestCase { Job.Builder job = buildJobBuilder("unit-test-job"); Detector.Builder detectorBuilder = new Detector.Builder("mean", "value"); - detectorBuilder.setByFieldName("byField"); detectorBuilder.setPartitionFieldName("foo"); AnalysisConfig.Builder acBuilder = new AnalysisConfig.Builder(Collections.singletonList(detectorBuilder.build())); acBuilder.setBucketSpan(TimeValue.timeValueSeconds(120));