[ML] verify that there are no duplicate leaf fields in aggs

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java

@@ -20,6 +20,8 @@
 import java.io.IOException;
 import java.util.Objects;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 public class PutDataFrameTransformAction extends Action<AcknowledgedResponse> {
 
     public static final PutDataFrameTransformAction INSTANCE = new PutDataFrameTransformAction();
@@ -53,7 +55,11 @@ public static Request fromXContent(final XContentParser parser, final String id)
 
         @Override
         public ActionRequestValidationException validate() {
-            return null;
+            ActionRequestValidationException validationException = null;
+            for(String failure : config.getPivotConfig().aggFieldValidation()) {
+                validationException = addValidationError(failure, validationException);
+            }
+            return validationException;
         }
 
         @Override

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java

@@ -13,13 +13,20 @@
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.search.aggregations.AggregationBuilder;
+import org.elasticsearch.search.aggregations.PipelineAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.composite.CompositeAggregationBuilder;
 import org.elasticsearch.xpack.core.dataframe.DataFrameField;
 import org.elasticsearch.xpack.core.dataframe.utils.ExceptionsHelper;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map.Entry;
 import java.util.Objects;
+import java.util.Set;
 
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
@@ -144,4 +151,66 @@ public boolean isValid() {
     public static PivotConfig fromXContent(final XContentParser parser, boolean lenient) throws IOException {
         return lenient ? LENIENT_PARSER.apply(parser, null) : STRICT_PARSER.apply(parser, null);
     }
+
+    /**
+     * Does the following checks:
+     *
+     *  - determines if there are any full duplicate names between the aggregation names and the group by names.
+     *  - finds if there are conflicting name paths that could cause a failure later when the config is started.
+     *
+     * Examples showing conflicting field name paths:
+     *
+     * aggName1: foo.bar.baz
+     * aggName2: foo.bar
+     *
+     * This should fail as aggName1 will cause foo.bar to be an object, causing a conflict with the use of foo.bar in aggName2.
+     *
+     * @return List of validation failure messages
+     */
+    public List<String> aggFieldValidation() {
+        if ((aggregationConfig.isValid() && groups.isValid()) == false) {
+            return Collections.emptyList();
+        }
+
+        List<String> validationFailures = new ArrayList<>();
+        List<String> usedNames = new ArrayList<>();
+        // TODO this will need changed once we allow multi-bucket aggs + field merging
+        aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames));
+        aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames));
+        usedNames.addAll(groups.getGroups().keySet());
+
+        Set<String> leafNames = new HashSet<>(usedNames.size());
+        for(String name : usedNames) {
+            if (leafNames.contains(name)) {
+                validationFailures.add("duplicate field [" + name + "] detected");
+            }
+            leafNames.add(name);
+        }
+
+        for (String fullName : usedNames) {
+            String[] tokens = fullName.split("\\.");
+            for (int i = tokens.length - 1; i > 0; i--) {
+                StringBuilder prefix = new StringBuilder();
+                for (int j = 0; j < i; j++) {
+                    prefix.append(tokens[j]);
+                }
+                String prefixString = prefix.toString();
+                if (leafNames.contains(prefixString)) {
+                    validationFailures.add("field [" + prefixString + "] cannot be both an object and a field");
+                }
+            }
+        }
+        return validationFailures;
+    }
+
+
+    private static void addAggNames(AggregationBuilder aggregationBuilder, List<String> names) {
+        names.add(aggregationBuilder.getName());
+        aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names));
+        aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(agg, names));
+    }
+
+    private static void addAggNames(PipelineAggregationBuilder pipelineAggregationBuilder, List<String> names) {
+        names.add(pipelineAggregationBuilder.getName());
+    }
 }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java

