bito-code-review[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3610548532
##########
superset/models/sql_lab.py:
##########
@@ -367,8 +490,31 @@ def perm(self) -> str:
def default_endpoint(self) -> str:
return ""
- def get_extra_cache_keys(self, query_obj: QueryObjectDict) ->
list[Hashable]:
- return []
+ def get_extra_cache_keys(self, query_obj: QueryObjectDict) -> list[Any]:
+ from superset.utils.rls import ( # pylint:
disable=import-outside-toplevel
+ collect_rls_predicates_for_sql_or_raise,
+ )
+
+ source = self._get_explore_source()
+ default_schema = self.database.get_default_schema(self.catalog)
+ try:
+ predicates = collect_rls_predicates_for_sql_or_raise(
+ source,
+ self.database,
+ self.catalog,
+ self.schema or default_schema or "",
+ exclude_dataset_id=None,
+ )
+ except Exception as ex:
+ raise QueryObjectValidationError(
+ __("Unable to validate row-level security for this SQL Lab
query.")
+ ) from ex
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Exception chain exposure</b></div>
<div id="fix">
Using `from ex` propagates the internal RLS error (which is raised with
`from None`) to the user-facing exception. For RLS validation failures, only
the generic message should be shown. This follows the pattern used elsewhere in
Superset for internal validation errors (e.g., datasource/api.py:367,
encrypt.py:100).
</div>
</div>
<small><i>Code Review Run #f15b3a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing 403 authorization test coverage</b></div>
<div id="fix">
The new test covers the 400/validation path but not the 403/authorization
path. The `data_from_cache` handler (api.py:451–452) also catches
`SupersetSecurityException` and calls `response_403`; that branch is exercised
only when `command.validate()` raises it, but no test verifies this behavior.
Adding a companion test ensures authorization failures produce a controlled 403
response.
</div>
</div>
<small><i>Code Review Run #f15b3a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate test setup code</b></div>
<div id="fix">
This PR contains syntactic duplication in
`tests/unit_tests/common/test_query_actions_currency.py`. The test setup code
for QueryAcquisitionResult objects (24 lines) is duplicated at lines 122-145
and 290-313. Additionally, a partial duplication (18 lines) exists at lines
76-93 and 132-149. Consider extracting the common test setup into a helper
function or shared fixture to reduce duplication and improve test
maintainability.
</div>
</div>
<small><i>Code Review Run #f15b3a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
