diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/DateHistogramGroupSourceTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/DateHistogramGroupSourceTests.java index 2a448457435..4b78d10df38 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/DateHistogramGroupSourceTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/DateHistogramGroupSourceTests.java @@ -38,7 +38,7 @@ public class DateHistogramGroupSourceTests extends AbstractXContentTestCase { public static HistogramGroupSource randomHistogramGroupSource() { - String field = randomAlphaOfLengthBetween(1, 20); + String field = randomBoolean() ? randomAlphaOfLengthBetween(1, 20) : null; Script script = randomBoolean() ? new Script(randomAlphaOfLengthBetween(1, 10)) : null; boolean missingBucket = randomBoolean(); double interval = randomDoubleBetween(Math.nextUp(0), Double.MAX_VALUE, false); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/TermsGroupSourceTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/TermsGroupSourceTests.java index efae7fb4784..acf42540b6b 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/TermsGroupSourceTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/TermsGroupSourceTests.java @@ -29,8 +29,9 @@ import java.util.function.Predicate; public class TermsGroupSourceTests extends AbstractXContentTestCase { public static TermsGroupSource randomTermsGroupSource() { + String field = randomBoolean() ? randomAlphaOfLengthBetween(1, 20) : null; Script script = randomBoolean() ? new Script(randomAlphaOfLengthBetween(1, 10)) : null; - return new TermsGroupSource(randomAlphaOfLengthBetween(1, 20), script, randomBoolean()); + return new TermsGroupSource(field, script, randomBoolean()); } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GroupConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GroupConfig.java index 4c7dea5b4e8..33e87ff530d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GroupConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GroupConfig.java @@ -54,26 +54,26 @@ public class GroupConfig implements Writeable, ToXContentObject { groups = in.readMap(StreamInput::readString, (stream) -> { SingleGroupSource.Type groupType = SingleGroupSource.Type.fromId(stream.readByte()); switch (groupType) { - case TERMS: - return new TermsGroupSource(stream); - case HISTOGRAM: - return new HistogramGroupSource(stream); - case DATE_HISTOGRAM: - return new DateHistogramGroupSource(stream); - case GEOTILE_GRID: - return new GeoTileGroupSource(stream); - default: - throw new IOException("Unknown group type"); + case TERMS: + return new TermsGroupSource(stream); + case HISTOGRAM: + return new HistogramGroupSource(stream); + case DATE_HISTOGRAM: + return new DateHistogramGroupSource(stream); + case GEOTILE_GRID: + return new GeoTileGroupSource(stream); + default: + throw new IOException("Unknown group type"); } }); } - public Map getGroups() { + public Map getGroups() { return groups; } public boolean isValid() { - return this.groups != null; + return this.groups != null && this.groups.values().stream().allMatch(SingleGroupSource::isValid); } @Override @@ -122,9 +122,11 @@ public class GroupConfig implements Writeable, ToXContentObject { throw new IllegalArgumentException(TransformMessages.TRANSFORM_CONFIGURATION_PIVOT_NO_GROUP_BY); } } else { - try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().map(source); - XContentParser sourceParser = XContentType.JSON.xContent().createParser(registry, LoggingDeprecationHandler.INSTANCE, - BytesReference.bytes(xContentBuilder).streamInput())) { + try ( + XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().map(source); + XContentParser sourceParser = XContentType.JSON.xContent() + .createParser(registry, LoggingDeprecationHandler.INSTANCE, BytesReference.bytes(xContentBuilder).streamInput()) + ) { groups = parseGroupConfig(sourceParser, lenient); } catch (Exception e) { if (lenient) { @@ -137,8 +139,7 @@ public class GroupConfig implements Writeable, ToXContentObject { return new GroupConfig(source, groups); } - private static Map parseGroupConfig(final XContentParser parser, - boolean lenient) throws IOException { + private static Map parseGroupConfig(final XContentParser parser, boolean lenient) throws IOException { Matcher validAggMatcher = AggregatorFactories.VALID_AGG_NAME.matcher(""); LinkedHashMap groups = new LinkedHashMap<>(); @@ -156,8 +157,10 @@ public class GroupConfig implements Writeable, ToXContentObject { ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser); String destinationFieldName = parser.currentName(); if (validAggMatcher.reset(destinationFieldName).matches() == false) { - throw new ParsingException(parser.getTokenLocation(), "Invalid group name [" + destinationFieldName - + "]. Group names can contain any character except '[', ']', and '>'"); + throw new ParsingException( + parser.getTokenLocation(), + "Invalid group name [" + destinationFieldName + "]. Group names can contain any character except '[', ']', and '>'" + ); } token = parser.nextToken(); @@ -170,20 +173,20 @@ public class GroupConfig implements Writeable, ToXContentObject { ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser); SingleGroupSource groupSource; switch (groupType) { - case TERMS: - groupSource = TermsGroupSource.fromXContent(parser, lenient); - break; - case HISTOGRAM: - groupSource = HistogramGroupSource.fromXContent(parser, lenient); - break; - case DATE_HISTOGRAM: - groupSource = DateHistogramGroupSource.fromXContent(parser, lenient); - break; - case GEOTILE_GRID: - groupSource = GeoTileGroupSource.fromXContent(parser, lenient); - break; - default: - throw new ParsingException(parser.getTokenLocation(), "invalid grouping type: " + groupType); + case TERMS: + groupSource = TermsGroupSource.fromXContent(parser, lenient); + break; + case HISTOGRAM: + groupSource = HistogramGroupSource.fromXContent(parser, lenient); + break; + case DATE_HISTOGRAM: + groupSource = DateHistogramGroupSource.fromXContent(parser, lenient); + break; + case GEOTILE_GRID: + groupSource = GeoTileGroupSource.fromXContent(parser, lenient); + break; + default: + throw new ParsingException(parser.getTokenLocation(), "invalid grouping type: " + groupType); } parser.nextToken(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/SingleGroupSource.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/SingleGroupSource.java index c9e983d058b..41e4d7cf4f7 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/SingleGroupSource.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/SingleGroupSource.java @@ -76,6 +76,10 @@ public abstract class SingleGroupSource implements Writeable, ToXContentObject { parser.declareString(optionalConstructorArg(), FIELD); parser.declareObject(optionalConstructorArg(), (p, c) -> ScriptConfig.fromXContent(p, lenient), SCRIPT); parser.declareBoolean(optionalConstructorArg(), MISSING_BUCKET); + if (lenient == false) { + // either a script or a field must be declared, or both + parser.declareRequiredFieldSet(FIELD.getPreferredName(), SCRIPT.getPreferredName()); + } } public SingleGroupSource(final String field, final ScriptConfig scriptConfig, final boolean missingBucket) { @@ -98,6 +102,11 @@ public abstract class SingleGroupSource implements Writeable, ToXContentObject { } } + boolean isValid() { + // either a script or a field must be declared + return field != null || scriptConfig != null; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/DateHistogramGroupSourceTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/DateHistogramGroupSourceTests.java index 448d21bd165..4c3fc91827d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/DateHistogramGroupSourceTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/DateHistogramGroupSourceTests.java @@ -30,10 +30,16 @@ public class DateHistogramGroupSourceTests extends AbstractSerializingTestCase { } } + public void testInvalidGroupSourceValidation() throws IOException { + XContentBuilder source = JsonXContent.contentBuilder() + .startObject() + .startObject(randomAlphaOfLengthBetween(1, 20)) + .startObject("terms") + .endObject() + .endObject() + .endObject(); + + // lenient, passes but reports invalid + try (XContentParser parser = createParser(source)) { + GroupConfig groupConfig = GroupConfig.fromXContent(parser, true); + assertFalse(groupConfig.isValid()); + } + + // strict throws + try (XContentParser parser = createParser(source)) { + Exception e = expectThrows(IllegalArgumentException.class, () -> GroupConfig.fromXContent(parser, false)); + assertTrue(e.getMessage().startsWith("Required one of fields [field, script], but none were specified.")); + } + } + private static Map getSource(SingleGroupSource groupSource) { try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { XContentBuilder content = groupSource.toXContent(xContentBuilder, ToXContent.EMPTY_PARAMS); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/HistogramGroupSourceTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/HistogramGroupSourceTests.java index 432f8779c24..f917d28840d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/HistogramGroupSourceTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/HistogramGroupSourceTests.java @@ -20,10 +20,17 @@ public class HistogramGroupSourceTests extends AbstractSerializingTestCase