fix: invalid metric should raise an exception (#20882)

superset/dashboards/api.py

@@ -383,8 +383,12 @@ class DashboardRestApi(BaseSupersetModelRestApi):
                 self.dashboard_dataset_schema.dump(dataset) for dataset in datasets
             ]
             return self.response(200, result=result)
-        except TypeError:
-            return self.response_400(message=gettext("Dataset schema is invalid."))
+        except (TypeError, ValueError) as err:
+            return self.response_400(
+                message=gettext(
+                    "Dataset schema is invalid, caused by: %(error)s", error=str(err)
+                )
+            )
         except DashboardAccessDeniedError:
             return self.response_403()
         except DashboardNotFoundError:

superset/models/sql_lab.py

@@ -61,7 +61,9 @@ if TYPE_CHECKING:
 logger = logging.getLogger(__name__)
 
 
-class Query(Model, ExtraJSONMixin, ExploreMixin):  # pylint: disable=abstract-method
+class Query(
+    Model, ExtraJSONMixin, ExploreMixin
+):  # pylint: disable=abstract-method,too-many-public-methods
     """ORM model for SQL query
 
     Now that SQL Lab support multi-statement execution, an entry in this

superset/utils/core.py

@@ -1294,7 +1294,7 @@ def get_metric_name(
             sql_expression = metric.get("sqlExpression")
             if sql_expression:
                 return sql_expression
-        elif expression_type == "SIMPLE":
+        if expression_type == "SIMPLE":
             column: AdhocMetricColumn = metric.get("column") or {}
             column_name = column.get("column_name")
             aggregate = metric.get("aggregate")
@@ -1302,10 +1302,12 @@ def get_metric_name(
                 return f"{aggregate}({column_name})"
             if column_name:
                 return column_name
-        raise ValueError(__("Invalid metric object"))
 
-    verbose_map = verbose_map or {}
-    return verbose_map.get(metric, metric)  # type: ignore
+    if isinstance(metric, str):
+        verbose_map = verbose_map or {}
+        return verbose_map.get(metric, metric)
+
+    raise ValueError(__("Invalid metric object: %(metric)s", metric=str(metric)))
 
 
 def get_column_names(

tests/integration_tests/fixtures/deck_geojson_form_data.json

@@ -43,5 +43,5 @@
   "granularity_sqla": null,
   "autozoom": true,
   "url_params": {},
-  "size": 100
+  "size": "100"
 }

tests/integration_tests/fixtures/deck_path_form_data.json

@@ -45,5 +45,5 @@
   "granularity_sqla": null,
   "autozoom": true,
   "url_params": {},
-  "size": 100
+  "size": "100"
 }

tests/integration_tests/viz_tests.py

@@ -716,7 +716,7 @@ class TestPairedTTest(SupersetTestCase):
         self.assertEqual(data, expected)
 
     def test_get_data_empty_null_keys(self):
-        form_data = {"groupby": [], "metrics": ["", None]}
+        form_data = {"groupby": [], "metrics": [""]}
         datasource = self.get_datasource_mock()
         # Test data
         raw = {}
@@ -739,19 +739,13 @@ class TestPairedTTest(SupersetTestCase):
                     "group": "All",
                 }
             ],
-            "NULL": [
-                {
-                    "values": [
-                        {"x": 100, "y": 10},
-                        {"x": 200, "y": 20},
-                        {"x": 300, "y": 30},
-                    ],
-                    "group": "All",
-                }
-            ],
         }
         self.assertEqual(data, expected)
 
+        form_data = {"groupby": [], "metrics": [None]}
+        with self.assertRaises(ValueError):
+            viz.viz_types["paired_ttest"](datasource, form_data)
+
 
 class TestPartitionViz(SupersetTestCase):
     @patch("superset.viz.BaseViz.query_obj")

tests/unit_tests/core_tests.py

@@ -105,6 +105,18 @@ def test_get_metric_name_invalid_metric():
     with pytest.raises(ValueError):
         get_metric_name(metric)
 
+    metric = deepcopy(SQL_ADHOC_METRIC)
+    del metric["expressionType"]
+    with pytest.raises(ValueError):
+        get_metric_name(metric)
+
+    with pytest.raises(ValueError):
+        get_metric_name(None)
+    with pytest.raises(ValueError):
+        get_metric_name(0)
+    with pytest.raises(ValueError):
+        get_metric_name({})
+
 
 def test_get_metric_names():
     assert get_metric_names(