Copilot commented on code in PR #42534:
URL: https://github.com/apache/superset/pull/42534#discussion_r3667941612
##########
superset-frontend/src/explore/components/DataTablesPane/components/SamplesPane.tsx:
##########
@@ -154,7 +155,14 @@ export const SamplesPane = ({
rowLimitOptions={ROW_LIMIT_OPTIONS}
onRowLimitChange={handleRowLimitChange}
/>
- <Error>{responseError}</Error>
+ <ErrorAlertWrapper>
+ <Alert
+ type="error"
+ showIcon
+ message={t('Failed to load samples')}
+ description={responseError}
+ />
+ </ErrorAlertWrapper>
Review Comment:
Switching from a `<pre>` to `Alert` likely removes whitespace/newline
preservation for multi-line errors (stack traces, structured backend errors),
making them harder to read/copy. Consider wrapping `responseError` in a
preformatted container (e.g., a `<pre>`/`<code>` with `white-space: pre-wrap`
and zero margin) while still using `Alert` for the headline.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -199,7 +200,14 @@ export const useResultsPane = ({
isLoading={false}
canDownload={canDownload}
/>
- <Error>{responseError}</Error>
+ <ErrorAlertWrapper>
+ <Alert
+ type="error"
+ showIcon
+ message={t('Failed to load results')}
+ description={responseError}
+ />
+ </ErrorAlertWrapper>
Review Comment:
Same issue as SamplesPane: rendering `responseError` as a plain string in an
`Alert` description can collapse formatting for multi-line errors. To keep
error output readable and copyable, render the description in a preformatted
block (while retaining `Alert` for consistent UX and screen-reader semantics).
##########
superset/views/datasource/views.py:
##########
@@ -202,6 +202,23 @@ def samples(self) -> FlaskResponse:
payload = SamplesPayloadSchema().load(request.json)
except ValidationError as err:
return json_error_response(err.messages, status=400)
+
+ # Refuse early for datasource types that don't model raw rows
+ # (e.g. semantic views, which only expose pre-defined metrics and
+ # dimensions). Without this gate the request would still go through
+ # the standard query pipeline and fail with an opaque 500.
+ # ``supports_samples`` defaults to True for any datasource class that
+ # doesn't explicitly opt out, so SqlaTable/Query/SavedQuery continue
+ # to work without needing the attribute declared on each class.
+ ds_class: type[DatasourceUnion] | None = DatasourceDAO.sources.get(
+ DatasourceType(params["datasource_type"]),
+ )
+ if ds_class is not None and not getattr(ds_class, "supports_samples",
True):
+ return json_error_response(
+ _("Samples are not available for this datasource type."),
+ status=400,
+ )
+
dashboard_id = None
if security_manager.is_guest_user():
if not params["dashboard_id"]:
Review Comment:
This new early-return happens before the guest-user branch
(`security_manager.is_guest_user()`), which changes behavior for guest requests
(they’ll now get a 400 capability error instead of going through the
guest/dashboard validation logic). To avoid bypassing later
access-control/guest validation, move the capability gate to after the
guest/dashboard checks (and any datasource access checks that occur later in
this view), while still keeping it before `get_samples` is called.
--
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]
