rusackas commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4344499888
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -126,23 +135,471 @@ 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}]
+ # ``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])
+
+ # The requesting user differs between the two calls. Binding user identity
+ # into the context (by name, from either module) would make the contexts
+ # diverge, so equality below proves the key is scope-based.
with (
+ patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
+ ) as security_manager,
patch(
"superset.common.query_context_processor.get_user_id",
+ create=True,
side_effect=[1, 2],
),
- patch("superset.common.query_context_processor.security_manager"),
+ patch("superset.utils.core.get_user_id", side_effect=[1, 2]),
+ ):
+ 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
+
+
+def test_query_cache_key_does_not_bind_annotation_scope(processor) -> None:
+ """The dataframe cache key must stay shared across viewers of the same
+ chart, even when the query has annotation layers — only the separate
+ annotation cache key (see above) carries access-scope material."""
+ query_obj = MagicMock()
+ query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a",
"value": 1}]
+ with patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
):
processor.query_cache_key(query_obj)
processor.query_cache_key(query_obj)
- contexts = [
- call.kwargs["annotation_context"] for call in
query_obj.cache_key.call_args_list
+ for call in query_obj.cache_key.call_args_list:
+ assert "annotation_context" not in call.kwargs
+
+
[email protected]
+def mock_annotation_chart() -> Iterator[MagicMock]:
+ """A found chart, wired as the referenced chart for
+ ``_annotation_source_scope`` tests -- factors out the repeated
+ ``ChartDAO.find_by_id`` patch those tests all need."""
+ chart = MagicMock()
+ with patch(
+ "superset.common.query_context_processor.ChartDAO.find_by_id",
+ return_value=chart,
+ ):
+ yield chart
+
+
+def test_annotation_source_scope_binds_datasource_access(
+ processor: QueryContextProcessor, mock_annotation_chart: MagicMock
+) -> None:
+ """A chart-backed annotation layer's scope must differ when the
+ requester's access to the referenced datasource differs."""
+ mock_annotation_chart.get_query_context.return_value = None
+ with patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
+ ) as security_manager:
+ security_manager.can_access_datasource.side_effect = [True, False]
+ security_manager.get_rls_cache_key.return_value = []
+ scope_a = processor._annotation_source_scope({"value": 1})
+ scope_b = processor._annotation_source_scope({"value": 1})
+ assert scope_a is not None
+ assert scope_b is not None
+ assert scope_a != scope_b
+ assert scope_a["access"] is True
+ assert scope_b["access"] is False
+
+
+def test_annotation_source_scope_reuses_referenced_chart_cache_key(
+ processor: QueryContextProcessor, mock_annotation_chart: MagicMock
+) -> None:
+ """When the referenced chart has a saved query context, its own cache
+ key(s) -- covering RLS and per-user Jinja/virtual-dataset material -- are
+ reused rather than re-derived."""
+ mock_query_object: MagicMock = MagicMock()
+ mock_query_context: MagicMock = MagicMock()
+ mock_query_context.queries = [mock_query_object]
+ mock_query_context.query_cache_key.return_value = "referenced-chart-key"
+ mock_annotation_chart.get_query_context.return_value = mock_query_context
+ with patch(
+ "superset.common.query_context_processor.security_manager",
+ new_callable=MagicMock,
+ ) as security_manager:
+ security_manager.can_access_datasource.return_value = True
+ scope = processor._annotation_source_scope({"value": 1})
+ assert scope == {"access": True, "data_key": ["referenced-chart-key"]}
+
mock_query_context.query_cache_key.assert_called_once_with(mock_query_object)
+
+
+def test_annotation_source_scope_keys_on_result_type_prepared_query(
+ processor: QueryContextProcessor, mock_annotation_chart: MagicMock
+) -> None:
+ """A referenced chart saved with a samples result type executes a
+ rewritten query (metrics cleared, every column selected), so its key
+ must be derived from that prepared query, not the saved one -- a Jinja
+ clause that only renders user-specific material without metrics would
+ otherwise give two viewers one key over different rows."""
+ from superset.common.query_object import QueryObject
+
+ mock_datasource = MagicMock()
+ mock_datasource.columns = []
+ saved_query = QueryObject(
+ datasource=mock_datasource, metrics=["count"], columns=["col1"]
+ )
+ mock_query_context = MagicMock()
+ mock_query_context.result_type = ChartDataResultType.SAMPLES
Review Comment:
Added
`test_annotation_source_scope_keys_on_query_level_samples_result_type`: a
`FULL` context with a query-level `SAMPLES` result type, asserting `data_key`
comes from the cleared-metrics query.
--
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]
