mirror of
https://github.com/honeymoose/OpenSearch.git
synced 2025-02-26 14:54:56 +00:00
[Transform] disallow dotted fieldnames (#51369)
adds field validation to disallow output field names starting and/or ending with a '.'. Avoids indexing/mapping problems when starting the transform.
This commit is contained in:
Hendrik Muhs
parent
3443d69883
commit
d46e8c3f7f
@ -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];
|
||||
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];
|
||||
}
|
||||
// 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]");
|
||||
}
|
||||
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]);
|
||||
});
|
||||
return new PivotConfig(groups, aggregationConfig, (Integer) args[3]);
|
||||
});
|
||||
|
||||
parser.declareObject(constructorArg(),
|
||||
(p, c) -> (GroupConfig.fromXContent(p, lenient)), TransformField.GROUP_BY);
|
||||
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,26 @@ public class PivotConfig implements Writeable, ToXContentObject {
|
||||
|
||||
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));
|
||||
|
||||
@ -24,15 +24,19 @@ import static org.hamcrest.Matchers.empty;
|
||||
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
|
||||
@ -52,44 +56,39 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
|
||||
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 class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
|
||||
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 class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
}
|
||||
|
||||
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 class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
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,32 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
dotJoin(prefix, nestedField1),
|
||||
dotJoin(prefix2, nestedField1),
|
||||
dotJoin(prefix2, nestedField1),
|
||||
prefix2));
|
||||
prefix2
|
||||
)
|
||||
);
|
||||
|
||||
assertThat(failures,
|
||||
containsInAnyOrder("duplicate field [" + dotJoin(prefix2, nestedField1) + "] detected",
|
||||
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) {
|
||||
@ -210,8 +224,8 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
|
||||
}
|
||||
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user