@@ -13,6 +13,9 @@
 import org.elasticsearch.xpack.core.dataframe.transforms.AbstractSerializingDataFrameTestCase;
 
 import java.io.IOException;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.equalTo;
 
 public class PivotConfigTests extends AbstractSerializingDataFrameTestCase<PivotConfig> {
 
@@ -103,7 +106,7 @@ public void testEmptyGroupBy() throws IOException {
         assertFalse(pivotConfig.isValid());
     }
 
-    public void testMissingGroupBy() throws IOException {
+    public void testMissingGroupBy() {
         String pivot = "{"
                 + " \"aggs\": {"
                 + "   \"avg\": {"
@@ -114,7 +117,7 @@ public void testMissingGroupBy() throws IOException {
         expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false));
     }
 
-    public void testDoubleAggs() throws IOException {
+    public void testDoubleAggs() {
         String pivot = "{"
                 + " \"group_by\": {"
                 + "   \"id\": {"
@@ -136,6 +139,74 @@ public void testDoubleAggs() throws IOException {
         expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false));
     }
 
+    public void testAggNameValidations() throws IOException {
+        String pivotAggs = "{"
+            + " \"group_by\": {"
+            + "   \"user\": {"
+            + "     \"terms\": {"
+            + "       \"field\": \"id\""
+            + "} } },"
+            + " \"aggs\": {"
+            + "   \"user.avg\": {"
+            + "     \"avg\": {"
+            + "       \"field\": \"points\""
+            + "} } } }";
+        PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true);
+        assertTrue(pivotConfig.isValid());
+        List<String> fieldValidation = pivotConfig.aggFieldValidation();
+        assertFalse(fieldValidation.isEmpty());
+        assertThat(fieldValidation.get(0), equalTo("field [user] cannot be both an object and a field"));
+
+        pivotAggs = "{"
+            + " \"group_by\": {"
+            + "   \"user\": {"
+            + "     \"terms\": {"
+            + "       \"field\": \"id\""
+            + "} } },"
+            + " \"aggs\": {"
+            + "   \"user\": {"
+            + "     \"avg\": {"
+            + "       \"field\": \"points\""
+            + "} } } }";
+        pivotConfig = createPivotConfigFromString(pivotAggs, true);
+        assertTrue(pivotConfig.isValid());
+        fieldValidation = pivotConfig.aggFieldValidation();
+        assertFalse(fieldValidation.isEmpty());
+        assertThat(fieldValidation.get(0), equalTo("duplicate field [user] detected"));
+
+        pivotAggs = "{"
+            + " \"group_by\": {"
+            + "   \"user\": {"
+            + "     \"terms\": {"
+            + "       \"field\": \"id\""
+            + "} } },"
+            + " \"aggs\": {"
+            + "   \"avg\": {"
+            + "     \"avg\": {"
+            + "       \"field\": \"points\""
+            + "} } } }";
+        pivotConfig = createPivotConfigFromString(pivotAggs, true);
+        assertTrue(pivotConfig.isValid());
+        fieldValidation = pivotConfig.aggFieldValidation();
+        assertTrue(fieldValidation.isEmpty());
+
+        pivotAggs = "{"
+            + " \"group_by\": {"
+            + "   \"user.id.field\": {"
+            + "     \"terms\": {"
+            + "       \"field\": \"id\""
+            + "} } },"
+            + " \"aggs\": {"
+            + "   \"avg.field.value\": {"
+            + "     \"avg\": {"
+            + "       \"field\": \"points\""
+            + "} } } }";
+        pivotConfig = createPivotConfigFromString(pivotAggs, true);
+        assertTrue(pivotConfig.isValid());
+        fieldValidation = pivotConfig.aggFieldValidation();
+        assertTrue(fieldValidation.isEmpty());
+    }
+
     private PivotConfig createPivotConfigFromString(String json, boolean lenient) throws IOException {
         final XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry(),
                 DeprecationHandler.THROW_UNSUPPORTED_OPERATION, json);

x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/transforms_crud.yml

@@ -302,3 +302,35 @@ setup:
               "aggs": {"avg_response": {"avg": {"field": "responsetime"}}}
             }
           }
+---
+"Test creation failures due to duplicate and conflicting field names":
+  - do:
+      catch: /duplicate field \[airline\] detected/
+      data_frame.put_data_frame_transform:
+        transform_id: "duplicate-field-transform"
+        body: >
+          {
+            "source": {
+              "index": "source-index"
+            },
+            "dest": { "index": "dest-index" },
+            "pivot": {
+              "group_by": { "airline": {"terms": {"field": "airline"}}},
+              "aggs": {"airline": {"avg": {"field": "responsetime"}}}
+            }
+          }
+  - do:
+      catch: /field \[airline\] cannot be both an object and a field/
+      data_frame.put_data_frame_transform:
+        transform_id: "duplicate-field-transform"
+        body: >
+          {
+            "source": {
+              "index": "source-index"
+            },
+            "dest": { "index": "dest-index" },
+            "pivot": {
+              "group_by": { "airline": {"terms": {"field": "airline"}}},
+              "aggs": {"airline.responsetime": {"avg": {"field": "responsetime"}}}
+            }
+          }