From f74663f5560f3fc27ac35c790150ab9bc972d6a9 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Thu, 23 Jan 2020 19:32:12 +0100 Subject: [PATCH 1/2] code formating --- .../transforms/pivot/PivotConfig.java | 55 ++++--- .../transforms/pivot/PivotConfigTests.java | 146 +++++++++--------- 2 files changed, 99 insertions(+), 102 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java index 9d040c1180bd7..1794e4997ab9d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java @@ -42,32 +42,30 @@ public class PivotConfig implements Writeable, ToXContentObject { private static final ConstructingObjectParser LENIENT_PARSER = createParser(true); private static ConstructingObjectParser createParser(boolean lenient) { - ConstructingObjectParser parser = new ConstructingObjectParser<>(NAME, lenient, - args -> { - GroupConfig groups = (GroupConfig) args[0]; - - // allow "aggs" and "aggregations" but require one to be specified - // if somebody specifies both: throw - AggregationConfig aggregationConfig = null; - if (args[1] != null) { - aggregationConfig = (AggregationConfig) args[1]; - } - - if (args[2] != null) { - if (aggregationConfig != null) { - throw new IllegalArgumentException("Found two aggregation definitions: [aggs] and [aggregations]"); - } - aggregationConfig = (AggregationConfig) args[2]; - } - if (aggregationConfig == null) { - throw new IllegalArgumentException("Required [aggregations]"); - } - - return new PivotConfig(groups, aggregationConfig, (Integer)args[3]); - }); - - parser.declareObject(constructorArg(), - (p, c) -> (GroupConfig.fromXContent(p, lenient)), TransformField.GROUP_BY); + ConstructingObjectParser parser = new ConstructingObjectParser<>(NAME, lenient, args -> { + GroupConfig groups = (GroupConfig) args[0]; + + // allow "aggs" and "aggregations" but require one to be specified + // if somebody specifies both: throw + AggregationConfig aggregationConfig = null; + if (args[1] != null) { + aggregationConfig = (AggregationConfig) args[1]; + } + + if (args[2] != null) { + if (aggregationConfig != null) { + throw new IllegalArgumentException("Found two aggregation definitions: [aggs] and [aggregations]"); + } + aggregationConfig = (AggregationConfig) args[2]; + } + if (aggregationConfig == null) { + throw new IllegalArgumentException("Required [aggregations]"); + } + + return new PivotConfig(groups, aggregationConfig, (Integer) args[3]); + }); + + parser.declareObject(constructorArg(), (p, c) -> (GroupConfig.fromXContent(p, lenient)), TransformField.GROUP_BY); parser.declareObject(optionalConstructorArg(), (p, c) -> AggregationConfig.fromXContent(p, lenient), TransformField.AGGREGATIONS); parser.declareObject(optionalConstructorArg(), (p, c) -> AggregationConfig.fromXContent(p, lenient), TransformField.AGGS); @@ -206,17 +204,16 @@ static List aggFieldValidation(List usedNames) { usedNames.sort(String::compareTo); for (int i = 0; i < usedNames.size() - 1; i++) { - if (usedNames.get(i+1).startsWith(usedNames.get(i) + ".")) { + if (usedNames.get(i + 1).startsWith(usedNames.get(i) + ".")) { validationFailures.add("field [" + usedNames.get(i) + "] cannot be both an object and a field"); } - if (usedNames.get(i+1).equals(usedNames.get(i))) { + if (usedNames.get(i + 1).equals(usedNames.get(i))) { validationFailures.add("duplicate field [" + usedNames.get(i) + "] detected"); } } return validationFailures; } - private static void addAggNames(AggregationBuilder aggregationBuilder, Collection names) { names.add(aggregationBuilder.getName()); aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java index 30c310fe3e412..a06d3bdb78c0a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java @@ -24,15 +24,19 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase { public static PivotConfig randomPivotConfig() { - return new PivotConfig(GroupConfigTests.randomGroupConfig(), + return new PivotConfig( + GroupConfigTests.randomGroupConfig(), AggregationConfigTests.randomAggregationConfig(), - randomBoolean() ? null : randomIntBetween(10, 10_000)); + randomBoolean() ? null : randomIntBetween(10, 10_000) + ); } public static PivotConfig randomInvalidPivotConfig() { - return new PivotConfig(GroupConfigTests.randomGroupConfig(), + return new PivotConfig( + GroupConfigTests.randomGroupConfig(), AggregationConfigTests.randomInvalidAggregationConfig(), - randomBoolean() ? null : randomIntBetween(10, 10_000)); + randomBoolean() ? null : randomIntBetween(10, 10_000) + ); } @Override @@ -52,44 +56,39 @@ protected Reader instanceReader() { public void testAggsAbbreviations() throws IOException { String pivotAggs = "{" - + " \"group_by\": {" - + " \"id\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + " \"aggs\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; + + " \"group_by\": {" + + " \"id\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; PivotConfig p1 = createPivotConfigFromString(pivotAggs, false); String pivotAggregations = pivotAggs.replace("aggs", "aggregations"); assertNotEquals(pivotAggs, pivotAggregations); PivotConfig p2 = createPivotConfigFromString(pivotAggregations, false); - assertEquals(p1,p2); + assertEquals(p1, p2); } public void testMissingAggs() throws IOException { - String pivot = "{" - + " \"group_by\": {" - + " \"id\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } } }"; + String pivot = "{" + " \"group_by\": {" + " \"id\": {" + " \"terms\": {" + " \"field\": \"id\"" + "} } } }"; expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } public void testEmptyAggs() throws IOException { String pivot = "{" - + " \"group_by\": {" - + " \"id\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + "\"aggs\": {}" - + " }"; + + " \"group_by\": {" + + " \"id\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + "\"aggs\": {}" + + " }"; expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); @@ -100,12 +99,12 @@ public void testEmptyAggs() throws IOException { public void testEmptyGroupBy() throws IOException { String pivot = "{" - + " \"group_by\": {}," - + " \"aggs\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; + + " \"group_by\": {}," + + " \"aggs\": {" + + " \"avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); @@ -115,34 +114,29 @@ public void testEmptyGroupBy() throws IOException { } public void testMissingGroupBy() { - String pivot = "{" - + " \"aggs\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; + String pivot = "{" + " \"aggs\": {" + " \"avg\": {" + " \"avg\": {" + " \"field\": \"points\"" + "} } } }"; expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } public void testDoubleAggs() { String pivot = "{" - + " \"group_by\": {" - + " \"id\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + " \"aggs\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } }," - + " \"aggregations\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } }" - + "}"; + + " \"group_by\": {" + + " \"id\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } }," + + " \"aggregations\": {" + + " \"avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } }" + + "}"; expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } @@ -171,17 +165,17 @@ public void testAggNameValidationsWithoutIssues() { String nestedField1 = randomAlphaOfLength(10) + "3"; String nestedField2 = randomAlphaOfLength(10) + "4"; - assertThat(PivotConfig.aggFieldValidation(Arrays.asList(prefix + nestedField1 + nestedField2, - prefix + nestedField1, - prefix, - prefix2)), is(empty())); + assertThat( + PivotConfig.aggFieldValidation(Arrays.asList(prefix + nestedField1 + nestedField2, prefix + nestedField1, prefix, prefix2)), + is(empty()) + ); - assertThat(PivotConfig.aggFieldValidation( - Arrays.asList( - dotJoin(prefix, nestedField1, nestedField2), - dotJoin(nestedField1, nestedField2), - nestedField2, - prefix2)), is(empty())); + assertThat( + PivotConfig.aggFieldValidation( + Arrays.asList(dotJoin(prefix, nestedField1, nestedField2), dotJoin(nestedField1, nestedField2), nestedField2, prefix2) + ), + is(empty()) + ); } public void testAggNameValidationsWithDuplicatesAndNestingIssues() { @@ -197,12 +191,18 @@ public void testAggNameValidationsWithDuplicatesAndNestingIssues() { dotJoin(prefix, nestedField1), dotJoin(prefix2, nestedField1), dotJoin(prefix2, nestedField1), - prefix2)); - - assertThat(failures, - containsInAnyOrder("duplicate field [" + dotJoin(prefix2, nestedField1) + "] detected", + prefix2 + ) + ); + + assertThat( + failures, + containsInAnyOrder( + "duplicate field [" + dotJoin(prefix2, nestedField1) + "] detected", "field [" + prefix2 + "] cannot be both an object and a field", - "field [" + dotJoin(prefix, nestedField1) + "] cannot be both an object and a field")); + "field [" + dotJoin(prefix, nestedField1) + "] cannot be both an object and a field" + ) + ); } private static String dotJoin(String... fields) { @@ -210,8 +210,8 @@ private static String dotJoin(String... fields) { } private PivotConfig createPivotConfigFromString(String json, boolean lenient) throws IOException { - final XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry(), - DeprecationHandler.THROW_UNSUPPORTED_OPERATION, json); + final XContentParser parser = XContentType.JSON.xContent() + .createParser(xContentRegistry(), DeprecationHandler.THROW_UNSUPPORTED_OPERATION, json); return PivotConfig.fromXContent(parser, lenient); } } From 234044f9c8ebe0ca40a9d5fe3b6172ec7a480502 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Thu, 23 Jan 2020 19:33:06 +0100 Subject: [PATCH 2/2] disallow fieldnames to start or end with '.' to avoid mapping problems on indexing. --- .../transform/transforms/pivot/PivotConfig.java | 10 ++++++++++ .../transforms/pivot/PivotConfigTests.java | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java index 1794e4997ab9d..d681d5bf04ddd 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java @@ -211,6 +211,16 @@ static List aggFieldValidation(List usedNames) { validationFailures.add("duplicate field [" + usedNames.get(i) + "] detected"); } } + + for (String name : usedNames) { + if (name.startsWith(".")) { + validationFailures.add("field [" + name + "] must not start with '.'"); + } + if (name.endsWith(".")) { + validationFailures.add("field [" + name + "] must not end with '.'"); + } + } + return validationFailures; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java index a06d3bdb78c0a..e9bb572f33d0d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java @@ -205,6 +205,20 @@ public void testAggNameValidationsWithDuplicatesAndNestingIssues() { ); } + public void testAggNameValidationsWithInvalidFieldnames() { + List failures = PivotConfig.aggFieldValidation(Arrays.asList(".at_start", "at_end.", ".start_and_end.")); + + assertThat( + failures, + containsInAnyOrder( + "field [.at_start] must not start with '.'", + "field [at_end.] must not end with '.'", + "field [.start_and_end.] must not start with '.'", + "field [.start_and_end.] must not end with '.'" + ) + ); + } + private static String dotJoin(String... fields) { return Strings.arrayToDelimitedString(fields, "."); }