codeant-ai-for-open-source[bot] commented on code in PR #41120:
URL: https://github.com/apache/superset/pull/41120#discussion_r3507229626
##########
superset/models/core.py:
##########
@@ -698,6 +698,46 @@ def get_default_schema_for_query(
self, query, template_params
)
+ def resolve_query_default_schema(
+ self,
+ sql: str,
+ schema: str | None,
+ catalog: str | None,
+ template_params: Optional[dict[str, Any]] = None,
+ ) -> str | None:
+ """
+ Resolve the effective per-query default schema for a SQL string through
+ the query-aware :meth:`get_default_schema_for_query`.
+
+ Builds a transient (unsaved) ``Query`` probe so the engine spec
resolves
+ the schema exactly as execution does -- running engine-specific
per-query
+ security gates too -- then expunges it so the ``database`` backref's
+ ``cascade="all, delete-orphan"`` cannot autoflush this incomplete row
+ into the session. Centralizes the probe construction shared by the SQL
+ Lab executor, the cost-estimate command, and datasource denylist checks
+ so the schema-resolution behavior stays in sync across these
+ security-sensitive paths.
+
+ :param sql: Original (pre-render) SQL the query will execute
+ :param schema: Explicit per-query schema, if any
+ :param catalog: Resolved catalog
+ :param template_params: Jinja template parameters, if any
+ :returns: The runtime-resolved default schema, or None
+ """
+ from superset.models.sql_lab import Query
+
+ probe_query = Query(
+ database=self,
+ sql=sql,
+ schema=schema or None,
+ catalog=catalog,
+ client_id=utils.shortid()[:10],
+ user_id=utils.get_user_id(),
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for the local query probe
variable to satisfy the requirement for annotating relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is new Python code introducing a local variable that can be explicitly
annotated. The custom rule requires type hints for relevant variables where
possible, so omitting an annotation here is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6f5c2ed5d1f410b93f03a0f8a13660d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f6f5c2ed5d1f410b93f03a0f8a13660d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 729:736
**Comment:**
*Custom Rule: Add an explicit type annotation for the local query probe
variable to satisfy the requirement for annotating relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=671e4374de6d3d71e60d02034779b724b69bb4713ed2e7ae7bd3d2c1d46b7cfd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=671e4374de6d3d71e60d02034779b724b69bb4713ed2e7ae7bd3d2c1d46b7cfd&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -1187,6 +1187,31 @@ def get_sqla_row_level_filters(
# for datasources of type query
return []
+ def _resolve_denylist_schema(self, sql: str) -> Optional[str]:
+ """
+ Resolve the effective default schema for ``DISALLOWED_SQL_TABLES``
+ checks, memoized per datasource instance.
+
+ An explicit datasource schema always wins. Otherwise the schema is
+ resolved through the query-aware ``get_default_schema_for_query`` (not
+ the static ``get_default_schema``) so the value matches what the engine
+ uses at runtime -- honoring dynamic-schema engines and URI/connect-arg
+ schema rules -- exactly like the SQL Lab / executor denylist gate.
+
+ The result is cached on the instance so adhoc-expression validation,
+ which runs once per selected column/metric/order-by, resolves the
schema
+ at most once instead of issuing a fallback inspector round-trip per
+ expression.
+ """
+ if self.schema:
+ return self.schema
+ if not hasattr(self, "_denylist_default_schema"):
+ catalog = self.catalog or self.database.get_default_catalog()
Review Comment:
**Suggestion:** Add an explicit type annotation for the `catalog` local
variable to satisfy the type-hint requirement for newly introduced variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python local variable that can be annotated, but
it is assigned without a type hint. That matches the custom rule requiring type
hints for relevant new or modified variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c013870bada54557ac5cbdff9f3a4e10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c013870bada54557ac5cbdff9f3a4e10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1209:1209
**Comment:**
*Custom Rule: Add an explicit type annotation for the `catalog` local
variable to satisfy the type-hint requirement for newly introduced variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=cb7a7930ff7c28e0262fb1ede82419e03828e805241401f5bac188aa9d4d47e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=cb7a7930ff7c28e0262fb1ede82419e03828e805241401f5bac188aa9d4d47e0&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -1480,19 +1504,21 @@ def _raise_for_disallowed_sql(self, sql: str) -> None:
disallowed_functions
):
raise SupersetDisallowedSQLFunctionException(disallowed_functions)
- if disallowed_tables and
parsed_script.check_tables_present(disallowed_tables):
+ if disallowed_tables:
# Report only the tables actually found in the query, mirroring the
# canonical execution-time gate so the user-facing error doesn't
- # echo the operator's full denylist.
- present_tables = {
- table.table.lower()
- for statement in parsed_script.statements
- for table in statement.tables
- }
- found_tables = {
- table for table in disallowed_tables if table.lower() in
present_tables
- }
- raise SupersetDisallowedSQLTableException(found_tables or
disallowed_tables)
+ # echo the operator's full denylist. Honors schema-qualified
+ # denylist entries (e.g. ``information_schema.tables``) and
resolves
+ # unqualified references against the same runtime schema the SQL
Lab
+ # / executor gate resolves them to (via the query-aware resolver,
+ # not the static inspector default), so both surfaces enforce the
+ # denylist consistently.
+ effective_schema = self._resolve_denylist_schema(sql)
+ found_tables = parsed_script.get_disallowed_tables(
+ disallowed_tables, effective_schema
+ )
Review Comment:
**Suggestion:** Add explicit type annotations for the new `effective_schema`
and `found_tables` locals in the query-level disallowed-table check.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
These are new local variables in Python code and they are left implicitly
typed. They are suitable for annotation, so this is a real violation of the
type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=248855e293964d7bb0b7560622333cb8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=248855e293964d7bb0b7560622333cb8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1516:1519
**Comment:**
*Custom Rule: Add explicit type annotations for the new
`effective_schema` and `found_tables` locals in the query-level
disallowed-table check.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=1d07e7457dd3727728356aaaf99d34b717336bc20daa79ae168c8ba6a8453e43&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=1d07e7457dd3727728356aaaf99d34b717336bc20daa79ae168c8ba6a8453e43&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -1187,6 +1187,31 @@ def get_sqla_row_level_filters(
# for datasources of type query
return []
+ def _resolve_denylist_schema(self, sql: str) -> Optional[str]:
+ """
+ Resolve the effective default schema for ``DISALLOWED_SQL_TABLES``
+ checks, memoized per datasource instance.
+
+ An explicit datasource schema always wins. Otherwise the schema is
+ resolved through the query-aware ``get_default_schema_for_query`` (not
+ the static ``get_default_schema``) so the value matches what the engine
+ uses at runtime -- honoring dynamic-schema engines and URI/connect-arg
+ schema rules -- exactly like the SQL Lab / executor denylist gate.
+
+ The result is cached on the instance so adhoc-expression validation,
+ which runs once per selected column/metric/order-by, resolves the
schema
+ at most once instead of issuing a fallback inspector round-trip per
+ expression.
+ """
+ if self.schema:
+ return self.schema
+ if not hasattr(self, "_denylist_default_schema"):
+ catalog = self.catalog or self.database.get_default_catalog()
+ self._denylist_default_schema =
self.database.resolve_query_default_schema(
+ sql, None, catalog
+ )
Review Comment:
**Suggestion:** Provide a type annotation for the cached
`_denylist_default_schema` assignment so this new instance-level value is not
introduced as implicitly typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new instance attribute assignment in Python and it is not
explicitly typed. Since the value is a schema string cache that can be
annotated, the suggestion correctly identifies a type-hint omission.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=633c6fdd44044706ad6136152ae5098f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=633c6fdd44044706ad6136152ae5098f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1210:1212
**Comment:**
*Custom Rule: Provide a type annotation for the cached
`_denylist_default_schema` assignment so this new instance-level value is not
introduced as implicitly typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=f3bc001a0207f09f1dad87d1d6217cf2c0ef02189f8a332efd51e18c06a5061f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=f3bc001a0207f09f1dad87d1d6217cf2c0ef02189f8a332efd51e18c06a5061f&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -1236,24 +1261,23 @@ def _process_sql_expression( # pylint:
disable=too-many-arguments
disallowed_functions
):
raise
SupersetDisallowedSQLFunctionException(disallowed_functions)
- if disallowed_tables and
parsed.check_tables_present(disallowed_tables):
+ if disallowed_tables:
# Report only the tables actually found in the expression,
# mirroring the canonical execution-time gate in
# `superset.sql_lab._validate_query` so the user-facing
- # error doesn't echo the operator's full denylist.
- present_tables = {
- table.table.lower()
- for statement in parsed.statements
- for table in statement.tables
- }
- found_tables = {
- table
- for table in disallowed_tables
- if table.lower() in present_tables
- }
- raise SupersetDisallowedSQLTableException(
- found_tables or disallowed_tables
+ # error doesn't echo the operator's full denylist. Honors
+ # schema-qualified denylist entries (e.g.
+ # ``information_schema.tables``) and resolves unqualified
+ # references against the same runtime schema the SQL Lab /
+ # executor gate resolves them to (via the query-aware
+ # resolver, not the static inspector default), so both
+ # surfaces enforce the denylist consistently.
+ effective_schema =
self._resolve_denylist_schema(sql_to_check)
+ found_tables = parsed.get_disallowed_tables(
+ disallowed_tables, effective_schema
)
Review Comment:
**Suggestion:** Add explicit type annotations for the new `effective_schema`
and `found_tables` locals in the disallowed-table validation path. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
Both locals are newly introduced and can be statically annotated, but they
are not. This is a valid instance of the project rule requiring type hints on
relevant new variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1255a850dece4bdda5a9a69772e6df3c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1255a850dece4bdda5a9a69772e6df3c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1275:1278
**Comment:**
*Custom Rule: Add explicit type annotations for the new
`effective_schema` and `found_tables` locals in the disallowed-table validation
path.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=286809413e3500902e300c57c62641ceb3aa70bae8ef0db0183f323126be3974&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=286809413e3500902e300c57c62641ceb3aa70bae8ef0db0183f323126be3974&reaction=dislike'>👎</a>
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -1395,6 +1397,88 @@ def test_execute_uses_default_catalog_and_schema(
get_default_schema_mock.assert_called()
+def test_resolve_query_schema_uses_query_aware_resolution(
+ mocker: MockerFixture, database: Database, app_context: None
+) -> None:
+ """``_resolve_query_schema`` resolves through the query-aware
+ ``get_default_schema_for_query`` (which runs per-query engine security
gates),
+ handing it a transient probe Query that carries the request's SQL, schema,
+ catalog and template params."""
+ from superset.models.sql_lab import Query
+ from superset.sql.execution.executor import SQLExecutor
+
+ resolve_mock = mocker.patch.object(
+ database, "get_default_schema_for_query",
return_value="resolved_schema"
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this new mock variable
to comply with the rule requiring type hints on relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is introduced without a type annotation, and it is a
relevant variable that can be annotated under the Python type-hints rule. The
suggestion matches an actual omission in the new code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4626cea0a86d461cbd7de0cf4dd612c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4626cea0a86d461cbd7de0cf4dd612c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1410:1412
**Comment:**
*Custom Rule: Add an explicit type annotation for this new mock
variable to comply with the rule requiring type hints on relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=9bf1f1038639c124d895f49e91fa4616f5057967e7844d6923ddb3721e3897a7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=9bf1f1038639c124d895f49e91fa4616f5057967e7844d6923ddb3721e3897a7&reaction=dislike'>👎</a>
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -1395,6 +1397,88 @@ def test_execute_uses_default_catalog_and_schema(
get_default_schema_mock.assert_called()
+def test_resolve_query_schema_uses_query_aware_resolution(
+ mocker: MockerFixture, database: Database, app_context: None
+) -> None:
+ """``_resolve_query_schema`` resolves through the query-aware
+ ``get_default_schema_for_query`` (which runs per-query engine security
gates),
+ handing it a transient probe Query that carries the request's SQL, schema,
+ catalog and template params."""
+ from superset.models.sql_lab import Query
+ from superset.sql.execution.executor import SQLExecutor
+
+ resolve_mock = mocker.patch.object(
+ database, "get_default_schema_for_query",
return_value="resolved_schema"
+ )
+
+ executor = SQLExecutor(database)
+ options = QueryOptions(schema="explicit", template_params={"p": 1})
Review Comment:
**Suggestion:** Add a type annotation for this newly introduced options
variable so the new code consistently follows the type-hint requirement.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python variable that could be type-annotated, and
the code omits an explicit hint. That is a real violation of the type-hints
rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aef3082d91474074848200dd8f9c9256&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aef3082d91474074848200dd8f9c9256&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1415:1415
**Comment:**
*Custom Rule: Add a type annotation for this newly introduced options
variable so the new code consistently follows the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=71d2c21fb9689ff90f1b0898ed86ee677323c37933f4b58bdf183411e78f6fbb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=71d2c21fb9689ff90f1b0898ed86ee677323c37933f4b58bdf183411e78f6fbb&reaction=dislike'>👎</a>
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -1395,6 +1397,88 @@ def test_execute_uses_default_catalog_and_schema(
get_default_schema_mock.assert_called()
+def test_resolve_query_schema_uses_query_aware_resolution(
+ mocker: MockerFixture, database: Database, app_context: None
+) -> None:
+ """``_resolve_query_schema`` resolves through the query-aware
+ ``get_default_schema_for_query`` (which runs per-query engine security
gates),
+ handing it a transient probe Query that carries the request's SQL, schema,
+ catalog and template params."""
+ from superset.models.sql_lab import Query
+ from superset.sql.execution.executor import SQLExecutor
+
+ resolve_mock = mocker.patch.object(
+ database, "get_default_schema_for_query",
return_value="resolved_schema"
+ )
+
+ executor = SQLExecutor(database)
+ options = QueryOptions(schema="explicit", template_params={"p": 1})
+
+ result = executor._resolve_query_schema("SELECT 1", options, "cat")
+
+ assert result == "resolved_schema"
+ probe, template_params = resolve_mock.call_args.args
+ assert isinstance(probe, Query)
+ assert probe.sql == "SELECT 1"
+ assert probe.schema == "explicit"
+ assert probe.catalog == "cat"
+ assert template_params == {"p": 1}
+
+
+def test_resolve_query_schema_omits_blank_schema(
+ mocker: MockerFixture, database: Database, app_context: None
+) -> None:
+ """An unset request schema reaches the probe as ``None`` so the engine spec
+ resolves the runtime default instead of matching on an empty string."""
+ resolve_mock = mocker.patch.object(
+ database, "get_default_schema_for_query", return_value="public"
+ )
+
+ from superset.sql.execution.executor import SQLExecutor
+
+ executor = SQLExecutor(database)
+
+ result = executor._resolve_query_schema("SELECT 1", QueryOptions(), None)
+
+ assert result == "public"
+ probe = resolve_mock.call_args.args[0]
+ assert probe.schema is None
+ assert probe.catalog is None
+
+
+def test_prepare_sql_runs_schema_gate_with_explicit_schema(
+ mocker: MockerFixture, database: Database, app_context: None
+) -> None:
+ """The per-query schema gate must run even when an explicit schema is
+ supplied, so an explicit-schema request cannot smuggle a ``SET
search_path``
+ past the gate the resolver enforces (parity with the estimate path, which
+ resolves unconditionally)."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+ from superset.sql.execution.executor import SQLExecutor
+
+ gate = mocker.patch.object(
+ database,
+ "get_default_schema_for_query",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="blocked",
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
Review Comment:
**Suggestion:** Annotate this new patched gate variable with an explicit
type to satisfy the type-hints rule for relevant variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This patched object is a new local variable and it lacks a type annotation
even though it could be annotated. The suggestion correctly identifies a
type-hint omission.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d669d94d25424c92a52377e12a8c2c09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d669d94d25424c92a52377e12a8c2c09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1460:1470
**Comment:**
*Custom Rule: Annotate this new patched gate variable with an explicit
type to satisfy the type-hints rule for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=60b334f75d322a1c7277f3201c5eb917893fd4ed6d1fade7b7575e24e665ad6a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=60b334f75d322a1c7277f3201c5eb917893fd4ed6d1fade7b7575e24e665ad6a&reaction=dislike'>👎</a>
--
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]
