geido commented on code in PR #43719:
URL: https://github.com/apache/superset/pull/43719#discussion_r3915699163
##########
superset/datasets/schemas.py:
##########
@@ -518,6 +524,23 @@ def post_dump(
"""
Clear API response to avoid exposing sensitive information for
embedded users,
and filter columns to only include those with groupby=True for drill
operations.
+
+ ``metrics`` is deliberately not narrowed the same way. The two lists
answer
+ different questions: ``columns`` populates the drill-by dimension
picker, so
+ non-dimension columns are noise there, while ``metrics`` only ever
resolves
+ display labels (e.g. for the dashboard "View as table" results grid),
which
+ needs a label for every metric a chart may reference. Reusing the
dimension
+ filter on metrics would drop all of them, since a metric name is never
a
Review Comment:
You're right, and this is the better framing of the whole bug: it isn't
"metrics are missing", it's "this response is narrowed for the drill-by picker
while also being the source of the results grid's verbose map". A raw-records
table selecting non-dimension columns hits exactly the same raw headers. Fixed
rather than deferred.
`post_dump` no longer filters. `DatasetColumnDrillInfoSchema` carries
`groupby`, and the narrowing moves to the one consumer that needs it —
`ChartContextMenu`, which already did this on the extension path:
```ts
// before
(!loadDrillByOptionsExtension || column.groupby) && ...
// after
column.groupby && ...
```
That also drops the `loadDrillByOptionsExtension` special case, since both
paths now return every column and both need the same narrowing.
Two consequences worth flagging rather than burying:
- **The payload grows** — non-dimension columns are now included, for guests
too. That widens what @amaannawab923 raised above, so calling it out
explicitly: a guest reaching this via the dashboard fallback now gets every
column name and label, not only dimension ones. My read is unchanged — it's the
same class of dataset field metadata the branch already returned for
dimensions, and the guest sees these labels rendered in the dashboard's own
charts — but it is a real widening and I'd rather it be argued than slipped in.
- **It resolves the consistency point** @amaannawab923 made in the schema
thread: both lists are now scoped identically instead of one being filtered and
the other not.
Tests: `test_drill_info_returns_columns_and_metrics_unfiltered` (unit,
asserts `groupby` rides along on both a dimension and a non-dimension column),
the updated `test_get_drill_info_admin_user` (all four columns returned), and
`drill by only offers dimension columns` in `ChartContextMenu.test.tsx` so the
picker can't silently start offering non-dimensions.
--
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]
