From 7f1327d1ed876037ca35dbf7ebaa6a5c2e9df1da Mon Sep 17 00:00:00 2001
From: Giordano Ricci <me@giordanoricci.com>
Date: Thu, 2 Sep 2021 15:00:40 +0100
Subject: [PATCH] Elasticsearch: Prevent pipeline aggregations to show up in
 terms order by options (#38448)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Elasticsearch: Prevent pipeline aggregations to show up in terms order by options

* fix optional props

* Refactor & add tests

* Fix & lower strict ts count

* Update public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx

Co-authored-by: Piotr Jamróz <pm.jamroz@gmail.com>

* Update public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx

Co-authored-by: Piotr Jamróz <pm.jamroz@gmail.com>

Co-authored-by: Piotr Jamróz <pm.jamroz@gmail.com>
---
 .../TermsSettingsEditor.test.tsx              | 41 ++++++++++
 .../SettingsEditor/TermsSettingsEditor.tsx    | 79 +++++++++++++++++-
 .../SettingsEditor/useDescription.ts          |  2 +-
 .../BucketAggregationsEditor/utils.ts         | 80 ++-----------------
 .../elasticsearch/test-helpers/render.tsx     | 22 +++--
 scripts/ci-check-strict.sh                    |  2 +-
 6 files changed, 141 insertions(+), 85 deletions(-)
 create mode 100644 public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx

diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx
new file mode 100644
index 00000000000..3243378b00a
--- /dev/null
+++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.test.tsx
@@ -0,0 +1,41 @@
+import { screen } from '@testing-library/react';
+import { ElasticsearchQuery } from '../../../../types';
+import React from 'react';
+import { renderWithESProvider } from '../../../../test-helpers/render';
+import { Average, Derivative, TopMetrics } from '../../MetricAggregationsEditor/aggregations';
+import { Terms } from '../aggregations';
+import { TermsSettingsEditor } from './TermsSettingsEditor';
+import selectEvent from 'react-select-event';
+import { describeMetric } from 'app/plugins/datasource/elasticsearch/utils';
+
+describe('Terms Settings Editor', () => {
+  it('Pipeline aggregations should not be in "order by" options', () => {
+    const termsAgg: Terms = {
+      id: '1',
+      type: 'terms',
+    };
+    const avg: Average = { id: '2', type: 'avg', field: '@value' };
+    const derivative: Derivative = { id: '3', field: avg.id, type: 'derivative' };
+    const topMetrics: TopMetrics = { id: '4', type: 'top_metrics' };
+    const query: ElasticsearchQuery = {
+      refId: 'A',
+      query: '',
+      bucketAggs: [termsAgg],
+      metrics: [avg, derivative, topMetrics],
+    };
+
+    renderWithESProvider(<TermsSettingsEditor bucketAgg={termsAgg} />, { providerProps: { query } });
+
+    const selectEl = screen.getByLabelText('Order By');
+    expect(selectEl).toBeInTheDocument();
+
+    selectEvent.openMenu(selectEl);
+
+    // Derivative is a pipeline aggregation, it shouldn't be present in the order by options
+    expect(screen.queryByText(describeMetric(derivative))).not.toBeInTheDocument();
+    // TopMetrics cannot be used as order by option
+    expect(screen.queryByText(describeMetric(topMetrics))).not.toBeInTheDocument();
+    // All other metric aggregations can be used in order by
+    expect(screen.getByText(describeMetric(avg))).toBeInTheDocument();
+  });
+});
diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.tsx b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.tsx
index dfc9b5fd1db..b13e020d20f 100644
--- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.tsx
+++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/TermsSettingsEditor.tsx
@@ -3,10 +3,20 @@ import { InlineField, Select, Input } from '@grafana/ui';
 import { Terms } from '../aggregations';
 import { useDispatch } from '../../../../hooks/useStatelessReducer';
 import { inlineFieldProps } from '.';
-import { bucketAggregationConfig, createOrderByOptionsFromMetrics, orderOptions, sizeOptions } from '../utils';
+import { bucketAggregationConfig, orderByOptions, orderOptions, sizeOptions } from '../utils';
 import { useCreatableSelectPersistedBehaviour } from '../../../hooks/useCreatableSelectPersistedBehaviour';
 import { changeBucketAggregationSetting } from '../state/actions';
 import { useQuery } from '../../ElasticsearchQueryContext';
+import { SelectableValue } from '@grafana/data';
+import { describeMetric } from '../../../../utils';
+import {
+  ExtendedStatMetaType,
+  ExtendedStats,
+  isPipelineAggregation,
+  MetricAggregation,
+  Percentiles,
+} from '../../MetricAggregationsEditor/aggregations';
+import { uniqueId } from 'lodash';
 
 interface Props {
   bucketAgg: Terms;
@@ -14,7 +24,7 @@ interface Props {
 
 export const TermsSettingsEditor = ({ bucketAgg }: Props) => {
   const { metrics } = useQuery();
-  const orderBy = createOrderByOptionsFromMetrics(metrics);
+  const orderBy = createOrderByOptions(metrics);
 
   const dispatch = useDispatch();
 
@@ -60,6 +70,7 @@ export const TermsSettingsEditor = ({ bucketAgg }: Props) => {
 
       <InlineField label="Order By" {...inlineFieldProps}>
         <Select
+          inputId={uniqueId('es-terms-')}
           menuShouldPortal
           onChange={(e) =>
             dispatch(changeBucketAggregationSetting({ bucketAgg, settingName: 'orderBy', newValue: e.value }))
@@ -80,3 +91,67 @@ export const TermsSettingsEditor = ({ bucketAgg }: Props) => {
     </>
   );
 };
+
+/**
+ * This returns the valid options for each of the enabled extended stat
+ */
+function createOrderByOptionsForExtendedStats(metric: ExtendedStats): SelectableValue<string> {
+  if (!metric.meta) {
+    return [];
+  }
+  const metaKeys = Object.keys(metric.meta) as ExtendedStatMetaType[];
+  return metaKeys
+    .filter((key) => metric.meta?.[key])
+    .map((key) => {
+      let method = key as string;
+      // The bucket path for std_deviation_bounds.lower and std_deviation_bounds.upper
+      // is accessed via std_lower and std_upper, respectively.
+      if (key === 'std_deviation_bounds_lower') {
+        method = 'std_lower';
+      }
+      if (key === 'std_deviation_bounds_upper') {
+        method = 'std_upper';
+      }
+      return { label: `${describeMetric(metric)} (${method})`, value: `${metric.id}[${method}]` };
+    });
+}
+
+/**
+ * This returns the valid options for each of the percents listed in the percentile settings
+ */
+function createOrderByOptionsForPercentiles(metric: Percentiles): Array<SelectableValue<string>> {
+  if (!metric.settings?.percents) {
+    return [];
+  }
+  return metric.settings.percents.map((percent) => {
+    // The bucket path for percentile numbers is appended with a `.0` if the number is whole
+    // otherwise you have to use the actual value.
+    const percentString = /^\d+\.\d+/.test(`${percent}`) ? percent : `${percent}.0`;
+    return { label: `${describeMetric(metric)} (${percent})`, value: `${metric.id}[${percentString}]` };
+  });
+}
+
+function isValidOrderTarget(metric: MetricAggregation) {
+  return (
+    // top metrics can't be used for ordering
+    metric.type !== 'top_metrics' &&
+    // pipeline aggregations can't be used for ordering: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html#search-aggregations-bucket-terms-aggregation-order
+    !isPipelineAggregation(metric)
+  );
+}
+
+/**
+ * This creates all the valid order by options based on the metrics
+ */
+export const createOrderByOptions = (metrics: MetricAggregation[] = []): Array<SelectableValue<string>> => {
+  const metricOptions = metrics.filter(isValidOrderTarget).flatMap((metric) => {
+    if (metric.type === 'extended_stats') {
+      return createOrderByOptionsForExtendedStats(metric);
+    } else if (metric.type === 'percentiles') {
+      return createOrderByOptionsForPercentiles(metric);
+    } else {
+      return { label: describeMetric(metric), value: metric.id };
+    }
+  });
+  return [...orderByOptions, ...metricOptions];
+};
diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/useDescription.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/useDescription.ts
index 18d2343e728..53b3defef77 100644
--- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/useDescription.ts
+++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/SettingsEditor/useDescription.ts
@@ -3,7 +3,7 @@ import { useQuery } from '../../ElasticsearchQueryContext';
 import { BucketAggregation } from '../aggregations';
 import { bucketAggregationConfig, orderByOptions, orderOptions } from '../utils';
 
-const hasValue = (value: string) => (object: { value: string }) => object.value === value;
+const hasValue = (value: string) => (object: { value?: string }) => object.value === value;
 
 // FIXME: We should apply the same defaults we have in bucketAggregationsConfig here instead of "custom" values
 // as they might get out of sync.
diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/utils.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/utils.ts
index d313e9dcc6a..02f0dfa0f68 100644
--- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/utils.ts
+++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/utils.ts
@@ -1,12 +1,5 @@
 import { BucketsConfiguration } from '../../../types';
 import { defaultFilter } from './SettingsEditor/FiltersSettingsEditor/utils';
-import { describeMetric } from '../../../utils';
-import {
-  ExtendedStatMetaType,
-  ExtendedStats,
-  MetricAggregation,
-  Percentiles,
-} from '../MetricAggregationsEditor/aggregations';
 import { SelectableValue } from '@grafana/data';
 
 export const bucketAggregationConfig: BucketsConfiguration = {
@@ -53,9 +46,12 @@ export const bucketAggregationConfig: BucketsConfiguration = {
   },
 };
 
-// TODO: Define better types for the following
-type OrderByOption = SelectableValue<string>;
-export const orderOptions: OrderByOption[] = [
+export const orderByOptions: Array<SelectableValue<string>> = [
+  { label: 'Term value', value: '_term' },
+  { label: 'Doc Count', value: '_count' },
+];
+
+export const orderOptions: Array<SelectableValue<string>> = [
   { label: 'Top', value: 'desc' },
   { label: 'Bottom', value: 'asc' },
 ];
@@ -70,67 +66,3 @@ export const sizeOptions = [
   { label: '15', value: '15' },
   { label: '20', value: '20' },
 ];
-
-export const orderByOptions = [
-  { label: 'Term value', value: '_term' },
-  { label: 'Doc Count', value: '_count' },
-];
-
-/**
- * This returns the valid options for each of the enabled extended stat
- */
-function createOrderByOptionsForExtendedStats(metric: ExtendedStats): SelectableValue<string> {
-  if (!metric.meta) {
-    return [];
-  }
-  const metaKeys = Object.keys(metric.meta) as ExtendedStatMetaType[];
-  return metaKeys
-    .filter((key) => metric.meta?.[key])
-    .map((key) => {
-      let method = key as string;
-      // The bucket path for std_deviation_bounds.lower and std_deviation_bounds.upper
-      // is accessed via std_lower and std_upper, respectively.
-      if (key === 'std_deviation_bounds_lower') {
-        method = 'std_lower';
-      }
-      if (key === 'std_deviation_bounds_upper') {
-        method = 'std_upper';
-      }
-      return { label: `${describeMetric(metric)} (${method})`, value: `${metric.id}[${method}]` };
-    });
-}
-
-/**
- * This returns the valid options for each of the percents listed in the percentile settings
- */
-function createOrderByOptionsForPercentiles(metric: Percentiles): OrderByOption[] {
-  if (!metric.settings?.percents) {
-    return [];
-  }
-  return metric.settings.percents.map((percent) => {
-    // The bucket path for percentile numbers is appended with a `.0` if the number is whole
-    // otherwise you have to use the actual value.
-    const percentString = /^\d+\.\d+/.test(`${percent}`) ? percent : `${percent}.0`;
-    return { label: `${describeMetric(metric)} (${percent})`, value: `${metric.id}[${percentString}]` };
-  });
-}
-
-const INCOMPATIBLE_ORDER_BY_AGGS = ['top_metrics'];
-
-/**
- * This creates all the valid order by options based on the metrics
- */
-export const createOrderByOptionsFromMetrics = (metrics: MetricAggregation[] = []): OrderByOption[] => {
-  const metricOptions = metrics
-    .filter((metric) => !INCOMPATIBLE_ORDER_BY_AGGS.includes(metric.type))
-    .flatMap((metric) => {
-      if (metric.type === 'extended_stats') {
-        return createOrderByOptionsForExtendedStats(metric);
-      } else if (metric.type === 'percentiles') {
-        return createOrderByOptionsForPercentiles(metric);
-      } else {
-        return { label: describeMetric(metric), value: metric.id };
-      }
-    });
-  return [...orderByOptions, ...metricOptions];
-};
diff --git a/public/app/plugins/datasource/elasticsearch/test-helpers/render.tsx b/public/app/plugins/datasource/elasticsearch/test-helpers/render.tsx
index d022bc69e6e..1b5e68db9f8 100644
--- a/public/app/plugins/datasource/elasticsearch/test-helpers/render.tsx
+++ b/public/app/plugins/datasource/elasticsearch/test-helpers/render.tsx
@@ -4,20 +4,28 @@ import { getDefaultTimeRange } from '@grafana/data';
 import { ElasticDatasource } from '../datasource';
 import { ElasticsearchProvider } from '../components/QueryEditor/ElasticsearchQueryContext';
 
+const defaultProviderProps = {
+  datasource: {} as ElasticDatasource,
+  query: { refId: 'A' },
+  onChange: () => void 0,
+  onRunQuery: () => void 0,
+  range: getDefaultTimeRange(),
+};
+
 export const renderWithESProvider = (
   ui: ReactNode,
   {
     providerProps: {
-      datasource = {} as ElasticDatasource,
-      query = { refId: 'A' },
-      onChange = () => void 0,
-      onRunQuery = () => void 0,
-      range = getDefaultTimeRange(),
-    } = {},
+      datasource = defaultProviderProps.datasource,
+      query = defaultProviderProps.query,
+      onChange = defaultProviderProps.onChange,
+      onRunQuery = defaultProviderProps.onRunQuery,
+      range = defaultProviderProps.range,
+    } = defaultProviderProps,
     ...renderOptions
   }: { providerProps?: Partial<Omit<ComponentProps<typeof ElasticsearchProvider>, 'children'>> } & Parameters<
     typeof render
-  >[1]
+  >[1] = { providerProps: defaultProviderProps }
 ) => {
   return render(
     <ElasticsearchProvider
diff --git a/scripts/ci-check-strict.sh b/scripts/ci-check-strict.sh
index 01c25971960..a17872feb7b 100755
--- a/scripts/ci-check-strict.sh
+++ b/scripts/ci-check-strict.sh
@@ -3,7 +3,7 @@ set -e
 
 echo -e "Collecting code stats (typescript errors & more)"
 
-ERROR_COUNT_LIMIT=50
+ERROR_COUNT_LIMIT=49
 ERROR_COUNT="$(./node_modules/.bin/tsc --project tsconfig.json --noEmit --strict true | grep -oP 'Found \K(\d+)')"
 
 if [ "$ERROR_COUNT" -gt $ERROR_COUNT_LIMIT ]; then