From a779622982c676e269a633164870bde077a7a359 Mon Sep 17 00:00:00 2001
From: Marcus Andersson <marcus.andersson@grafana.com>
Date: Wed, 14 Oct 2020 12:55:28 +0200
Subject: [PATCH] Variables: prevent adhoc filters from crashing when they are
 not loaded properly (#28226)

* fixing so we will present a better error message when loading ad hoc filter variables.

* added tests to verify url parsing.

* added a test to make sure it works the oppisite way.
---
 .../src/components/Segment/SegmentAsync.tsx   | 38 +++++++-----
 .../variables/adhoc/urlParser.test.ts         | 60 +++++++++++++++++++
 .../app/features/variables/adhoc/urlParser.ts |  8 +--
 3 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/packages/grafana-ui/src/components/Segment/SegmentAsync.tsx b/packages/grafana-ui/src/components/Segment/SegmentAsync.tsx
index ef32da8a1cc..6e09ce1a990 100644
--- a/packages/grafana-ui/src/components/Segment/SegmentAsync.tsx
+++ b/packages/grafana-ui/src/components/Segment/SegmentAsync.tsx
@@ -1,9 +1,11 @@
-import React, { useState } from 'react';
+import React from 'react';
 import { cx } from 'emotion';
 import _ from 'lodash';
 import { SegmentSelect } from './SegmentSelect';
 import { SelectableValue } from '@grafana/data';
 import { useExpandableLabel, SegmentProps } from '.';
+import { useAsyncFn } from 'react-use';
+import { AsyncState } from 'react-use/lib/useAsync';
 
 export interface SegmentAsyncProps<T> extends SegmentProps<T> {
   value?: T | SelectableValue<T>;
@@ -20,20 +22,14 @@ export function SegmentAsync<T>({
   allowCustomValue,
   placeholder,
 }: React.PropsWithChildren<SegmentAsyncProps<T>>) {
-  const [selectPlaceholder, setSelectPlaceholder] = useState<string>('');
-  const [loadedOptions, setLoadedOptions] = useState<Array<SelectableValue<T>>>([]);
+  const [state, fetchOptions] = useAsyncFn(loadOptions, [loadOptions]);
   const [Label, width, expanded, setExpanded] = useExpandableLabel(false);
 
   if (!expanded) {
     const label = _.isObject(value) ? value.label : value;
     return (
       <Label
-        onClick={async () => {
-          setSelectPlaceholder('Loading options...');
-          const opts = await loadOptions();
-          setLoadedOptions(opts);
-          setSelectPlaceholder(opts.length ? '' : 'No options found');
-        }}
+        onClick={fetchOptions}
         Component={
           Component || (
             <a className={cx('gf-form-label', 'query-part', !value && placeholder && 'query-placeholder', className)}>
@@ -48,21 +44,33 @@ export function SegmentAsync<T>({
   return (
     <SegmentSelect
       value={value && !_.isObject(value) ? { value } : value}
-      options={loadedOptions}
+      options={state.value ?? []}
       width={width}
-      noOptionsMessage={selectPlaceholder}
+      noOptionsMessage={mapStateToNoOptionsMessage(state)}
       allowCustomValue={allowCustomValue}
       onClickOutside={() => {
-        setSelectPlaceholder('');
-        setLoadedOptions([]);
         setExpanded(false);
       }}
       onChange={item => {
-        setSelectPlaceholder('');
-        setLoadedOptions([]);
         setExpanded(false);
         onChange(item);
       }}
     />
   );
 }
+
+function mapStateToNoOptionsMessage<T>(state: AsyncState<Array<SelectableValue<T>>>): string {
+  if (state.loading) {
+    return 'Loading options...';
+  }
+
+  if (state.error) {
+    return 'Failed to load options';
+  }
+
+  if (!Array.isArray(state.value) || state.value.length === 0) {
+    return 'No options found';
+  }
+
+  return '';
+}
diff --git a/public/app/features/variables/adhoc/urlParser.test.ts b/public/app/features/variables/adhoc/urlParser.test.ts
index 228b07dc506..86fb993558e 100644
--- a/public/app/features/variables/adhoc/urlParser.test.ts
+++ b/public/app/features/variables/adhoc/urlParser.test.ts
@@ -42,6 +42,42 @@ describe('urlParser', () => {
     });
   });
 
+  describe('parsing toUrl with filters without values', () => {
+    it('then url params should be correct', () => {
+      const a: AdHocVariableFilter = {
+        value: '',
+        key: 'key',
+        operator: '',
+        condition: '',
+      };
+
+      const filters: AdHocVariableFilter[] = [a];
+
+      const expectedA = `key||`;
+      const expected: string[] = [expectedA];
+
+      expect(toUrl(filters)).toEqual(expected);
+    });
+  });
+
+  describe('parsing toUrl with filters with undefined values', () => {
+    it('then url params should be correct', () => {
+      const a = ({
+        value: undefined,
+        key: 'key',
+        operator: undefined,
+        condition: '',
+      } as unknown) as AdHocVariableFilter;
+
+      const filters: AdHocVariableFilter[] = [a];
+
+      const expectedA = `key||`;
+      const expected: string[] = [expectedA];
+
+      expect(toUrl(filters)).toEqual(expected);
+    });
+  });
+
   describe('parsing toFilters with url containing no filters as string', () => {
     it('then url params should be correct', () => {
       const url: UrlQueryValue = '';
@@ -89,6 +125,30 @@ describe('urlParser', () => {
       expect(toFilters(url)).toEqual(expected);
     });
   });
+
+  describe('parsing toFilters with url containing filter with empty values', () => {
+    it('then url params should be correct', () => {
+      const url: UrlQueryValue = 'key||';
+      const expected: AdHocVariableFilter[] = [
+        {
+          value: '',
+          key: 'key',
+          operator: '',
+          condition: '',
+        },
+      ];
+
+      expect(toFilters(url)).toEqual(expected);
+    });
+  });
+
+  describe('parsing toFilters with url containing no filters as string', () => {
+    it('then url params should be correct', () => {
+      const url: UrlQueryValue = '';
+      const expected: AdHocVariableFilter[] = [];
+      expect(toFilters(url)).toEqual(expected);
+    });
+  });
 });
 
 function createFilter(value: string, operator = '='): AdHocVariableFilter {
diff --git a/public/app/features/variables/adhoc/urlParser.ts b/public/app/features/variables/adhoc/urlParser.ts
index adbe40bf0dd..9a67aa3448b 100644
--- a/public/app/features/variables/adhoc/urlParser.ts
+++ b/public/app/features/variables/adhoc/urlParser.ts
@@ -20,12 +20,12 @@ export const toFilters = (value: UrlQueryValue): AdHocVariableFilter[] => {
   return filter === null ? [] : [filter];
 };
 
-function escapeDelimiter(value: string) {
-  return value.replace(/\|/g, '__gfp__');
+function escapeDelimiter(value: string | undefined): string {
+  return value?.replace(/\|/g, '__gfp__') ?? '';
 }
 
-function unescapeDelimiter(value: string) {
-  return value.replace(/__gfp__/g, '|');
+function unescapeDelimiter(value: string | undefined): string {
+  return value?.replace(/__gfp__/g, '|') ?? '';
 }
 
 function toArray(filter: AdHocVariableFilter): string[] {