feat: adding logging to validation (#16527)

* adding logging to validation

* Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
(cherry picked from commit 376c685838)

superset-frontend/spec/javascripts/sqllab/actions/sqlLab_spec.js

@@ -613,7 +613,7 @@ describe('async actions', () => {
 
     describe('queryEditorSetSql', () => {
       describe('with backend persistence flag on', () => {
-        it('does not update the tab state in the backend', () => {
+        it('updates the tab state in the backend', () => {
           expect.assertions(2);
 
           const sql = 'SELECT * ';
@@ -629,7 +629,7 @@ describe('async actions', () => {
       });
     });
     describe('with backend persistence flag off', () => {
-      it('updates the tab state in the backend', () => {
+      it('does not update the tab state in the backend', () => {
         const backendPersistenceOffMock = jest
           .spyOn(featureFlags, 'isFeatureEnabled')
           .mockImplementation(

superset-frontend/src/SqlLab/actions/sqlLab.js

@@ -949,6 +949,11 @@ export function queryEditorSetQueryLimit(queryEditor, queryLimit) {
 
 export function queryEditorSetTemplateParams(queryEditor, templateParams) {
   return function (dispatch) {
+    dispatch({
+      type: QUERY_EDITOR_SET_TEMPLATE_PARAMS,
+      queryEditor,
+      templateParams,
+    });
     const sync = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
       ? SupersetClient.put({
           endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
@@ -956,24 +961,16 @@ export function queryEditorSetTemplateParams(queryEditor, templateParams) {
         })
       : Promise.resolve();
 
-    return sync
-      .then(() =>
-        dispatch({
-          type: QUERY_EDITOR_SET_TEMPLATE_PARAMS,
-          queryEditor,
-          templateParams,
-        }),
-      )
-      .catch(() =>
-        dispatch(
-          addDangerToast(
-            t(
-              'An error occurred while setting the tab template parameters. ' +
-                'Please contact your administrator.',
-            ),
+    return sync.catch(() =>
+      dispatch(
+        addDangerToast(
+          t(
+            'An error occurred while setting the tab template parameters. ' +
+              'Please contact your administrator.',
           ),
         ),
-      );
+      ),
+    );
   };
 }
 

superset-frontend/src/explore/components/DatasourcePanel/types.ts

@@ -25,3 +25,13 @@ export interface DatasourcePanelDndItem {
   value: DndItemValue;
   type: DndItemType;
 }
+
+export function isDatasourcePanelDndItem(
+  item: any,
+): item is DatasourcePanelDndItem {
+  return item?.value && item?.type;
+}
+
+export function isSavedMetric(item: any): item is Metric {
+  return item?.metric_name;
+}

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx

@@ -45,10 +45,12 @@ import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetr
 import {
   DatasourcePanelDndItem,
   DndItemValue,
+  isSavedMetric,
 } from 'src/explore/components/DatasourcePanel/types';
 import { DndItemType } from 'src/explore/components/DndItemType';
 import { ControlComponentProps } from 'src/explore/components/Control';
 
+const EMPTY_OBJECT = {};
 const DND_ACCEPTED_TYPES = [
   DndItemType.Column,
   DndItemType.Metric,
@@ -78,7 +80,9 @@ export const DndFilterSelect = (props: DndFilterSelectProps) => {
   );
   const [partitionColumn, setPartitionColumn] = useState(undefined);
   const [newFilterPopoverVisible, setNewFilterPopoverVisible] = useState(false);
-  const [droppedItem, setDroppedItem] = useState<DndItemValue | null>(null);
+  const [droppedItem, setDroppedItem] = useState<
+    DndItemValue | typeof EMPTY_OBJECT
+  >({});
 
   const optionsForSelect = (
     columns: ColumnMeta[],
@@ -342,12 +346,12 @@ export const DndFilterSelect = (props: DndFilterSelectProps) => {
   );
 
   const handleClickGhostButton = useCallback(() => {
-    setDroppedItem(null);
+    setDroppedItem({});
     togglePopover(true);
   }, [togglePopover]);
 
   const adhocFilter = useMemo(() => {
-    if (droppedItem?.metric_name) {
+    if (isSavedMetric(droppedItem)) {
       return new AdhocFilter({
         expressionType: EXPRESSION_TYPES.SQL,
         clause: CLAUSES.HAVING,

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx

@@ -34,7 +34,10 @@ import { usePrevious } from 'src/common/hooks/usePrevious';
 import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
 import AdhocMetricPopoverTrigger from 'src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger';
 import MetricDefinitionValue from 'src/explore/components/controls/MetricControl/MetricDefinitionValue';
-import { DatasourcePanelDndItem } from 'src/explore/components/DatasourcePanel/types';
+import {
+  DatasourcePanelDndItem,
+  isDatasourcePanelDndItem,
+} from 'src/explore/components/DatasourcePanel/types';
 import { DndItemType } from 'src/explore/components/DndItemType';
 import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
 import { savedMetricType } from 'src/explore/components/controls/MetricControl/types';
@@ -143,9 +146,9 @@ export const DndMetricSelect = (props: any) => {
   const [value, setValue] = useState<ValueType[]>(
     coerceAdhocMetrics(props.value),
   );
-  const [droppedItem, setDroppedItem] = useState<DatasourcePanelDndItem | null>(
-    null,
-  );
+  const [droppedItem, setDroppedItem] = useState<
+    DatasourcePanelDndItem | typeof EMPTY_OBJECT
+  >({});
   const [newMetricPopoverVisible, setNewMetricPopoverVisible] = useState(false);
   const prevColumns = usePrevious(columns);
   const prevSavedMetrics = usePrevious(savedMetrics);
@@ -323,13 +326,16 @@ export const DndMetricSelect = (props: any) => {
   );
 
   const handleClickGhostButton = useCallback(() => {
-    setDroppedItem(null);
+    setDroppedItem({});
     togglePopover(true);
   }, [togglePopover]);
 
   const adhocMetric = useMemo(() => {
-    if (droppedItem?.type === DndItemType.Column) {
-      const itemValue = droppedItem?.value as ColumnMeta;
+    if (
+      isDatasourcePanelDndItem(droppedItem) &&
+      droppedItem.type === DndItemType.Column
+    ) {
+      const itemValue = droppedItem.value as ColumnMeta;
       const config: Partial<AdhocMetric> = {
         column: { column_name: itemValue?.column_name },
       };

superset-frontend/src/setup/setupColors.ts

@@ -31,44 +31,43 @@ import {
   SequentialScheme,
 } from '@superset-ui/core';
 import superset from '@superset-ui/core/lib/color/colorSchemes/categorical/superset';
+import ColorSchemeRegistry from '@superset-ui/core/lib/color/ColorSchemeRegistry';
+
+function registerColorSchemes(
+  registry: ColorSchemeRegistry<unknown>,
+  colorSchemes: (CategoricalScheme | SequentialScheme)[],
+  standardDefaultKey: string,
+) {
+  colorSchemes.forEach(scheme => {
+    registry.registerValue(scheme.id, scheme);
+  });
+
+  const defaultKey =
+    colorSchemes.find(scheme => scheme.isDefault)?.id || standardDefaultKey;
+  registry.setDefaultKey(defaultKey);
+}
 
 export default function setupColors(
-  extraCategoricalColorSchemas: CategoricalScheme[] = [],
+  extraCategoricalColorSchemes: CategoricalScheme[] = [],
   extraSequentialColorSchemes: SequentialScheme[] = [],
 ) {
-  // Register color schemes
-  const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
-
-  if (extraCategoricalColorSchemas?.length > 0) {
-    extraCategoricalColorSchemas.forEach(scheme => {
-      categoricalSchemeRegistry.registerValue(scheme.id, scheme);
-    });
-  }
-
-  [superset, airbnb, categoricalD3, echarts, google, lyft, preset].forEach(
-    group => {
-      group.forEach(scheme => {
-        categoricalSchemeRegistry.registerValue(scheme.id, scheme);
-      });
-    },
+  registerColorSchemes(
+    getCategoricalSchemeRegistry(),
+    [
+      ...superset,
+      ...airbnb,
+      ...categoricalD3,
+      ...echarts,
+      ...google,
+      ...lyft,
+      ...preset,
+      ...extraCategoricalColorSchemes,
+    ],
+    'supersetColors',
+  );
+  registerColorSchemes(
+    getSequentialSchemeRegistry(),
+    [...sequentialCommon, ...sequentialD3, ...extraSequentialColorSchemes],
+    'superset_seq_1',
   );
-  categoricalSchemeRegistry.setDefaultKey('supersetColors');
-
-  const sequentialSchemeRegistry = getSequentialSchemeRegistry();
-
-  if (extraSequentialColorSchemes?.length > 0) {
-    extraSequentialColorSchemes.forEach(scheme => {
-      sequentialSchemeRegistry.registerValue(
-        scheme.id,
-        new SequentialScheme(scheme),
-      );
-    });
-  }
-
-  [sequentialCommon, sequentialD3].forEach(group => {
-    group.forEach(scheme => {
-      sequentialSchemeRegistry.registerValue(scheme.id, scheme);
-    });
-  });
-  sequentialSchemeRegistry.setDefaultKey('superset_seq_1');
 }

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -908,14 +908,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
         />
       );
     }
-
-    const message: Array<string> = Object.values(dbErrors);
+    const message: Array<string> =
+      typeof dbErrors === 'object' ? Object.values(dbErrors) : [];
     return (
       <Alert
         type="error"
         css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)}
         message="Database Creation Error"
-        description={message[0]}
+        description={message?.[0] || dbErrors}
       />
     );
   };

superset/config.py

@@ -427,7 +427,14 @@ FEATURE_FLAGS: Dict[str, bool] = {}
 #         feature_flags_dict['some_feature'] = g.user and g.user.get_id() == 5
 #     return feature_flags_dict
 GET_FEATURE_FLAGS_FUNC: Optional[Callable[[Dict[str, bool]], Dict[str, bool]]] = None
-
+# A function that receives a feature flag name and an optional default value.
+# Has a similar utility to GET_FEATURE_FLAGS_FUNC but it's useful to not force the
+# evaluation of all feature flags when just evaluating a single one.
+#
+# Note that the default `get_feature_flags` will evaluate each feature with this
+# callable when the config key is set, so don't use both GET_FEATURE_FLAGS_FUNC
+# and IS_FEATURE_ENABLED_FUNC in conjunction.
+IS_FEATURE_ENABLED_FUNC: Optional[Callable[[str, Optional[bool]], bool]] = None
 # A function that expands/overrides the frontend `bootstrap_data.common` object.
 # Can be used to implement custom frontend functionality,
 # or dynamically change certain configs.
@@ -449,6 +456,7 @@ COMMON_BOOTSTRAP_OVERRIDES_FUNC: Callable[
 #         "id": 'myVisualizationColors',
 #         "description": '',
 #         "label": 'My Visualization Colors',
+#         "isDefault": True,
 #         "colors":
 #          ['#006699', '#009DD9', '#5AAA46', '#44AAAA', '#DDAA77', '#7799BB', '#88AA77',
 #          '#552288', '#5AAA46', '#CC7788', '#EEDD55', '#9977BB', '#BBAA44', '#DDCCDD']
@@ -483,6 +491,7 @@ THEME_OVERRIDES: Dict[str, Any] = {}
 #         "description": '',
 #         "isDiverging": True,
 #         "label": 'My custom warm to hot',
+#         "isDefault": True,
 #         "colors":
 #          ['#552288', '#5AAA46', '#CC7788', '#EEDD55', '#9977BB', '#BBAA44', '#DDCCDD',
 #          '#006699', '#009DD9', '#5AAA46', '#44AAAA', '#DDAA77', '#7799BB', '#88AA77']

superset/databases/commands/test_connection.py

@@ -72,6 +72,10 @@ class TestConnectionDatabaseCommand(BaseCommand):
             database.db_engine_spec.mutate_db_for_connection_test(database)
             username = self._actor.username if self._actor is not None else None
             engine = database.get_sqla_engine(user_name=username)
+            event_logger.log_with_context(
+                action="test_connection_attempt",
+                engine=database.db_engine_spec.__name__,
+            )
             with closing(engine.raw_connection()) as conn:
                 try:
                     alive = engine.dialect.do_ping(conn)

superset/databases/commands/validate.py

@@ -33,6 +33,7 @@ from superset.databases.dao import DatabaseDAO
 from superset.db_engine_specs import get_engine_specs
 from superset.db_engine_specs.base import BasicParametersMixin
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.extensions import event_logger
 from superset.models.core import Database
 
 BYPASS_VALIDATION_ENGINES = {"bigquery"}
@@ -89,6 +90,7 @@ class ValidateDatabaseParametersCommand(BaseCommand):
             self._properties.get("parameters", {})
         )
         if errors:
+            event_logger.log_with_context(action="validation_error", engine=engine)
             raise InvalidParametersError(errors)
 
         serialized_encrypted_extra = self._properties.get("encrypted_extra", "{}")

superset/utils/feature_flag_manager.py

@@ -24,24 +24,36 @@ class FeatureFlagManager:
     def __init__(self) -> None:
         super().__init__()
         self._get_feature_flags_func = None
+        self._is_feature_enabled_func = None
         self._feature_flags: Dict[str, Any] = {}
 
     def init_app(self, app: Flask) -> None:
         self._get_feature_flags_func = app.config["GET_FEATURE_FLAGS_FUNC"]
+        self._is_feature_enabled_func = app.config["IS_FEATURE_ENABLED_FUNC"]
         self._feature_flags = app.config["DEFAULT_FEATURE_FLAGS"]
         self._feature_flags.update(app.config["FEATURE_FLAGS"])
 
     def get_feature_flags(self) -> Dict[str, Any]:
         if self._get_feature_flags_func:
             return self._get_feature_flags_func(deepcopy(self._feature_flags))
-
+        if callable(self._is_feature_enabled_func):
+            return dict(
+                map(
+                    lambda kv: (kv[0], self._is_feature_enabled_func(kv[0], kv[1])),
+                    self._feature_flags.items(),
+                )
+            )
         return self._feature_flags
 
     def is_feature_enabled(self, feature: str) -> bool:
         """Utility function for checking whether a feature is turned on"""
+        if self._is_feature_enabled_func:
+            return (
+                self._is_feature_enabled_func(feature, self._feature_flags[feature])
+                if feature in self._feature_flags
+                else False
+            )
         feature_flags = self.get_feature_flags()
-
         if feature_flags and feature in feature_flags:
             return feature_flags[feature]
-
         return False

tests/integration_tests/feature_flag_tests.py

@@ -16,10 +16,16 @@
 # under the License.
 from unittest.mock import patch
 
-from superset import is_feature_enabled
+from parameterized import parameterized
+
+from superset import get_feature_flags, is_feature_enabled
 from tests.integration_tests.base_tests import SupersetTestCase
 
 
+def dummy_is_feature_enabled(feature_flag_name: str, default: bool = True) -> bool:
+    return True if feature_flag_name.startswith("True_") else default
+
+
 class TestFeatureFlag(SupersetTestCase):
     @patch.dict(
         "superset.extensions.feature_flag_manager._feature_flags",
@@ -38,3 +44,40 @@ class TestFeatureFlag(SupersetTestCase):
     def test_feature_flags(self):
         self.assertEqual(is_feature_enabled("foo"), "bar")
         self.assertEqual(is_feature_enabled("super"), "set")
+
+
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    {"True_Flag1": False, "True_Flag2": True, "Flag3": False, "Flag4": True},
+    clear=True,
+)
+class TestFeatureFlagBackend(SupersetTestCase):
+    @parameterized.expand(
+        [
+            ("True_Flag1", True),
+            ("True_Flag2", True),
+            ("Flag3", False),
+            ("Flag4", True),
+            ("True_DoesNotExist", False),
+        ]
+    )
+    @patch(
+        "superset.extensions.feature_flag_manager._is_feature_enabled_func",
+        dummy_is_feature_enabled,
+    )
+    def test_feature_flags_override(self, feature_flag_name, expected):
+        self.assertEqual(is_feature_enabled(feature_flag_name), expected)
+
+    @patch(
+        "superset.extensions.feature_flag_manager._is_feature_enabled_func",
+        dummy_is_feature_enabled,
+    )
+    @patch(
+        "superset.extensions.feature_flag_manager._get_feature_flags_func", None,
+    )
+    def test_get_feature_flags(self):
+        feature_flags = get_feature_flags()
+        self.assertEqual(
+            feature_flags,
+            {"True_Flag1": True, "True_Flag2": True, "Flag3": False, "Flag4": True},
+        )