sadpandajoe commented on code in PR #44851:
URL: https://github.com/apache/superset/pull/44851#discussion_r4212452154
##########
superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx:
##########
@@ -188,15 +218,102 @@ export default function SemanticViewEditModal({
});
addSuccessToast?.(t('Semantic view updated'));
onSave();
Review Comment:
A slow Save can overwrite a newer sync here. Closing the modal resets
`busy`, so the editor can be reopened and Sync metadata completed while the PUT
is still pending. When the PUT then resolves, this unguarded `onSave()` runs
the `handleDatasourceSave(datasource)` captured at click time and restores the
pre-sync datasource and controls in Explore. Should `onSave()` be skipped (or
re-read the current datasource) when `isCurrent()` is false?
##########
superset/common/query_context_processor.py:
##########
@@ -428,28 +448,100 @@ def query_cache_key(self, query_obj: QueryObject,
**kwargs: Any) -> str | None:
"""
Returns a QueryObject cache key for objects in self.queries
"""
+ return self._query_cache_key(query_obj, **kwargs)[0]
+
+ def _query_cache_key(
+ self,
+ query_obj: QueryObject,
+ *,
+ parent_cache_context: dict[str, Any] | None = None,
+ **kwargs: Any,
+ ) -> tuple[str | None, bool]:
+ """Keep annotation cacheability alongside the opaque hashed result
key."""
datasource: Explorable = self._qc_datasource
- # Reject unenforceable restrictions before provider identity or cache
reads.
- rls: list[str] = security_manager.get_rls_cache_key(datasource)
- extra_cache_keys = datasource.get_extra_cache_keys(query_obj.to_dict())
+ if parent_cache_context is None:
+ parent_cache_context = {}
+ if not parent_cache_context:
+ # Capture parent/Jinja inputs once; annotation discovery may re-key
+ # the result after execution but must not evaluate new parent
inputs.
+ rls: list[str] = security_manager.get_rls_cache_key(datasource)
+ parent_cache_context.update(
+ datasource=datasource.uid,
+
extra_cache_keys=datasource.get_extra_cache_keys(query_obj.to_dict()),
+ rls=rls,
+ changed_on=datasource.changed_on,
+ )
+ cacheable: bool = True
# Annotation data is cached on the same entry as the dataframe, so the
# key must also bind the annotation sources' security context.
if query_obj and query_obj.annotation_layers:
- kwargs["annotation_context"] =
self._annotation_cache_context(query_obj)
+ annotation_context: dict[str, Any] =
self._annotation_cache_context(
+ query_obj
+ )
+ kwargs["annotation_context"] = annotation_context
+ source_metadata: dict[str, str] = annotation_context.get(
+ "source_metadata", {}
+ )
+ cacheable = not any(
+ token.startswith("uncaptured:") for token in
source_metadata.values()
+ )
- cache_key = (
+ cache_key: str | None = (
query_obj.cache_key(
- datasource=datasource.uid,
- extra_cache_keys=extra_cache_keys,
- rls=rls,
- changed_on=datasource.changed_on,
+ **parent_cache_context,
**kwargs,
)
if query_obj
else None
)
- return cache_key
+ if cache_key is not None:
+ capture_result_identity(self._query_context, query_obj, cache_key)
+ return cache_key, cacheable
+
+ def _capture_annotation_metadata(self, query_obj: QueryObject) -> None:
+ """Capture authorized annotation views on a miss before a slow parent
query."""
+ if not query_obj.annotation_layers:
+ return
+ from superset_core.semantic_layers.metadata import MetadataRefreshError
+
+ from superset.semantic_layers.metadata_binding import (
+ metadata_refresh_enabled,
+ participates,
+ )
+ from superset.semantic_layers.models import SemanticView
+
+ if not metadata_refresh_enabled():
+ return
+ layer: dict[str, Any]
+ for layer in query_obj.annotation_layers:
+ if (
+ layer.get("sourceType")
+ not in ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ ):
+ continue
+ value: int | str | None = layer.get("value")
+ chart: Slice | None = (
+ ChartDAO.find_by_id(value) if value is not None else None
+ )
+ if chart is None:
+ continue
+ try:
+ context: QueryContext | None = chart.get_query_context()
+ if context is None:
+ # The annotation executor reports its missing-context
error.
+ continue
+ source: Explorable = context.datasource
+ if not isinstance(source, SemanticView) or not participates(
+ source.semantic_layer
+ ):
+ continue
+ context.raise_for_access()
+ # Reuse the annotation command's canonical query authority.
+ # Later execution retains this view without renewing its
budget.
+ _captured: str | None = source.metadata_cache_token
+ except (SupersetException, MetadataRefreshError) as ex:
+ raise QueryObjectValidationError(error_msg_from_exception(ex))
from ex
Review Comment:
A `MetadataRefreshError` raised while preparing a semantic-view annotation
is converted to `QueryObjectValidationError` here, so chart-data returns a
validation-style 400 with no category. For example, with metadata storage
unavailable the client gets a 400 instead of the 503 `unavailable` / 502 / 504
mapping that `semantic-metadata-operations.md` says applies to chart-data
requests, so retry handling keyed on the status code won't fire. Should this
re-raise the typed error, or should the doc call out this exception?
--
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]
