codeant-ai-for-open-source[bot] commented on code in PR #42079:
URL: https://github.com/apache/superset/pull/42079#discussion_r3586061661
##########
tests/unit_tests/models/core_test.py:
##########
@@ -716,142 +716,104 @@ def
test_get_sqla_engine_user_impersonation_email(mocker: MockerFixture) -> None
)
-def test_get_sqla_engine_registers_prequery_event_listener(
+def test_get_sqla_engine_no_event_listener_for_prequeries(
app_context: None,
mocker: MockerFixture,
) -> None:
"""
- Test that get_sqla_engine registers a connect event listener for
prequeries.
+ Test that get_sqla_engine does not register connect event listeners.
- Engines returned by get_sqla_engine must automatically execute prequeries
- (e.g. SET search_path) on every new connection, so that callers don't need
- to remember to call get_prequeries() themselves.
+ Prequeries are executed directly on the connection in get_raw_connection()
+ to avoid thread-safety issues with shared cached engines (#40903).
"""
mock_engine = mocker.MagicMock()
mocker.patch.object(Database, "_get_sqla_engine", return_value=mock_engine)
db_engine_spec = mocker.patch.object(Database, "db_engine_spec")
db_engine_spec.get_prequeries.return_value = ['SET search_path =
"my_schema"']
event_listen = mocker.patch("superset.models.core.sqla.event.listen")
- mocker.patch("superset.models.core.sqla.event.remove")
database = Database(database_name="my_db", sqlalchemy_uri="postgresql://")
with database.get_sqla_engine(catalog="my_catalog", schema="my_schema"):
pass
- db_engine_spec.get_prequeries.assert_called_once_with(
- database=database,
- catalog="my_catalog",
- schema="my_schema",
- )
- event_listen.assert_called_once_with(mock_engine, "connect", mocker.ANY)
-
- # Call the captured closure directly to verify cursor create β execute β
close.
- captured_fn = event_listen.call_args[0][2]
- mock_dbapi_conn = mocker.MagicMock()
- mock_cursor = mocker.MagicMock()
- mock_dbapi_conn.cursor.return_value = mock_cursor
- captured_fn(mock_dbapi_conn, None)
- mock_cursor.execute.assert_called_once_with('SET search_path =
"my_schema"')
- mock_cursor.close.assert_called_once()
+ event_listen.assert_not_called()
-def test_get_sqla_engine_prequery_cursor_closed_on_exception(
+def test_get_raw_connection_executes_prequeries(
app_context: None,
mocker: MockerFixture,
) -> None:
"""
- Test that the cursor is always closed even when a prequery raises.
+ Test that get_raw_connection() runs prequeries directly on the connection.
"""
mock_engine = mocker.MagicMock()
mocker.patch.object(Database, "_get_sqla_engine", return_value=mock_engine)
db_engine_spec = mocker.patch.object(Database, "db_engine_spec")
- db_engine_spec.get_prequeries.return_value = ['SET search_path =
"bad_schema"']
- event_listen = mocker.patch("superset.models.core.sqla.event.listen")
- mocker.patch("superset.models.core.sqla.event.remove")
-
- database = Database(database_name="my_db", sqlalchemy_uri="postgresql://")
- with database.get_sqla_engine(catalog=None, schema="bad_schema"):
- pass
+ prequery = 'SET search_path = "my_schema"'
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local variable so the new code fully complies with the type-hinting
requirement. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The new local variable is introduced without a type annotation in Python
code, and it can clearly be annotated as a string. This matches the rule
requiring type hints on relevant variables that can be annotated.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=28b68e99b4aa49feb96da17622aeb1af&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=28b68e99b4aa49feb96da17622aeb1af&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/models/core_test.py
**Line:** 753:753
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local variable so the new code fully complies with the type-hinting 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%2F42079&comment_hash=0bc293d4a6f7091a82c2d115ba9f87693af9e82dafa8a3cfa7c8d05d7ce2d785&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42079&comment_hash=0bc293d4a6f7091a82c2d115ba9f87693af9e82dafa8a3cfa7c8d05d7ce2d785&reaction=dislike'>π</a>
##########
superset/models/core.py:
##########
@@ -686,6 +645,18 @@ def get_raw_connection(
) as engine:
with check_for_oauth2(self):
with closing(engine.raw_connection()) as conn:
+ prequeries = self.db_engine_spec.get_prequeries(
+ database=self,
+ catalog=catalog,
+ schema=schema,
+ )
+ if prequeries:
+ cursor = conn.cursor()
Review Comment:
**Suggestion:** Add a type annotation for this cursor variable so the new
database execution block remains fully type-hinted. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a newly added local variable in Python code and it is not
type-annotated. That matches the type-hinting rule for new or modified code, so
the suggestion is valid.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fb154f87d5db49ee9d7f89101fe7ba8d&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=fb154f87d5db49ee9d7f89101fe7ba8d&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:** 654:654
**Comment:**
*Custom Rule: Add a type annotation for this cursor variable so the new
database execution block remains fully type-hinted.
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%2F42079&comment_hash=c226d9017e2f0ba680fc8b5ffc94be7c0d2c8600e6ad17716a0551e58800fcda&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42079&comment_hash=c226d9017e2f0ba680fc8b5ffc94be7c0d2c8600e6ad17716a0551e58800fcda&reaction=dislike'>π</a>
##########
superset/models/core.py:
##########
@@ -686,6 +645,18 @@ def get_raw_connection(
) as engine:
with check_for_oauth2(self):
with closing(engine.raw_connection()) as conn:
+ prequeries = self.db_engine_spec.get_prequeries(
+ database=self,
+ catalog=catalog,
+ schema=schema,
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local variable to comply with the projectβs type-hinting requirement.
[custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is newly added Python code that introduces a local variable without a
type annotation. Since the project rule requires type hints on relevant
annotatable variables, this is a real violation.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ba6fdbaf7ef249298e9dd084d488e829&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=ba6fdbaf7ef249298e9dd084d488e829&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:** 648:651
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local variable to comply with the projectβs type-hinting 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%2F42079&comment_hash=f438fd6383e603eef969b49f763ca9455469d22db7629324dd14c8a15e90614b&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42079&comment_hash=f438fd6383e603eef969b49f763ca9455469d22db7629324dd14c8a15e90614b&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]
