From 8e2e26fc44d9ca34441f2345f9e023f1b14384fd Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 7 Apr 2017 16:09:22 +0000 Subject: [PATCH] [ML] Forbid 'by' and 'over' as fieldnames for by/over (elastic/x-pack-elasticsearch#1007) relates elastic/x-pack-elasticsearch#1002 Original commit: elastic/x-pack-elasticsearch@ba9005a58e771de87584cd83e44125f5b4999886 --- .../xpack/ml/job/config/Detector.java | 22 +++- .../xpack/ml/job/messages/Messages.java | 4 + .../ml/job/config/AnalysisConfigTests.java | 8 +- .../xpack/ml/job/config/DetectorTests.java | 106 ++++++++++++------ .../xpack/ml/job/config/JobTests.java | 12 +- 5 files changed, 104 insertions(+), 48 deletions(-) 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 db5d60046a8..12289296d97 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 @@ -137,6 +137,8 @@ public class Detector extends ToXContentToBytes implements Writeable { public static final String NON_NULL_SUM = "non_null_sum"; public static final String LOW_NON_NULL_SUM = "low_non_null_sum"; public static final String HIGH_NON_NULL_SUM = "high_non_null_sum"; + public static final String BY = "by"; + public static final String OVER = "over"; /** * Population variance is called varp to match Splunk */ @@ -674,7 +676,7 @@ public class Detector extends ToXContentToBytes implements Writeable { byFieldName)); } - // by/over field names cannot be "count" - this requirement dates back to the early + // by/over field names cannot be "count", "over', "by" - this requirement dates back to the early // days of the Splunk app and could be removed now BUT ONLY IF THE C++ CODE IS CHANGED // FIRST - see https://github.com/elastic/x-pack-elasticsearch/issues/858 if (COUNT.equals(byFieldName)) { @@ -686,6 +688,24 @@ public class Detector extends ToXContentToBytes implements Writeable { OVER_FIELD_NAME_FIELD.getPreferredName())); } + if (BY.equals(byFieldName)) { + throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_DETECTOR_BY_DISALLOWED, + BY_FIELD_NAME_FIELD.getPreferredName())); + } + if (BY.equals(overFieldName)) { + throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_DETECTOR_BY_DISALLOWED, + OVER_FIELD_NAME_FIELD.getPreferredName())); + } + + if (OVER.equals(byFieldName)) { + throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_DETECTOR_OVER_DISALLOWED, + BY_FIELD_NAME_FIELD.getPreferredName())); + } + if (OVER.equals(overFieldName)) { + throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_DETECTOR_OVER_DISALLOWED, + OVER_FIELD_NAME_FIELD.getPreferredName())); + } + return new Detector(detectorDescription, function, fieldName, byFieldName, overFieldName, partitionFieldName, useNull, excludeFrequent, detectorRules); } 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 83f1489fd04..f98b32198b1 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 @@ -123,6 +123,10 @@ public final class Messages { "{0} and {1} cannot be the same: ''{2}''"; public static final String JOB_CONFIG_DETECTOR_COUNT_DISALLOWED = "''count'' is not a permitted value for {0}"; + public static final String JOB_CONFIG_DETECTOR_BY_DISALLOWED = + "''by'' is not a permitted value for {0}"; + public static final String JOB_CONFIG_DETECTOR_OVER_DISALLOWED = + "''over'' is not a permitted value for {0}"; public static final String JOB_CONFIG_MAPPING_TYPE_CLASH = "A field has a different mapping type to an existing field with the same name. " + "Use the 'results_index_name' setting to assign the job to another index"; 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 205a6d28c69..568ec7dea2f 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 @@ -434,20 +434,20 @@ public class AnalysisConfigTests extends AbstractSerializingTestCase { public void testEquals_GivenEqual() { Detector.Builder builder = new Detector.Builder("mean", "field"); - builder.setByFieldName("by"); - builder.setOverFieldName("over"); + builder.setByFieldName("by_field"); + builder.setOverFieldName("over_field"); builder.setPartitionFieldName("partition"); builder.setUseNull(false); Detector detector1 = builder.build(); builder = new Detector.Builder("mean", "field"); - builder.setByFieldName("by"); - builder.setOverFieldName("over"); + builder.setByFieldName("by_field"); + builder.setOverFieldName("over_field"); builder.setPartitionFieldName("partition"); builder.setUseNull(false); Detector detector2 = builder.build(); @@ -60,7 +60,7 @@ public class DetectorTests extends AbstractSerializingTestCase { Detector.Builder builder = new Detector.Builder(detector2); builder.setByFieldName("by2"); Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule("over", "targetValue", Connective.AND, + DetectionRule rule = new DetectionRule("over_field", "targetValue", Connective.AND, Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by2", "val", condition, null))); builder.setDetectorRules(Collections.singletonList(rule)); detector2 = builder.build(); @@ -82,19 +82,19 @@ public class DetectorTests extends AbstractSerializingTestCase { public void testExtractAnalysisFields() { Detector detector = createDetector().build(); - assertEquals(Arrays.asList("by", "over", "partition"), detector.extractAnalysisFields()); + assertEquals(Arrays.asList("by_field", "over_field", "partition"), detector.extractAnalysisFields()); Detector.Builder builder = new Detector.Builder(detector); builder.setPartitionFieldName(null); detector = builder.build(); - assertEquals(Arrays.asList("by", "over"), detector.extractAnalysisFields()); + assertEquals(Arrays.asList("by_field", "over_field"), detector.extractAnalysisFields()); builder = new Detector.Builder(detector); Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule("over", "targetValue", Connective.AND, + DetectionRule rule = new DetectionRule("over_field", "targetValue", Connective.AND, Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))); builder.setDetectorRules(Collections.singletonList(rule)); builder.setByFieldName(null); detector = builder.build(); - assertEquals(Arrays.asList("over"), detector.extractAnalysisFields()); + assertEquals(Arrays.asList("over_field"), detector.extractAnalysisFields()); builder = new Detector.Builder(detector); rule = new DetectionRule(null, null, Connective.AND, Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))); @@ -107,8 +107,8 @@ public class DetectorTests extends AbstractSerializingTestCase { public void testExtractReferencedLists() { Detector.Builder builder = createDetector(); builder.setDetectorRules(Arrays.asList( - new DetectionRule(null, null, Connective.OR, Arrays.asList(RuleCondition.createCategorical("by", "list1"))), - new DetectionRule(null, null, Connective.OR, Arrays.asList(RuleCondition.createCategorical("by", "list2"))))); + new DetectionRule(null, null, Connective.OR, Arrays.asList(RuleCondition.createCategorical("by_field", "list1"))), + new DetectionRule(null, null, Connective.OR, Arrays.asList(RuleCondition.createCategorical("by_field", "list2"))))); Detector detector = builder.build(); assertEquals(new HashSet<>(Arrays.asList("list1", "list2")), detector.extractReferencedFilters()); @@ -116,13 +116,13 @@ public class DetectorTests extends AbstractSerializingTestCase { private Detector.Builder createDetector() { Detector.Builder detector = new Detector.Builder("mean", "field"); - detector.setByFieldName("by"); - detector.setOverFieldName("over"); + detector.setByFieldName("by_field"); + detector.setOverFieldName("over_field"); detector.setPartitionFieldName("partition"); detector.setUseNull(true); Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule("over", "targetValue", Connective.AND, - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by", "val", condition, null))); + DetectionRule rule = new DetectionRule("over_field", "targetValue", Connective.AND, + Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by_field", "val", condition, null))); detector.setDetectorRules(Arrays.asList(rule)); return detector; } @@ -261,16 +261,16 @@ public class DetectorTests extends AbstractSerializingTestCase { private static Detector.Builder createDetectorWithValidFieldNames() { Detector.Builder d = new Detector.Builder("metric", "field"); - d.setByFieldName("by"); - d.setOverFieldName("over"); + d.setByFieldName("by_field"); + d.setOverFieldName("over_field"); d.setPartitionFieldName("partition"); return d; } private static Detector.Builder createDetectorWithSpecificFieldName(String fieldName) { Detector.Builder d = new Detector.Builder("metric", fieldName); - d.setByFieldName("by"); - d.setOverFieldName("over"); + d.setByFieldName("by_field"); + d.setOverFieldName("over_field"); d.setPartitionFieldName("partition"); return d; } @@ -330,7 +330,7 @@ public class DetectorTests extends AbstractSerializingTestCase { Detector.LOW_NON_ZERO_COUNT, Detector.LOW_NZC, Detector.HIGH_NON_ZERO_COUNT, Detector.HIGH_NZC}) { Detector.Builder builder = new Detector.Builder(f, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); try { builder.build(); Assert.fail("IllegalArgumentException not thrown when expected"); @@ -352,7 +352,7 @@ public class DetectorTests extends AbstractSerializingTestCase { difference.remove(Detector.TIME_OF_WEEK); for (String f : difference) { Detector.Builder builder = new Detector.Builder(f, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); try { builder.build(); Assert.fail("IllegalArgumentException not thrown when expected"); @@ -369,15 +369,15 @@ public class DetectorTests extends AbstractSerializingTestCase { for (String f : new String[]{Detector.COUNT, Detector.HIGH_COUNT, Detector.LOW_COUNT}) { Detector.Builder builder = new Detector.Builder(f, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); builder.build(); builder.build(true); } for (String f : new String[]{Detector.RARE, Detector.FREQ_RARE}) { Detector.Builder builder = new Detector.Builder(f, null); - builder.setOverFieldName("over"); - builder.setByFieldName("by"); + builder.setOverFieldName("over_field"); + builder.setByFieldName("by_field"); builder.build(); builder.build(true); } @@ -393,7 +393,7 @@ public class DetectorTests extends AbstractSerializingTestCase { Detector.LOW_NON_NULL_SUM, Detector.HIGH_NON_NULL_SUM, Detector.POPULATION_VARIANCE, Detector.LOW_POPULATION_VARIANCE, Detector.HIGH_POPULATION_VARIANCE}) { Detector.Builder builder = new Detector.Builder(f, "f"); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); builder.build(); try { builder.build(true); @@ -437,7 +437,7 @@ public class DetectorTests extends AbstractSerializingTestCase { difference.remove(Detector.LAT_LONG); for (String f : difference) { Detector.Builder builder = new Detector.Builder(f, "f"); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); try { builder.build(); Assert.fail("IllegalArgumentException not thrown when expected"); @@ -461,12 +461,12 @@ public class DetectorTests extends AbstractSerializingTestCase { } Detector.Builder builder = new Detector.Builder(Detector.FREQ_RARE, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); builder.setByFieldName("b"); builder.build(); builder.build(true); builder = new Detector.Builder(Detector.FREQ_RARE, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); builder.setByFieldName("b"); builder.build(); @@ -538,7 +538,7 @@ public class DetectorTests extends AbstractSerializingTestCase { builder = new Detector.Builder(Detector.FREQ_RARE, "field"); builder.setByFieldName("b"); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); try { builder.build(); Assert.fail("IllegalArgumentException not thrown when expected"); @@ -554,7 +554,7 @@ public class DetectorTests extends AbstractSerializingTestCase { for (String f : new String[]{Detector.HIGH_COUNT, Detector.LOW_COUNT, Detector.NON_ZERO_COUNT, Detector.NZC}) { builder = new Detector.Builder(f, null); - builder.setByFieldName("by"); + builder.setByFieldName("by_field"); builder.build(); builder.build(true); } @@ -562,7 +562,7 @@ public class DetectorTests extends AbstractSerializingTestCase { for (String f : new String[]{Detector.COUNT, Detector.HIGH_COUNT, Detector.LOW_COUNT}) { builder = new Detector.Builder(f, null); - builder.setOverFieldName("over"); + builder.setOverFieldName("over_field"); builder.build(); builder.build(true); } @@ -570,8 +570,8 @@ public class DetectorTests extends AbstractSerializingTestCase { for (String f : new String[]{Detector.HIGH_COUNT, Detector.LOW_COUNT}) { builder = new Detector.Builder(f, null); - builder.setByFieldName("by"); - builder.setOverFieldName("over"); + builder.setByFieldName("by_field"); + builder.setOverFieldName("over_field"); builder.build(); builder.build(true); } @@ -579,16 +579,16 @@ public class DetectorTests extends AbstractSerializingTestCase { for (String f : new String[]{Detector.NON_ZERO_COUNT, Detector.NZC}) { try { builder = new Detector.Builder(f, "field"); - builder.setByFieldName("by"); - builder.setOverFieldName("over"); + builder.setByFieldName("by_field"); + builder.setOverFieldName("over_field"); builder.build(); Assert.fail("IllegalArgumentException not thrown when expected"); } catch (IllegalArgumentException e) { } try { builder = new Detector.Builder(f, "field"); - builder.setByFieldName("by"); - builder.setOverFieldName("over"); + builder.setByFieldName("by_field"); + builder.setOverFieldName("over_field"); builder.build(true); Assert.fail("IllegalArgumentException not thrown when expected"); } catch (IllegalArgumentException e) { @@ -666,6 +666,38 @@ public class DetectorTests extends AbstractSerializingTestCase { assertEquals("'count' is not a permitted value for over_field_name", e.getMessage()); } + public void testVerify_GivenByIsBy() { + Detector.Builder detector = new Detector.Builder("count", ""); + detector.setByFieldName("by"); + IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, detector::build); + + assertEquals("'by' is not a permitted value for by_field_name", e.getMessage()); + } + + public void testVerify_GivenOverIsBy() { + Detector.Builder detector = new Detector.Builder("count", ""); + detector.setOverFieldName("by"); + IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, detector::build); + + assertEquals("'by' is not a permitted value for over_field_name", e.getMessage()); + } + + public void testVerify_GivenByIsOver() { + Detector.Builder detector = new Detector.Builder("count", ""); + detector.setByFieldName("over"); + IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, detector::build); + + assertEquals("'over' is not a permitted value for by_field_name", e.getMessage()); + } + + public void testVerify_GivenOverIsOver() { + Detector.Builder detector = new Detector.Builder("count", ""); + detector.setOverFieldName("over"); + IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, detector::build); + + assertEquals("'over' is not a permitted value for over_field_name", e.getMessage()); + } + public void testExcludeFrequentForString() { assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("all")); assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("ALL")); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/JobTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/JobTests.java index cc082900fc6..9b61a833c1e 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/JobTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/JobTests.java @@ -161,10 +161,10 @@ public class JobTests extends AbstractSerializingTestCase { */ public void testAnalysisConfigRequiredFields() { Detector.Builder d1 = new Detector.Builder("max", "field"); - d1.setByFieldName("by"); + d1.setByFieldName("by_field"); Detector.Builder d2 = new Detector.Builder("metric", "field2"); - d2.setOverFieldName("over"); + d2.setOverFieldName("over_field"); AnalysisConfig.Builder ac = new AnalysisConfig.Builder(Arrays.asList(d1.build(), d2.build())); ac.setSummaryCountFieldName("agg"); @@ -174,9 +174,9 @@ public class JobTests extends AbstractSerializingTestCase { assertTrue(analysisFields.contains("agg")); assertTrue(analysisFields.contains("field")); - assertTrue(analysisFields.contains("by")); + assertTrue(analysisFields.contains("by_field")); assertTrue(analysisFields.contains("field2")); - assertTrue(analysisFields.contains("over")); + assertTrue(analysisFields.contains("over_field")); assertFalse(analysisFields.contains("max")); assertFalse(analysisFields.contains("")); @@ -193,10 +193,10 @@ public class JobTests extends AbstractSerializingTestCase { assertTrue(analysisFields.contains("partition")); assertTrue(analysisFields.contains("field")); - assertTrue(analysisFields.contains("by")); + assertTrue(analysisFields.contains("by_field")); assertTrue(analysisFields.contains("by2")); assertTrue(analysisFields.contains("field2")); - assertTrue(analysisFields.contains("over")); + assertTrue(analysisFields.contains("over_field")); assertFalse(analysisFields.contains("count")); assertFalse(analysisFields.contains("max"));