Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Transform] disallow fieldnames with a dot at start and/or end #51369

Merged
merged 2 commits into from
Jan 24, 2020
Merged

[Transform] disallow fieldnames with a dot at start and/or end #51369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f74663f
code formating
Jan 23, 2020
234044f
disallow fieldnames to start or end with '.' to avoid mapping problems
Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions ...re/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java
View file
Open in desktop
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
Open in desktop
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);
}
}