EnxDev commented on PR #43376:
URL: https://github.com/apache/superset/pull/43376#issuecomment-5507720115
## EnxDev's Review Agent โ apache/superset#43376 ยท HEAD f4d63bc
comment โ solid, backward-compatible feature; one permission-scope question
worth a maintainer call, and a CI coverage gap to close before merge.
### ๐ก Should-fix
-
**`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:573-595`**
โ The "Query inspector" menu (and its new **Stats** tab) is still gated by
`canExplore || canViewQuery`. Before this PR, a principal with only
`can_view_query` (Dashboard) saw generated SQL only. Now that same principal
also sees query count, returned-row count, cache-hit count, and response
size/duration via Stats โ none of that was previously exposed to a
query-view-only role. It's aggregate metadata, not raw row data, so it's not a
clear SECURITY.md violation, but it is new information for that principal and
worth an explicit maintainer sign-off rather than an implicit side effect of
the menu rename. **regression test:** an RTL test asserting the Stats tab is
hidden (or shown, if intentional) for a `can_view_query`-only role, mirroring
the existing `canExplore`/`canViewTable` menu-visibility tests in
`SliceHeaderControls.test.tsx`.
- **`superset-frontend/src/explore/components/controls/ViewQueryModal.tsx`**
โ Codecov reports 97.56% patch coverage with 1 line in this file uncovered by
the new tests. Worth identifying and closing before merge (likely a
duration/branch edge case not exercised by the three new tests).
### ๐ต Nits
- `superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:96`
โ `getResponseStats` casts each `QueryData` entry `as JsonObject` to read
`.data`/`.is_cached`, since `QueryData = LegacyQueryData |
ChartDataResponseResult`. Works, but a type guard (`'is_cached' in response`)
would avoid the cast.
### ๐ Praise
- `superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:597`
(SliceHeaderControls) โ `showResponse={canExplore || canViewTable}` correctly
reuses existing permission state to keep raw cached-response JSON out of reach
for query-view-only users, matching the PR description's stated intent.
-
`superset-frontend/src/explore/components/controls/ViewQueryModal.test.tsx` โ
new tests assert real computed values (row counts, cache counts, byte sizes,
duration math) rather than smoke-testing, and cover the null/empty/undefined
edge cases.
<!-- enxdev-review-agent:f4d63bc -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD f4d63bc._
--
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]
