sadpandajoe commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4246802793
##########
superset/common/query_context_processor.py:
##########
@@ -937,6 +1119,51 @@ def get_native_annotation_data(query_obj: QueryObject) ->
dict[str, Any]:
annotation_data[layer_name] = result
return annotation_data
+ @staticmethod
+ def _apply_annotation_overrides(
+ query_context: QueryContext, annotation_layer: dict[str, Any]
+ ) -> None:
+ """
+ Apply a chart-backed annotation layer's per-request overrides (time
+ grain, time range) to its saved query context's queries, in place.
+
+ Shared by :meth:`get_viz_annotation_data` (which executes the
+ overridden query) and :meth:`_annotation_source_scope` (which derives
+ its cache key from it), so the key always reflects the query that
+ actually runs: an override can introduce per-user Jinja/RLS material
+ the saved, un-overridden query lacks (e.g. a template that only calls
+ ``current_user_id()`` at a finer time grain), which deriving the key
+ from the un-overridden query would silently miss.
+ """
+ # Only the source chart's rows are used. Dropping its own annotation
+ # layers also stops charts that annotate each other (A -> B -> A)
+ # from recursing.
+ for query_object in query_context.queries:
+ query_object.annotation_layers = []
+
+ if not (overrides := annotation_layer.get("overrides")):
+ return
+
+ if time_grain_sqla := overrides.get("time_grain_sqla"):
+ for query_object in query_context.queries:
+ query_object.extras["time_grain_sqla"] = time_grain_sqla
+
+ if time_range := overrides.get("time_range"):
+ from_dttm, to_dttm = get_since_until_from_time_range(time_range)
+
+ for query_object in query_context.queries:
+ query_object.from_dttm = from_dttm
+ query_object.to_dttm = to_dttm
+ # Keep the logical range alongside the resolved bounds:
+ # ``QueryObject.cache_key`` keys on ``time_range`` when set
+ # (dropping the bounds), so a relative override such as
+ # "Last week" keeps one key as time passes instead of
+ # contributing freshly resolved timestamps on every request
+ # (which would defeat cache and force-nonce reuse), and an
+ # override on a saved query without a range of its own is
+ # still told apart from a different override.
+ query_object.time_range = time_range
Review Comment:
Assigning `query_object.time_range` here makes `QueryObject.cache_key()`
drop the resolved bounds, but the bounds above are computed without the saved
query's `time_shift`. For a saved annotation chart with `time_shift="1 year
ago"`, a `2025-01-01 : 2025-02-01` override executes January 2025 data, while a
normal run of that chart with the same range and shift executes January 2024;
both now produce the same query cache key. Whichever runs first populates the
entry the other reads, so the annotation can show the wrong year's rows (or the
chart can show the annotation's). Should the override keep the explicit bounds
in the key whenever the query carries a `time_shift`, or apply the shift to the
override range the way the query factory does?
##########
tests/unit_tests/semantic_layers/simple_cache_clear_test.py:
##########
@@ -307,32 +317,24 @@ def
test_annotation_result_key_uses_saved_source_generation(
from superset.common.query_context_processor import QueryContextProcessor
view: SemanticView = SemanticView(name="view", semantic_layer=layer)
- chart: Mock =
Mock(query_context='{"datasource":{"id":17,"type":"semantic_view"}}')
+ chart: Mock = _saved_context_chart(view)
query: Mock = Mock(annotation_layers=[{"sourceType": "line", "value": 7}])
processor: QueryContextProcessor = QueryContextProcessor(Mock())
- get_datasource: Mock
with (
app.app_context(),
patch(
"superset.common.query_context_processor.ChartDAO.find_by_id",
return_value=chart,
),
- patch(
- "superset.daos.datasource.DatasourceDAO.get_datasource",
- return_value=view,
- ) as get_datasource,
- patch(
-
"superset.common.query_context_processor.security_manager.get_rls_cache_key",
- return_value=[],
- ),
):
- first: dict[str, Any] = processor._annotation_cache_context(query)
+ first: dict[str, Any] | None =
processor._annotation_cache_context(query)
+ assert first is not None
layer.cache_version = 1
session.info.clear()
view.forget_metadata()
assert processor._annotation_cache_context(query) != first
Review Comment:
This inequality also passes if scope derivation starts failing after the
version rotates: `_annotation_cache_context` returns `None` on any derivation
error, and `None != first` is true. A regression that silently disables
annotation caching for semantic views after a metadata change would therefore
still satisfy this test (the `call_count == 2` check below doesn't distinguish
it either), and the same applies to the version-rotation assertion in
`test_cache_keys_compose_metadata_and_producer_versions`. Could the second
context be captured and asserted non-`None`, with the rotated version field
compared and the unchanged one asserted equal?
--
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]
