[Transform] disallow field and script being empty for group sources (#63313)

fail validation earlier when field and script are both missing in a group source

client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/DateHistogramGroupSourceTests.java

@@ -38,7 +38,7 @@ public class DateHistogramGroupSourceTests extends AbstractXContentTestCase<Date
     }
 
     public static DateHistogramGroupSource randomDateHistogramGroupSource() {
-        String field = randomAlphaOfLengthBetween(1, 20);
+        String field = randomBoolean() ? randomAlphaOfLengthBetween(1, 20) : null;
         Script script = randomBoolean() ? new Script(randomAlphaOfLengthBetween(1, 10)) : null;
 
         return new DateHistogramGroupSource(

client/rest-high-level/src/test/java/org/elasticsearch/client/transform/transforms/pivot/HistogramGroupSourceTests.java

@@ -29,7 +29,7 @@ import java.util.function.Predicate;
 public class HistogramGroupSourceTests extends AbstractXContentTestCase<HistogramGroupSource> {
 
     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);

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<TermsGroupSource> {
 
     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

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 <String, SingleGroupSource> getGroups() {
+    public Map<String, SingleGroupSource> 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<String, SingleGroupSource> parseGroupConfig(final XContentParser parser,
-            boolean lenient) throws IOException {
+    private static Map<String, SingleGroupSource> parseGroupConfig(final XContentParser parser, boolean lenient) throws IOException {
         Matcher validAggMatcher = AggregatorFactories.VALID_AGG_NAME.matcher("");
         LinkedHashMap<String, SingleGroupSource> 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();

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

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<D
     }
 
     public static DateHistogramGroupSource randomDateHistogramGroupSource(Version version) {
-        String field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
-        ScriptConfig scriptConfig = version.onOrAfter(Version.V_7_7_0)
-            ? randomBoolean() ? null : ScriptConfigTests.randomScriptConfig()
-            : null;
+        ScriptConfig scriptConfig = null;
+        String field;
+
+        // either a field or a script must be specified, it's possible to have both, but disallowed to have none
+        if (version.onOrAfter(Version.V_7_7_0) && randomBoolean()) {
+            scriptConfig = ScriptConfigTests.randomScriptConfig();
+            field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
+        } else {
+            field = randomAlphaOfLengthBetween(1, 20);
+        }
         boolean missingBucket = version.onOrAfter(Version.V_7_10_0) ? randomBoolean() : false;
 
         DateHistogramGroupSource dateHistogramGroupSource;

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GeoTileGroupSourceTests.java

@@ -28,7 +28,7 @@ public class GeoTileGroupSourceTests extends AbstractSerializingTestCase<GeoTile
         Rectangle rectangle = GeometryTestUtils.randomRectangle();
         boolean missingBucket = version.onOrAfter(Version.V_7_10_0) ? randomBoolean() : false;
         return new GeoTileGroupSource(
-            randomBoolean() ? null : randomAlphaOfLength(10),
+            randomAlphaOfLength(10),
             missingBucket,
             randomBoolean() ? null : randomIntBetween(1, GeoTileUtils.MAX_ZOOM),
             randomBoolean()

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/GroupConfigTests.java

@@ -130,6 +130,28 @@ public class GroupConfigTests extends AbstractSerializingTestCase<GroupConfig> {
         }
     }
 
+    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<String, Object> getSource(SingleGroupSource groupSource) {
         try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) {
             XContentBuilder content = groupSource.toXContent(xContentBuilder, ToXContent.EMPTY_PARAMS);

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<Histo
     }
 
     public static HistogramGroupSource randomHistogramGroupSource(Version version) {
-        String field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
-        ScriptConfig scriptConfig = version.onOrAfter(Version.V_7_7_0)
-            ? randomBoolean() ? null : ScriptConfigTests.randomScriptConfig()
-            : null;
+        ScriptConfig scriptConfig = null;
+        String field;
+
+        // either a field or a script must be specified, it's possible to have both, but disallowed to have none
+        if (version.onOrAfter(Version.V_7_7_0) && randomBoolean()) {
+            scriptConfig = ScriptConfigTests.randomScriptConfig();
+            field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
+        } else {
+            field = randomAlphaOfLengthBetween(1, 20);
+        }
+
         boolean missingBucket = version.onOrAfter(Version.V_7_10_0) ? randomBoolean() : false;
         double interval = randomDoubleBetween(Math.nextUp(0), Double.MAX_VALUE, false);
         return new HistogramGroupSource(field, scriptConfig, missingBucket, interval);

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/TermsGroupSourceTests.java

@@ -20,10 +20,17 @@ public class TermsGroupSourceTests extends AbstractSerializingTestCase<TermsGrou
     }
 
     public static TermsGroupSource randomTermsGroupSource(Version version) {
-        String field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
-        ScriptConfig scriptConfig = version.onOrAfter(Version.V_7_7_0)
-            ? randomBoolean() ? null : ScriptConfigTests.randomScriptConfig()
-            : null;
+        ScriptConfig scriptConfig = null;
+        String field;
+
+        // either a field or a script must be specified, it's possible to have both, but disallowed to have none
+        if (version.onOrAfter(Version.V_7_7_0) && randomBoolean()) {
+            scriptConfig = ScriptConfigTests.randomScriptConfig();
+            field = randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20);
+        } else {
+            field = randomAlphaOfLengthBetween(1, 20);
+        }
+
         boolean missingBucket = version.onOrAfter(Version.V_7_10_0) ? randomBoolean() : false;
         return new TermsGroupSource(field, scriptConfig, missingBucket);
     }