Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: invalid metric should raise an exception #20882

Merged
merged 8 commits into from
Jul 28, 2022
Merged

fix: invalid metric should raise an exception #20882

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
03c23ad
fix: invalid metric should raise an exception
zhaoyongjie Jul 27, 2022
786535c
remove unused raise
zhaoyongjie Jul 27, 2022
31be123
codes smell
zhaoyongjie Jul 27, 2022
b793f1c
fix ut
zhaoyongjie Jul 27, 2022
5e90682
handle None metric
zhaoyongjie Jul 27, 2022
d9033e3
fix deckgl fixture
zhaoyongjie Jul 27, 2022
f1cd5bd
lint
zhaoyongjie Jul 27, 2022
5572f3f
add unit test
zhaoyongjie Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions superset/dashboards/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,10 @@ def get_datasets(self, id_or_slug: str) -> Response:
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(f"Dataset schema is invalid, caused by: {str(err)}")
zhaoyongjie marked this conversation as resolved.
Show resolved Hide resolved
)
except DashboardAccessDeniedError:
return self.response_403()
except DashboardNotFoundError:
Expand Down
10 changes: 6 additions & 4 deletions superset/utils/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,18 +1294,20 @@ 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")
if column and aggregate:
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(
Expand Down
16 changes: 5 additions & 11 deletions tests/integration_tests/viz_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ def test_get_data_transforms_dataframe(self):
self.assertEqual(data, expected)

def test_get_data_empty_null_keys(self):
form_data = {"groupby": [], "metrics": ["", None]}
Copy link
Member Author

@zhaoyongjie zhaoyongjie Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metric must be a string or AdhocMetric.

form_data = {"groupby": [], "metrics": [""]}
datasource = self.get_datasource_mock()
# Test data
raw = {}
Expand All @@ -739,19 +739,13 @@ def test_get_data_empty_null_keys(self):
"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]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test case where metric is a dict, too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the more unit test at here. I added some invalid metric case as well.

with self.assertRaises(ValueError):
viz.viz_types["paired_ttest"](datasource, form_data)


class TestPartitionViz(SupersetTestCase):
@patch("superset.viz.BaseViz.query_obj")
Expand Down