Skip to content

Commit

Permalink
[Transform] disallow dotted fieldnames (#51369)
Browse files Browse the repository at this point in the history 
adds field validation to disallow output field names starting and/or ending with a '.'. Avoids
indexing/mapping problems when starting the transform.
  • Loading branch information
Hendrik Muhs committed Jan 24, 2020
1 parent 3443d69 commit d46e8c3
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 102 deletions.
65 changes: 36 additions & 29 deletions ...re/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,32 +42,30 @@ public class PivotConfig implements Writeable, ToXContentObject {
private static final ConstructingObjectParser<PivotConfig, Void> LENIENT_PARSER = createParser(true);

private static ConstructingObjectParser<PivotConfig, Void> createParser(boolean lenient) {
ConstructingObjectParser<PivotConfig, Void> 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<PivotConfig, Void> 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);
Expand Down Expand Up @@ -206,17 +204,26 @@ static List<String> aggFieldValidation(List<String> 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");
}
}

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;
}


private static void addAggNames(AggregationBuilder aggregationBuilder, Collection<String> names) {
names.add(aggregationBuilder.getName());
aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names));
Expand Down
160 changes: 87 additions & 73 deletions ...c/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,19 @@
public class PivotConfigTests extends AbstractSerializingTransformTestCase<PivotConfig> {

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
Expand All @@ -52,44 +56,39 @@ protected Reader<PivotConfig> 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));

Expand All @@ -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));

Expand All @@ -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));
}
Expand Down Expand Up @@ -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() {
Expand All @@ -197,21 +191,41 @@ 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"
)
);
}

public void testAggNameValidationsWithInvalidFieldnames() {
List<String> 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, ".");
}

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);
}
}

0 comments on commit d46e8c3

Please sign in to comment.