mikebridge commented on code in PR #44413:
URL: https://github.com/apache/superset/pull/44413#discussion_r4048155859
##########
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:
Good catch — I reproduced this with an AttributeError regression: the
current head omits the dataset instead of propagating the programming error.
Could we establish the operational provider exception contract before narrowing
the catch? The core discovery ABCs currently declare no shared provider
exception family, so substituting the semantic CRUD command exceptions would
not cover discovery failures. I have flagged that boundary for the coordinator;
no fix is pushed yet.
##########
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:
Good catch — an AttributeError propagation regression fails against the
current head. Could we distinguish operational discovery failures at the
provider boundary first? The core get_dimensions/get_metrics contract has no
shared failure type today. That is the remaining implementation decision;
narrowing to an invented exception list would risk losing the required
provider-failure isolation. No fix is pushed yet.
##########
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:
Agreed that a shared host-key helper would avoid repeating the format. Could
we keep provider identity separate when extracting it? SemanticView.uid
delegates to implementation.uid(), whereas the dashboard store needs id__type;
the existing regression deliberately uses a different provider uid. Replacing
this with datasource.uid would reintroduce the lookup failure. I have recorded
shared-helper extraction as the safe direction; no change is pushed yet.
--
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]
