codeant-ai-for-open-source[bot] commented on code in PR #43263:
URL: https://github.com/apache/superset/pull/43263#discussion_r3797518356
##########
tests/unit_tests/databases/filters_test.py:
##########
@@ -117,7 +117,9 @@ def test_database_filter(mocker: MockerFixture) -> None:
engine,
compile_kwargs={"literal_binds": True},
)
- assert (
- str(compiled_query)
- == "SELECT dbs.id, dbs.verbose_name, dbs.database_name,
dbs.sqlalchemy_uri, dbs.password, dbs.cache_timeout,
dbs.select_as_create_table_as, dbs.expose_in_sqllab, dbs.configuration_method,
dbs.allow_run_async, dbs.allow_file_upload, dbs.allow_ctas, dbs.allow_cvas,
dbs.allow_dml, dbs.force_ctas_schema, dbs.extra, dbs.encrypted_extra,
dbs.impersonate_user, dbs.server_cert, dbs.is_managed_externally,
dbs.external_url, dbs.created_on, dbs.changed_on, dbs.created_by_fk,
dbs.changed_by_fk, dbs.uuid, ssh_tunnels_1.id AS id_1,
ssh_tunnels_1.database_id, ssh_tunnels_1.server_address,
ssh_tunnels_1.server_port, ssh_tunnels_1.username, ssh_tunnels_1.password AS
password_1, ssh_tunnels_1.private_key, ssh_tunnels_1.private_key_password,
ssh_tunnels_1.server_host_key, ssh_tunnels_1.created_on AS created_on_1,
ssh_tunnels_1.changed_on AS changed_on_1, ssh_tunnels_1.created_by_fk AS
created_by_fk_1, ssh_tunnels_1.changed_by_fk AS changed_by_fk_1,
ssh_tunnels_1.extra_json, ssh_tunnels_1.uuid
AS uuid_1 \nFROM dbs LEFT OUTER JOIN ssh_tunnels AS ssh_tunnels_1 ON dbs.id =
ssh_tunnels_1.database_id \nWHERE ('[' || dbs.database_name || '].(id:' ||
CAST(dbs.id AS VARCHAR) || ')') IN ('[my_db].(id:42)', '[my_other_db].(id:43)')
OR dbs.database_name IN ('my_db', 'my_other_db', 'third_db')" # noqa: E501
- )
+ sql = str(compiled_query)
+ # SQLAlchemy 1.4 and 2.x produce the same filter but differ in inherited
+ # column order and optional grouping parentheses when compiling it.
+ assert "FROM dbs LEFT OUTER JOIN ssh_tunnels" in sql
+ assert "'[my_db].(id:42)', '[my_other_db].(id:43)'" in sql
+ assert "dbs.database_name IN ('my_db', 'my_other_db', 'third_db')" in sql
Review Comment:
**Suggestion:** The assertions only verify that each predicate appears
somewhere in the SQL, not that the permission predicates are combined with the
required `OR` grouping. A regression changing the authorization expression to
`AND`, or otherwise altering its boolean grouping, would still pass all three
assertions while returning an incorrect set of accessible databases. Assert the
complete predicate structure or execute the query against representative rows.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Database listing authorization regressions remain undetected.
- ⚠️ Report APIs apply `DatabaseFilter` to database relations.
- ⚠️ Saved-query database selectors use the same filter.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e03038c4c73b4f858d79a73ace4373ec&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=e03038c4c73b4f858d79a73ace4373ec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/filters_test.py
**Line:** 123:125
**Comment:**
*Incomplete Implementation: The assertions only verify that each
predicate appears somewhere in the SQL, not that the permission predicates are
combined with the required `OR` grouping. A regression changing the
authorization expression to `AND`, or otherwise altering its boolean grouping,
would still pass all three assertions while returning an incorrect set of
accessible databases. Assert the complete predicate structure or execute the
query against representative rows.
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%2F43263&comment_hash=81c40846aa1ae059d7cda817457c58edf1f52170dd1d3dff3db2b67c01099634&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43263&comment_hash=81c40846aa1ae059d7cda817457c58edf1f52170dd1d3dff3db2b67c01099634&reaction=dislike'>👎</a>
##########
superset/utils/core.py:
##########
@@ -829,7 +830,8 @@ def ping_connection(connection: Connection, branch: bool)
-> None:
# here also causes the whole connection pool to be invalidated
# so that all stale connections are discarded.
connection.scalar(select(1))
- connection.rollback() # pylint:
disable=consider-using-transaction
+ if transaction := connection.get_transaction():
+ transaction.rollback()
Review Comment:
**Suggestion:** In the invalidated-connection path, the failed `SELECT 1`
has already started a transaction under SQLAlchemy 2.x. Retrying
`connection.scalar(select(1))` before rolling back leaves the invalidated
connection in an active transaction, so SQLAlchemy can raise
`PendingRollbackError` instead of reconnecting. Roll back the existing
transaction before the retry, then roll back any transaction created by the
retry. [state/lifecycle]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Stale pooled connections can fail health-check recovery.
- ⚠️ Requests requiring `db.engine` may receive connection errors.
- ⚠️ Transient database disconnects may become application-level failures.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6d867b0d796487c8340360db7f9f2fd&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=f6d867b0d796487c8340360db7f9f2fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/core.py
**Line:** 833:834
**Comment:**
*State Lifecycle: In the invalidated-connection path, the failed
`SELECT 1` has already started a transaction under SQLAlchemy 2.x. Retrying
`connection.scalar(select(1))` before rolling back leaves the invalidated
connection in an active transaction, so SQLAlchemy can raise
`PendingRollbackError` instead of reconnecting. Roll back the existing
transaction before the retry, then roll back any transaction created by the
retry.
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%2F43263&comment_hash=13b037ea3f5318f668e2b2c27f83254518b4ef550d857c9d2a5c5a746cc6f025&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43263&comment_hash=13b037ea3f5318f668e2b2c27f83254518b4ef550d857c9d2a5c5a746cc6f025&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]
