bito-code-review[bot] commented on code in PR #44413:
URL: https://github.com/apache/superset/pull/44413#discussion_r4048221453
##########
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:
<!-- Bito Reply -->
The suggestion to narrow the exception type is appropriate for improving
error handling, as the current broad catch masks programming errors like
AttributeError or TypeError. However, as noted in the discussion, simply
narrowing the exception type without a shared failure contract between the
provider boundary and the calling code risks losing necessary isolation for
provider-specific failures. Establishing a shared exception family or a
specific provider-failure contract is a sound architectural step before
narrowing the catch block to ensure that genuine code bugs are still propagated
while expected provider discovery failures are handled gracefully.
**superset/models/dashboard.py**
```
try:
payload: dict[str, Any] = dict(
datasource.data_for_slices(list(slices))
)
except Exception: # noqa: BLE001
if not isinstance(datasource, SemanticView):
raise
```
##########
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:
<!-- Bito Reply -->
The decision to keep the shared host-key helper extraction as a separate
refactor is appropriate. Maintaining the existing boundary where
`SemanticView.uid` serves as the provider identity while centralizing the
`{id}__{type}` format logic in a shared utility is a sound approach that
preserves the necessary functional separation for dashboard store lookups.
--
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]
