fix(explore): Filters with custom SQL disappearing (#21114)

* fix(explore): Filters with custom SQL disappearing * Fix adhoc filter for query b disappearing * Improve test coverage
apache · Aug 17, 2022 · 55304b0 · 55304b0
1 parent 1517956
commit 55304b0
Show file tree

Hide file tree

Showing 4 changed files with 144 additions and 18 deletions.
diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Filter.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Filter.ts
@@ -72,6 +72,12 @@ export function isSimpleAdhocFilter(
   return filter.expressionType === 'SIMPLE';
 }
 
+export function isFreeFormAdhocFilter(
+  filter: AdhocFilter,
+): filter is FreeFormAdhocFilter {
+  return filter.expressionType === 'SQL';
+}
+
 export function isUnaryAdhocFilter(
   filter: SimpleAdhocFilter,
 ): filter is UnaryAdhocFilter {

diff --git a/superset-frontend/packages/superset-ui-core/test/query/types/Filter.test.ts b/superset-frontend/packages/superset-ui-core/test/query/types/Filter.test.ts
@@ -21,6 +21,7 @@ import {
   isUnaryAdhocFilter,
   isBinaryAdhocFilter,
   isSetAdhocFilter,
+  isFreeFormAdhocFilter,
 } from '@superset-ui/core';
 
 describe('Filter type guards', () => {
@@ -95,4 +96,26 @@ describe('Filter type guards', () => {
       ).toEqual(false);
     });
   });
+  describe('isFreeFormAdhocFilter', () => {
+    it('should return true when it is the correct type', () => {
+      expect(
+        isFreeFormAdhocFilter({
+          expressionType: 'SQL',
+          clause: 'WHERE',
+          sqlExpression: 'gender = "boy"',
+        }),
+      ).toEqual(true);
+    });
+    it('should return false otherwise', () => {
+      expect(
+        isFreeFormAdhocFilter({
+          expressionType: 'SIMPLE',
+          clause: 'WHERE',
+          subject: 'tea',
+          operator: '==',
+          comparator: 'matcha',
+        }),
+      ).toEqual(false);
+    });
+  });
 });
diff --git a/superset-frontend/src/explore/controlUtils/getFormDataFromDashboardContext.test.ts b/superset-frontend/src/explore/controlUtils/getFormDataFromDashboardContext.test.ts
@@ -30,6 +30,35 @@ const getExploreFormData = (overrides: JsonObject = {}) => ({
       comparator: ['boys'],
       filterOptionName: '123',
     },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "name = 'John'",
+      filterOptionName: '456',
+    },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "city = 'Warsaw'",
+      filterOptionName: '567',
+    },
+  ],
+  adhoc_filters_b: [
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "country = 'Poland'",
+      filterOptionName: '789',
+    },
   ],
   applied_time_extras: {},
   color_scheme: 'supersetColors',
@@ -111,6 +140,53 @@ const getExpectedResultFormData = (overrides: JsonObject = {}) => ({
       comparator: ['boys'],
       filterOptionName: '123',
     },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "name = 'John'",
+      filterOptionName: '456',
+    },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "city = 'Warsaw'",
+      filterOptionName: '567',
+    },
+    {
+      clause: 'WHERE',
+      expressionType: 'SIMPLE',
+      operator: 'IN',
+      subject: 'name',
+      comparator: ['Aaron'],
+      isExtra: true,
+      filterOptionName: expect.any(String),
+    },
+    {
+      clause: 'WHERE',
+      expressionType: 'SIMPLE',
+      operator: '<=',
+      subject: 'num_boys',
+      comparator: 10000,
+      isExtra: true,
+      filterOptionName: expect.any(String),
+    },
+  ],
+  adhoc_filters_b: [
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "country = 'Poland'",
+      filterOptionName: expect.any(String),
+    },
     {
       clause: 'WHERE',
       expressionType: 'SIMPLE',

diff --git a/superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts b/superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts
@@ -26,6 +26,9 @@ import {
   QueryObjectFilterClause,
   SimpleAdhocFilter,
   QueryFormData,
+  AdhocFilter,
+  isFreeFormAdhocFilter,
+  isSimpleAdhocFilter,
 } from '@superset-ui/core';
 import { NO_TIME_RANGE } from '../constants';
 
@@ -51,28 +54,34 @@ const simpleFilterToAdhoc = (
   return result;
 };
 
-const removeAdhocFilterDuplicates = (filters: SimpleAdhocFilter[]) => {
+const removeAdhocFilterDuplicates = (filters: AdhocFilter[]) => {
   const isDuplicate = (
-    adhocFilter: SimpleAdhocFilter,
-    existingFilters: SimpleAdhocFilter[],
+    adhocFilter: AdhocFilter,
+    existingFilters: AdhocFilter[],
   ) =>
     existingFilters.some(
-      (existingFilter: SimpleAdhocFilter) =>
-        existingFilter.operator === adhocFilter.operator &&
-        existingFilter.subject === adhocFilter.subject &&
-        ((!('comparator' in existingFilter) &&
-          !('comparator' in adhocFilter)) ||
-          ('comparator' in existingFilter &&
-            'comparator' in adhocFilter &&
-            isEqual(existingFilter.comparator, adhocFilter.comparator))),
+      (existingFilter: AdhocFilter) =>
+        (isFreeFormAdhocFilter(existingFilter) &&
+          isFreeFormAdhocFilter(adhocFilter) &&
+          existingFilter.clause === adhocFilter.clause &&
+          existingFilter.sqlExpression === adhocFilter.sqlExpression) ||
+        (isSimpleAdhocFilter(existingFilter) &&
+          isSimpleAdhocFilter(adhocFilter) &&
+          existingFilter.operator === adhocFilter.operator &&
+          existingFilter.subject === adhocFilter.subject &&
+          ((!('comparator' in existingFilter) &&
+            !('comparator' in adhocFilter)) ||
+            ('comparator' in existingFilter &&
+              'comparator' in adhocFilter &&
+              isEqual(existingFilter.comparator, adhocFilter.comparator)))),
     );
 
   return filters.reduce((acc, filter) => {
     if (!isDuplicate(filter, acc)) {
       acc.push(filter);
     }
     return acc;
-  }, [] as SimpleAdhocFilter[]);
+  }, [] as AdhocFilter[]);
 };
 
 const mergeFilterBoxToFormData = (
@@ -176,16 +185,28 @@ export const getFormDataWithDashboardContext = (
     exploreFormData,
     dashboardContextFormData,
   );
-  const adhocFilters = removeAdhocFilterDuplicates([
-    ...ensureIsArray(exploreFormData.adhoc_filters),
-    ...ensureIsArray(filterBoxData.adhoc_filters),
-    ...ensureIsArray(nativeFiltersData.adhoc_filters),
-  ]);
+  const adhocFilters = [
+    ...Object.keys(exploreFormData),
+    ...Object.keys(filterBoxData),
+    ...Object.keys(nativeFiltersData),
+  ]
+    .filter(key => key.match(/adhoc_filter.*/))
+    .reduce(
+      (acc, key) => ({
+        ...acc,
+        [key]: removeAdhocFilterDuplicates([
+          ...ensureIsArray(exploreFormData[key]),
+          ...ensureIsArray(filterBoxData[key]),
+          ...ensureIsArray(nativeFiltersData[key]),
+        ]),
+      }),
+      {},
+    );
   return {
     ...exploreFormData,
     ...dashboardContextFormData,
     ...filterBoxData,
     ...nativeFiltersData,
-    adhoc_filters: adhocFilters,
+    ...adhocFilters,
   };
 };