rusackas commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3657252252
##########
superset/commands/chart/warm_up_cache.py:
##########
@@ -96,12 +101,14 @@ def _warm_up_non_legacy_cache(self, chart: Slice) ->
tuple[Any, Any]:
query_context.force = True
command = ChartDataCommand(query_context)
command.validate()
- payload = command.run()
+ execution: ChartDataExecutionResult = command.execute(
+ ChartDataExecutionOptions(mode=ChartDataExecutionMode.CACHE_ONLY)
+ )
Review Comment:
Traced this through: `force_cached` is False either way inside
`get_query_results_cache_only`, and `_warm_up_non_legacy_cache` sets
`query_context.force = True`, so `acquire_query_data` still hits the DB fresh
on a cache miss. CACHE_ONLY here just skips materializing the full response
payload (`cache_acquired_query` drops the df before returning), it doesn't skip
execution. I think Codeant is reading too much into the name.
##########
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:
This duplication between execute() and execute_async() predates this PR,
it's the same pattern on master already. Worth a follow-up refactor, but not
something I'd pull into this one.
##########
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:
The message that actually reaches callers is the outer
QueryObjectValidationError's generic text, not ex's. from ex just keeps the
original traceback attached for server-side logs, nothing from the RLS
exception leaks into the response.
--
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]
