mikebridge commented on code in PR #44413:
URL: https://github.com/apache/superset/pull/44413#discussion_r4048219321
##########
superset/models/dashboard.py:
##########
@@ -346,21 +347,37 @@ def data(self) -> dict[str, Any]:
def datasets_trimmed_for_slices(
self,
- ) -> list[tuple[BaseDatasource, dict[str, Any]]]:
- slices_by_datasource: dict[int, set[Slice]] = defaultdict(set)
+ ) -> list[tuple[BaseDatasource | SemanticView, dict[str, Any]]]:
+ """Return trimmed chart metadata, keeping datasource types distinct."""
+ slices_by_datasource: dict[tuple[str, int], set[Slice]] =
defaultdict(set)
for slc in self.slices:
- slices_by_datasource[slc.datasource_id].add(slc)
+ slices_by_datasource[(slc.datasource_type,
slc.datasource_id)].add(slc)
- result: list[tuple[BaseDatasource, dict[str, Any]]] = []
+ result: list[tuple[BaseDatasource | SemanticView, dict[str, Any]]] = []
for _, slices in slices_by_datasource.items():
- # Use the eagerly-loaded datasource from any slice in the group
- datasource = next(iter(slices)).datasource
+ # Resolve once per typed datasource, retaining eager-loaded tables.
+ datasource: Datasource | None =
next(iter(slices)).resolved_datasource
- if datasource:
+ if isinstance(datasource, (BaseDatasource, SemanticView)):
# Filter out unneeded fields from the datasource payload
- result.append((datasource,
datasource.data_for_slices(list(slices))))
+ try:
+ payload: dict[str, Any] = dict(
+ datasource.data_for_slices(list(slices))
+ )
+ except Exception: # noqa: BLE001
+ if not isinstance(datasource, SemanticView):
+ raise
+ # Provider discovery must not hide other charts' metadata.
+ # Exception details may contain credentials or provider
URLs.
+ logger.warning(
+ "Could not serialize semantic view id=%s
layer_uuid=%s",
+ datasource.id,
+ datasource.semantic_layer_uuid,
+ )
+ continue
Review Comment:
Following the owner decision, could we retain isolation here until providers
have a shared operational exception contract? 33ccf0a3fa keeps the catch and
adds a warning with persisted view id/layer UUID and exception class only,
never provider uid, message or traceback. Red-first coverage now verifies this
policy for ValueError, RuntimeError and AttributeError; table failures still
propagate. All 11 dataset tests pass.
##########
superset/models/dashboard.py:
##########
@@ -346,21 +347,37 @@ def data(self) -> dict[str, Any]:
def datasets_trimmed_for_slices(
self,
- ) -> list[tuple[BaseDatasource, dict[str, Any]]]:
- slices_by_datasource: dict[int, set[Slice]] = defaultdict(set)
+ ) -> list[tuple[BaseDatasource | SemanticView, dict[str, Any]]]:
+ """Return trimmed chart metadata, keeping datasource types distinct."""
+ slices_by_datasource: dict[tuple[str, int], set[Slice]] =
defaultdict(set)
for slc in self.slices:
- slices_by_datasource[slc.datasource_id].add(slc)
+ slices_by_datasource[(slc.datasource_type,
slc.datasource_id)].add(slc)
- result: list[tuple[BaseDatasource, dict[str, Any]]] = []
+ result: list[tuple[BaseDatasource | SemanticView, dict[str, Any]]] = []
for _, slices in slices_by_datasource.items():
- # Use the eagerly-loaded datasource from any slice in the group
- datasource = next(iter(slices)).datasource
+ # Resolve once per typed datasource, retaining eager-loaded tables.
+ datasource: Datasource | None =
next(iter(slices)).resolved_datasource
- if datasource:
+ if isinstance(datasource, (BaseDatasource, SemanticView)):
# Filter out unneeded fields from the datasource payload
- result.append((datasource,
datasource.data_for_slices(list(slices))))
+ try:
+ payload: dict[str, Any] = dict(
+ datasource.data_for_slices(list(slices))
+ )
+ except Exception: # noqa: BLE001
+ if not isinstance(datasource, SemanticView):
+ raise
Review Comment:
Following the owner decision, the broad catch remains intentional so one
broken semantic provider cannot hide other charts metadata. Could we revisit
narrowing when the provider exception contract exists? 33ccf0a3fa improves
diagnostics with persisted identity and exception class only, without message
or exc_info. Tests cover isolation and safe warning output, including
AttributeError; 11 dataset tests pass.
##########
superset/dashboards/api.py:
##########
@@ -756,6 +757,10 @@ def _serialize_dashboard_dataset(
) -> dict[str, Any]:
"""Dump a member dataset, narrowed when the caller cannot access it."""
serialized = self.dashboard_dataset_schema.dump(payload)
+ if isinstance(datasource, SemanticView):
+ # Redux keys dashboard datasets by Slice.form_data["datasource"],
+ # whereas SemanticView.uid is the provider's independent identity.
Review Comment:
Could we keep shared host-key helper extraction as a separate refactor, as
the owner decided? This patch retains the existing id__type boundary
normalization: SemanticView.uid is provider identity and cannot substitute for
the dashboard lookup key. The logging follow-up uses persisted id/layer UUID,
not provider uid, and records only the exception class so diagnostics do not
expose provider messages or traceback.
--
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]
