bito-code-review[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3657255851
##########
tests/unit_tests/common/test_query_actions_currency.py:
##########
@@ -50,241 +63,270 @@ def mock_datasource() -> MagicMock:
return ds
-def test_detect_currency_returns_none_when_form_data_is_none(
- mock_query_obj: MagicMock,
- mock_datasource: MagicMock,
-) -> None:
- """Returns None when query context has no form_data."""
- context = MagicMock()
- context.form_data = None
-
- result = _detect_currency(context, mock_query_obj, mock_datasource)
-
- assert result is None
-
-
-def test_detect_currency_returns_none_when_currency_format_not_dict(
+def test_modern_currency_dependency_is_cache_aware_and_bounded(
+ mock_query_context: MagicMock,
mock_query_obj: MagicMock,
mock_datasource: MagicMock,
) -> None:
- """Returns None when currency_format is not a dict."""
- context = MagicMock()
- context.form_data = {"currency_format": "invalid"}
+ timing = QueryAcquisitionTiming(
+ cache_key_ns=1,
+ cache_read_ns=2,
+ source_ns=3,
+ cache_write_ns=4,
+ cache_write_outcome=CacheWriteOutcome.SUCCEEDED,
+ cache_hit=False,
+ sources=(),
+ )
+ primary = QueryAcquisitionResult(
+ payload={
+ "df": pd.DataFrame({"value": [1]}),
+ "status": QueryStatus.SUCCESS,
+ },
+ timing=timing,
+ )
+ dependency = QueryAcquisitionResult(
+ payload={
+ "df": pd.DataFrame({"currency_code": ["USD"]}),
+ "status": QueryStatus.SUCCESS,
+ },
+ timing=timing,
+ )
+ mock_query_obj.datasource = mock_datasource
+ mock_query_obj.annotation_layers = []
+ mock_query_obj.time_offsets = []
+ mock_query_context.get_df_payload_result.return_value = dependency
- result = _detect_currency(context, mock_query_obj, mock_datasource)
+ combined, currency, _ = _acquire_currency_dependency(
+ mock_query_context,
+ mock_query_obj,
+ primary,
+ force_cached=True,
+ )
- assert result is None
+ hidden_query = mock_query_context.get_df_payload_result.call_args.args[0]
+ assert hidden_query.columns == ["currency_code"]
+ assert hidden_query.filter == [
+ {"col": "currency_code", "op": FilterOperator.IS_NOT_NULL, "val": ""}
+ ]
+ assert hidden_query.row_limit == 2
+ assert hidden_query.metrics == []
+ assert hidden_query.is_timeseries is False
+ assert mock_query_context.get_df_payload_result.call_args.kwargs == {
+ "force_cached": True,
+ "source_kind": SourceKind.CURRENCY_DETECTION,
+ }
+ assert currency == "USD"
+ assert combined.timing.source_ns == 6
-def test_detect_currency_returns_none_when_symbol_not_auto(
+def
test_currency_dependency_preserves_existing_filters_and_excludes_null_codes(
Review Comment:
<!-- Bito Reply -->
That is a great improvement. Extracting the duplicated test setup into a
helper function significantly improves the maintainability and readability of
the test suite.
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -236,3 +252,162 @@ def test_extract_export_filename_preserves_normal_name()
-> None:
def test_extract_export_filename_all_special_falls_back_to_none() -> None:
"""A name with no usable characters becomes None (generated downstream)."""
assert _extract_filename("***") is None
+
+
+def _chart_execution_result(
+ cache_values: tuple[bool | None, ...] = (False,),
+) -> ChartDataExecutionResult:
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.JSON
+ timing = QueryTiming(
+ cache_key_ns=1_000_000,
+ cache_read_ns=2_000_000,
+ source_ns=3_000_000,
+ cache_write_ns=None,
+ cache_write_outcome=CacheWriteOutcome.NOT_ATTEMPTED,
+ materialization_ns=4_000_000,
+ total_ns=10_000_000,
+ cache_hit=False,
+ sources=(),
+ )
+ return ChartDataExecutionResult(
+ query_context=query_context,
+ queries=tuple(
+ QueryDataResult({"data": [{"value": 1}], "is_cached": is_cached},
timing)
+ for is_cached in cache_values
+ ),
+ )
+
+
+def _send_chart_json_response(
+ execution: ChartDataExecutionResult, include_timing: bool
+) -> dict[str, Any]:
+ app = Flask(__name__)
+ app.config["CHART_DATA_INCLUDE_TIMING"] = include_timing
+ api = ChartDataRestApi.__new__(ChartDataRestApi)
+ event_logger = MagicMock()
+ event_logger.log_context = MagicMock(return_value=nullcontext())
+ security_manager = MagicMock()
+ security_manager.is_guest_user.return_value = False
+
+ with (
+ app.test_request_context("/api/v1/chart/data"),
+ patch("superset.charts.data.api.event_logger", event_logger),
+ patch("superset.charts.data.api.security_manager", security_manager),
+ ):
+ response = api._send_chart_response(execution)
+
+ return json.loads(response.get_data(as_text=True))
+
+
+def test_chart_response_keeps_timing_out_of_default_contract() -> None:
+ execution = _chart_execution_result()
+
+ payload = _send_chart_json_response(execution, include_timing=False)
+
+ assert payload == {"result": [{"data": [{"value": 1}], "is_cached":
False}]}
+ assert "timing" not in execution.queries[0].payload
+
+
+def test_chart_response_projects_timing_without_mutating_execution() -> None:
+ execution = _chart_execution_result()
+
+ payload = _send_chart_json_response(execution, include_timing=True)
+
+ assert payload["result"][0]["timing"]["version"] == 1
+ assert payload["result"][0]["timing"]["query"]["total_ms"] == 10.0
+ assert "timing" not in execution.queries[0].payload
+
+
+def test_log_is_cached_uses_scalar_for_single_query() -> None:
+ add_extra_log_payload = MagicMock()
+
+ ChartDataRestApi._log_is_cached(
+ MagicMock(), _chart_execution_result((True,)), add_extra_log_payload
+ )
+
+ add_extra_log_payload.assert_called_once_with(is_cached=True)
+
+
+def test_log_is_cached_uses_list_for_multiple_queries() -> None:
+ add_extra_log_payload = MagicMock()
+
+ ChartDataRestApi._log_is_cached(
+ MagicMock(), _chart_execution_result((False, True)),
add_extra_log_payload
+ )
+
+ add_extra_log_payload.assert_called_once_with(is_cached=[False, True])
+
+
+def test_async_execution_requires_nonempty_data_backed_queries() -> None:
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.queries = []
+
+ assert _supports_async_execution(query_context) is False
+
+ query_context.queries = [MagicMock(result_type=None)]
+ assert _supports_async_execution(query_context) is True
+
+
query_context.queries.append(MagicMock(result_type=ChartDataResultType.QUERY))
+ assert _supports_async_execution(query_context) is False
+
+
+def test_async_cache_lookup_uses_typed_execution_options() -> None:
+ app = Flask(__name__)
+ api = ChartDataRestApi.__new__(ChartDataRestApi)
+ command = MagicMock()
+ execution = _chart_execution_result()
+ command.execute.return_value = execution
+ expected_response = Response("{}", status=200, mimetype="application/json")
+
+ with (
+ app.test_request_context("/api/v1/chart/data"),
+ patch.object(
+ api, "_send_chart_response", return_value=expected_response
+ ) as send_response,
+ ):
+ response = api._run_async({}, command)
+
+ assert response is expected_response
+ command.execute.assert_called_once_with(
+ ChartDataExecutionOptions(force_cached=True)
+ )
+ send_response.assert_called_once_with(execution)
+
+
+def test_chart_data_query_failure_returns_controlled_400(app: SupersetApp) ->
None:
+ api = ChartDataRestApi.__new__(ChartDataRestApi)
+ command = MagicMock()
+ command.execute.side_effect = ChartDataQueryFailedError(
+ "Annotation layer with ID 8 was not found."
+ )
+
+ with app.test_request_context("/api/v1/chart/data"):
+ response = ChartDataRestApi._get_data_response.__wrapped__(api,
command)
+
+ assert response.status_code == 400
+ assert json.loads(response.get_data(as_text=True)) == {
+ "message": "Annotation layer with ID 8 was not found."
+ }
+
+
+def test_cached_chart_validation_failure_returns_controlled_400(
+ app: SupersetApp,
+) -> None:
+ api = ChartDataRestApi.__new__(ChartDataRestApi)
+ api._load_query_context_form_from_cache = MagicMock(return_value={})
+ api._create_query_context_from_form = MagicMock(return_value=MagicMock())
+ api.response_400 = MagicMock(return_value=Response(status=400))
+ command = MagicMock()
+ command.validate.side_effect = QueryObjectValidationError("Invalid query
context")
+
+ with (
+ app.test_request_context("/api/v1/chart/data/cache-key"),
+ patch("superset.charts.data.api.ChartDataCommand",
return_value=command),
+ ):
+ response = unwrap(ChartDataRestApi.data_from_cache)(api, "cache-key")
+
+ assert response.status_code == 400
+ api.response_400.assert_called_once_with(message="Invalid query context")
Review Comment:
<!-- Bito Reply -->
The addition of the test case
`test_cached_chart_authorization_failure_returns_controlled_403` effectively
addresses the reviewer's suggestion to ensure that authorization failures in
the `data_from_cache` path produce a controlled 403 response. This complements
the existing test coverage for validation failures and ensures consistent error
handling for security exceptions.
--
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]
