geido commented on code in PR #43719:
URL: https://github.com/apache/superset/pull/43719#discussion_r3915014022
##########
superset/datasets/schemas.py:
##########
@@ -516,5 +525,9 @@ def post_dump(
]
if security_manager.is_guest_user():
- return {"id": serialized["id"], "columns": serialized["columns"]}
+ return {
+ "id": serialized["id"],
+ "columns": serialized["columns"],
+ "metrics": serialized.get("metrics", []),
Review Comment:
Rebased onto master, so #43390's hardening (`DrillInfoEditorSchema`,
`UserSchema` dropping `email`) is now on the branch and the new comment sits
next to that reasoning.
I kept `metrics` in the guest branch, and moved the reasoning from a
one-line comment into the `post_dump` docstring plus a test, so it is
reviewable rather than implied:
- Guests only reach this through the dashboard fallback, which first
verifies `can_drill_dataset_via_dashboard_access` against a dashboard built on
this dataset.
- That branch already returns dataset-level dimension metadata — every
`groupby=True` column's name *and* its analyst-authored `verbose_name` —
regardless of whether the dashboard actually uses those columns. Metric labels
are the same class of information, so withholding them does not narrow what a
guest can learn about the dataset; it only leaves embedded viewers with
`can_view_chart_as_table` looking at raw metric names, which is the bug this PR
exists to fix.
- I read #43390 as a different class: it removed *user PII* (`email`, and
`secondary_label` which sync stores an email into), which dataset-read access
never entitled you to. A metric's display label is dataset field metadata, not
another principal's data.
On scoping to metrics present in the query result: `drill_info` has no
access to the query result, so this would mean passing the chart's metric list
into the request. That keys the response per chart instead of per dataset,
which multiplies requests per dashboard load and pulls directly against your
payload-size point in the other thread. I'd rather keep one dataset-level
response that `cachedSupersetGet` can dedupe.
Pinned in
`tests/unit_tests/datasets/schema_tests.py::test_drill_info_guest_payload_keeps_metric_labels`,
which also asserts the branch stays minimal in every other respect
(`set(dumped) == {"id", "columns", "metrics"}`).
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -3399,6 +3410,13 @@ def test_get_drill_info_admin_user(self):
{"column_name": "category", "verbose_name": "Category Column"},
{"column_name": "region", "verbose_name": None},
]
+ # Metrics must also carry their verbose_name so that consumers (e.g.
+ # the dashboard "View as table" results grid) can resolve a metric's
+ # friendly Label instead of falling back to its technical name.
+ assert result["metrics"] == [
Review Comment:
Fixed — the intent is now pinned by a fixture rather than by prose.
The dataset in this test already has a `value` column with `groupby=False`,
which the column filter drops. I added a metric *also* named `value`, so it is
exactly the case you described: under column-style filtering (`name in
dimensions`) it would be excluded, and it is asserted as present.
```python
assert result["metrics"] == [
{"metric_name": "sum__value", "verbose_name": "Yearly Total"},
{"metric_name": "value", "verbose_name": "Raw Value Metric"},
{"metric_name": "count", "verbose_name": None},
]
```
A later change that makes the two lists consistent now fails this assertion
instead of just contradicting a comment.
Also added
`tests/unit_tests/datasets/schema_tests.py::test_drill_info_metrics_are_not_dimension_filtered`,
which pins the same invariant directly on `DatasetDrillInfoSchema.post_dump`
without needing a DB — same shaped fixture, asserting `columns` is narrowed to
the dimension while `metrics` is not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
