bito-code-review[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3657254389
##########
superset/sql/execution/executor.py:
##########
@@ -226,24 +239,24 @@ def execute(
try:
# 1. Prepare SQL (assembly only, no security checks)
- original_script, transformed_script, catalog, schema =
self._prepare_sql(
- sql, opts
- )
+ prepared = self._prepare_sql(sql, opts)
# 2. Security checks on transformed script
- self._check_security(transformed_script, schema)
+ self._check_security(prepared.transformed_script, prepared.schema)
# 3. Get mutation status and format SQL
- has_mutation = transformed_script.has_mutation()
- final_sql = transformed_script.format()
+ has_mutation = prepared.transformed_script.has_mutation()
+ final_sql = prepared.transformed_script.format()
# DRY RUN: Return transformed SQL without execution
if opts.dry_run:
total_execution_time_ms = (time.time() - start_time) * 1000
# Create a StatementResult for each statement in dry-run mode
- original_sqls = [stmt.format() for stmt in
original_script.statements]
+ original_sqls = [
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to extract the SQL transformation and dry-run
logic into a shared helper function is appropriate. This refactoring would
improve maintainability and reduce code redundancy in the executor, as the
logic is currently duplicated in two separate locations.
##########
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:
<!-- Bito Reply -->
The suggestion is appropriate. By using `from ex` in the `except` block, the
original exception (which contains the RLS validation failure details) is
attached to the `QueryObjectValidationError`. This ensures the traceback is
preserved for server-side logging while the user only sees the generic error
message defined in the `QueryObjectValidationError`, preventing sensitive RLS
information from leaking into the response.
**superset/models/sql_lab.py**
```
except Exception as ex:
raise QueryObjectValidationError(
__("Unable to validate row-level security for this SQL Lab
query.")
) from ex
```
--
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]
