sadpandajoe commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4323408810
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -3167,11 +3915,211 @@ def payload(context: QueryContext, **kwargs: Any) ->
dict[str, Any]:
datasource="native-1", annotation_context=context
)
assert current_key != legacy_key
- assert context == {
- **legacy_context,
- "semantic_filter_protocol": "semantic-null-filters-v1",
- "source_versions": {"7": view.metadata_generation},
- }
+ assert context is not None
+ assert context["source_scope"][0]["semantic_filter_protocol"] == (
+ "semantic-null-filters-v1"
+ )
+ assert context["source_scope"][0]["metadata_version"] ==
view.metadata_generation
+
+
+def _annotated_payload_fixture(
+ *, status: str = "success"
+) -> tuple[QueryContextProcessor, Any, MagicMock]:
+ """A processor, annotated query object, and a dataframe cache hit."""
+ from superset.common.query_object import QueryObject
+
+ mock_query_context = MagicMock()
+ mock_query_context.force = False
+ mock_datasource = MagicMock()
+ mock_datasource.column_names = ["col1"]
+ processor = QueryContextProcessor(mock_query_context)
+ processor._qc_datasource = mock_datasource
+ query_obj = QueryObject(
+ datasource=mock_datasource,
+ columns=["col1"],
+ annotation_layers=[
+ {
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "name": "a",
+ "value": 1,
+ }
+ ],
+ )
+ cache = MagicMock()
+ cache.is_loaded = True
+ cache.applied_filter_columns = ["col1"]
+ cache.df = pd.DataFrame({"col1": [1, 2, 3]})
+ cache.query = ""
+ cache.status = status
+ cache.error_message = "dataframe failed" if status == QueryStatus.FAILED
else None
+ cache.is_cached = True
+ cache.applied_template_filters = []
+ cache.rejected_filter_columns = []
+ cache.bq_memory_limited = False
+ cache.result_persisted = False
+ return processor, query_obj, cache
+
+
+def _run_annotated_payload(
+ processor: QueryContextProcessor,
+ query_obj: Any,
+ cache: MagicMock,
+ *,
+ force_cached: bool = False,
+) -> Any:
+ with (
+ patch(
+ "superset.common.query_context_processor.QueryCacheManager"
+ ) as mock_cache_manager,
+ patch.object(query_obj, "validate", return_value=None),
+ patch.object(processor, "query_cache_key", return_value="df-key"),
+ patch.object(processor, "annotation_cache_key",
return_value="ann-key"),
+ patch.object(processor, "get_cache_timeout", return_value=3600),
+ ):
+ mock_cache_manager.get.return_value = cache
+ return processor.get_df_payload_result(query_obj,
force_cached=force_cached)
+
+
+def test_get_df_payload_result_skips_annotations_when_dataframe_failed() ->
None:
+ """A failed dataframe keeps its own error and never triggers the fetch."""
+ processor, query_obj, cache =
_annotated_payload_fixture(status=QueryStatus.FAILED)
+ with patch.object(processor, "_get_annotation_data_cached") as
mock_annotation:
+ result = _run_annotated_payload(processor, query_obj, cache)
+ mock_annotation.assert_not_called()
+ assert result.payload["annotation_data"] == {}
+ assert result.payload["error"] == "dataframe failed"
+
+
+def test_get_df_payload_result_force_cached_misses_without_annotation_entry()
-> None:
+ """Through the real helper, ``force_cached`` with no annotation entry
+ raises ``CacheLoadError`` instead of running the annotation SQL."""
+ processor, query_obj, cache = _annotated_payload_fixture()
+ with (
+ patch("superset.common.query_context_processor.cache_manager") as cm,
+ patch.object(processor, "get_annotation_data") as mock_live,
+ ):
+ cm.data_cache.get.return_value = None
+ with pytest.raises(CacheLoadError):
+ _run_annotated_payload(processor, query_obj, cache,
force_cached=True)
+ mock_live.assert_not_called()
+
+
+def test_get_df_payload_result_annotation_failure_marks_payload_failed() ->
None:
+ """An annotation error on a good dataframe fails the payload."""
+ processor, query_obj, cache = _annotated_payload_fixture()
+ with (
+ patch("superset.common.query_context_processor.cache_manager") as cm,
+ patch.object(
+ processor,
+ "get_annotation_data",
+ side_effect=QueryObjectValidationError("annotation boom"),
+ ),
+ ):
+ cm.data_cache.get.return_value = None
+ result = _run_annotated_payload(processor, query_obj, cache)
+ assert cache.status == QueryStatus.FAILED
+ assert result.payload["error"] == "annotation boom"
Review Comment:
This assertion also passes if the annotation catch goes back to storing a
plain `str(ex)`, because `QueryObjectValidationMessage` is a `str` subclass and
`==` ignores the type. Reverting the fix would leave this test green while a
deleted annotation layer surfaces as `ChartDataQueryFailedError` instead of the
400 validation error. Could this also assert
`isinstance(result.payload["error"], QueryObjectValidationMessage)` (and that
the cause is retained), as `test_get_df_payload_keeps_dataset_validation_type`
does for the dataframe path?
--
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]
