From 522ac08e503303cb867d468fcbae48b78ee607c5 Mon Sep 17 00:00:00 2001
From: David Kyle <david.kyle@elastic.co>
Date: Thu, 23 May 2019 13:00:16 +0100
Subject: [PATCH] Catch AggregationExtractionException and throw a status
 exception with bad_request
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

An alternative would be to make AggregationExtractionException extent ElasticsearchStatusException but I’m not sure where it is thrown from
---
 ...nsportPreviewDataFrameTransformAction.java | 24 +++++++----
 .../test/data_frame/preview_transforms.yml    | 43 +++++++++++++++++++
 2 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java b/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java
index f4b93cc6ac412..dde9edb37e55c 100644
--- a/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java
+++ b/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java
@@ -31,6 +31,7 @@
 import org.elasticsearch.xpack.core.dataframe.transforms.DataFrameIndexerTransformStats;
 import org.elasticsearch.xpack.core.dataframe.transforms.DataFrameTransformConfig;
 import org.elasticsearch.xpack.core.dataframe.transforms.SourceConfig;
+import org.elasticsearch.xpack.dataframe.transforms.pivot.AggregationResultUtils;
 import org.elasticsearch.xpack.dataframe.transforms.pivot.Pivot;
 
 import java.util.List;
@@ -102,14 +103,21 @@ private void getPreview(Pivot pivot, SourceConfig source, ActionListener<List<Ma
                     pivot.buildSearchRequest(source, null, NUMBER_OF_PREVIEW_BUCKETS),
                     ActionListener.wrap(
                         r -> {
-                            final CompositeAggregation agg = r.getAggregations().get(COMPOSITE_AGGREGATION_NAME);
-                            DataFrameIndexerTransformStats stats = DataFrameIndexerTransformStats.withDefaultTransformId();
-                            // remove all internal fields
-                            List<Map<String, Object>> results = pivot.extractResults(agg, deducedMappings, stats)
-                                    .peek(record -> {
-                                        record.keySet().removeIf(k -> k.startsWith("_"));
-                                    }).collect(Collectors.toList());
-                            listener.onResponse(results);
+
+                            try {
+                                final CompositeAggregation agg = r.getAggregations().get(COMPOSITE_AGGREGATION_NAME);
+                                DataFrameIndexerTransformStats stats = DataFrameIndexerTransformStats.withDefaultTransformId();
+                                // remove all internal fields
+                                List<Map<String, Object>> results = pivot.extractResults(agg, deducedMappings, stats)
+                                        .peek(record -> {
+                                            record.keySet().removeIf(k -> k.startsWith("_"));
+                                        }).collect(Collectors.toList());
+
+                                listener.onResponse(results);
+                            } catch (AggregationResultUtils.AggregationExtractionException extractionException) {
+                                listener.onFailure(
+                                        new ElasticsearchStatusException(extractionException.getMessage(), RestStatus.BAD_REQUEST));
+                            }
                         },
                         listener::onFailure
                     ));
diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/preview_transforms.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/preview_transforms.yml
index 1d4a190b24e14..e4845eafe7d7f 100644
--- a/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/preview_transforms.yml
+++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/preview_transforms.yml
@@ -132,3 +132,46 @@ setup:
               "aggs": {"avg_response": {"avg": {"field": "responsetime"}}}
             }
           }
+
+---
+"Test preview returns bad request with invalid agg":
+  - skip:
+      reason:  date histo interval is deprecated
+      features: "warnings"
+
+  - do:
+      warnings:
+        - "[interval] on [date_histogram] is deprecated, use [fixed_interval] or [calendar_interval] in the future."
+      catch: bad_request
+      data_frame.preview_data_frame_transform:
+        body: >
+          {
+            "source": { "index": "airline-data" },
+            "pivot": {
+              "group_by": {
+                "time": {"date_histogram": {"interval": "1h", "field": "time", "format": "yyyy-MM-DD HH"}}},
+              "aggs": {
+                "avg_response": {"avg": {"field": "responsetime"}},
+                "time.min": {"min": {"field": "time"}}
+              }
+            }
+          }
+
+  - do:
+      warnings:
+        - "[interval] on [date_histogram] is deprecated, use [fixed_interval] or [calendar_interval] in the future."
+      catch: /mixed object types of nested and non-nested fields \[time.min\]/
+      data_frame.preview_data_frame_transform:
+        body: >
+          {
+            "source": { "index": "airline-data" },
+            "pivot": {
+              "group_by": {
+                "time": {"date_histogram": {"interval": "1h", "field": "time", "format": "yyyy-MM-DD HH"}}},
+              "aggs": {
+                "avg_response": {"avg": {"field": "responsetime"}},
+                "time.min": {"min": {"field": "time"}}
+              }
+            }
+          }
+