rusackas commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4297986329
##########
superset/common/query_context_processor.py:
##########
@@ -343,6 +355,32 @@ def get_df_payload_result(
# nonce reads the freshly-cached result instead of recomputing
it.
self._mark_force_executed(query_obj, cache_key,
cache.result_persisted)
+ # Annotation data is fetched per requesting user (and, for chart-backed
+ # layers, scoped by the referenced chart datasource's RLS), so it is
+ # resolved and cached under its own entry — independent of whether the
+ # (shareable) dataframe above was a hit or a miss — rather than forcing
+ # every viewer of the same chart onto their own full dataframe copy.
+ annotation_data: dict[str, Any] = {}
+ if (
+ query_obj
+ and query_obj.annotation_layers
+ and cache.status != QueryStatus.FAILED
Review Comment:
Added tests driving `get_df_payload_result` for a failed dataframe
(annotation helper not called, `annotation_data == {}`, original error kept)
and for an annotation error on a good dataframe. The persisted-only marker
write is covered at the helper boundary in a separate test that parametrizes
`set_and_log_cache` returning True/False.
##########
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:
Agreed, that was a real mismatch. The override range is now resolved per
query with the saved query's `time_shift` (same call the factory makes), so the
window that runs matches what a normal run of that range and shift would run.
Added a test.
##########
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:
Fair. Both tests now require the rotated context to be non-None and compare
the version field (changed) against the rest of the scope (unchanged).
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -126,25 +131,350 @@ def processor(mock_query_context):
return processor
-def test_query_cache_key_binds_annotation_data_to_requesting_user(processor):
- """The cache key for annotated queries must differ per requesting user."""
+def test_annotation_cache_key_binds_native_annotation_read_scope(processor) ->
None:
+ """The annotation cache key for NATIVE layers must differ when the
+ requester's ``can_read`` (Annotation) access differs -- not who they
are."""
query_obj = MagicMock()
query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a",
"value": 1}]
- with (
- patch(
- "superset.common.query_context_processor.get_user_id",
- side_effect=[1, 2],
- ),
- patch("superset.common.query_context_processor.security_manager"),
- ):
- processor.query_cache_key(query_obj)
- processor.query_cache_key(query_obj)
+ # ``security_manager`` autodetects as an async spec under a bare
+ # ``patch()`` (its real object trips ``unittest.mock``'s coroutine
+ # inference), which would silently turn every attribute access into an
+ # ``AsyncMock`` returning a fresh unawaited coroutine per call -- always
+ # unequal to itself and never equal to a configured return value. Forcing
+ # ``new_callable=MagicMock`` keeps these synchronous, as the real object
+ # is.
+ with patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
+ ) as security_manager:
+ security_manager.can_access.side_effect = [True, False]
+ processor.annotation_cache_key(query_obj)
+ processor.annotation_cache_key(query_obj)
contexts = [
call.kwargs["annotation_context"] for call in
query_obj.cache_key.call_args_list
]
assert contexts[0] != contexts[1]
+def test_annotation_cache_key_shares_across_same_access_scope() -> None:
+ """Two distinct requesters (separate processor/query-object instances,
+ standing in for two different requests) with identical access scope must
+ produce identical annotation-context material. Reusing a single
+ processor/query_obj across both calls (as this test previously did)
+ would pass trivially regardless of whether the key is scope-based or
+ identity-based, since nothing about "who's asking" would ever vary."""
+ layer = {"sourceType": "NATIVE", "name": "a", "value": 1}
+ processor_a = QueryContextProcessor(MagicMock())
+ processor_b = QueryContextProcessor(MagicMock())
+ query_obj_a = MagicMock(annotation_layers=[layer])
+ query_obj_b = MagicMock(annotation_layers=[layer])
+
+ with patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
+ ) as security_manager:
+ security_manager.can_access.return_value = True
+ context_a = processor_a._annotation_cache_context(query_obj_a)
+ context_b = processor_b._annotation_cache_context(query_obj_b)
+
+ assert context_a == context_b
Review Comment:
Good call. The test now patches `get_user_id` to return different values per
call (both on the processor module and `superset.utils.core`) and still expects
equal contexts.
--
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]
