codeant-ai-for-open-source[bot] commented on code in PR #42277:
URL: https://github.com/apache/superset/pull/42277#discussion_r3624468522
##########
superset/cli/test_db.py:
##########
@@ -280,7 +280,7 @@ def test_sqlalchemy_dialect(
"""
Test the SQLAlchemy dialect, making sure it supports everything Superset
needs.
"""
- engine = create_engine(sqlalchemy_uri, **engine_kwargs)
+ engine = create_engine(sqlalchemy_uri, future=True, **engine_kwargs)
Review Comment:
**Suggestion:** This call can fail with `TypeError: got multiple values for
keyword argument` if `engine_kwargs` already includes `future`, which is
plausible since this function accepts externally supplied engine options. Merge
defaults safely instead of passing the same keyword both explicitly and via
`**engine_kwargs`. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ CLI database dialect test crashes for some engine options.
- ⚠️ Automated DB validation scripts may fail unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Python shell in the Superset environment and import the function
`test_sqlalchemy_dialect` from `superset/cli/test_db.py` (definition starts
at line 275).
2. Construct `engine_kwargs` with a `future` key, e.g. `engine_kwargs =
{"future": True}`.
This is supported by the function signature which accepts arbitrary
SQLAlchemy engine
options.
3. Call `test_sqlalchemy_dialect(console=Console(),
sqlalchemy_uri="sqlite://",
engine_kwargs=engine_kwargs)` so execution reaches line 283 where
`create_engine(sqlalchemy_uri, future=True, **engine_kwargs)` is invoked.
4. Observe Python raising `TypeError: create_engine() got multiple values
for keyword
argument 'future'` because `future` is passed both explicitly and via
`**engine_kwargs` at
`superset/cli/test_db.py:283`, aborting the dialect test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9279c1d0a81542d2abbeea95357c6974&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=9279c1d0a81542d2abbeea95357c6974&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/cli/test_db.py
**Line:** 283:283
**Comment:**
*Possible Bug: This call can fail with `TypeError: got multiple values
for keyword argument` if `engine_kwargs` already includes `future`, which is
plausible since this function accepts externally supplied engine options. Merge
defaults safely instead of passing the same keyword both explicitly and via
`**engine_kwargs`.
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%2F42277&comment_hash=970a5700f1e0792d04a406b150294a3faadf6145892c2b6b1c4a4295ea4995a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42277&comment_hash=970a5700f1e0792d04a406b150294a3faadf6145892c2b6b1c4a4295ea4995a3&reaction=dislike'>👎</a>
##########
superset/models/core.py:
##########
@@ -644,7 +644,7 @@ def _get_sqla_engine( # pylint: disable=too-many-locals #
noqa: C901
if cached := _ENGINE_CACHE.get(cache_key):
return cached
try:
- engine = create_engine(sqlalchemy_url, **engine_kwargs)
+ engine = create_engine(sqlalchemy_url, future=True,
**engine_kwargs)
Review Comment:
**Suggestion:** Adding `future=True` directly while also expanding
`**engine_kwargs` can raise a runtime `TypeError` when `engine_kwargs` already
contains a `future` key (for example from user-provided engine options), which
will prevent engine creation for those existing configurations. Ensure `future`
is injected only when absent, or normalize/remove duplicate keys before calling
`create_engine`. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Database engine creation fails for configs including `future`.
- ❌ Queries and charts cannot access affected database engines.
- ⚠️ Migration and CLI tasks using Database engine fail.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a Superset Python shell, obtain a `Database` model instance whose
`_get_sqla_engine`
method is defined in `superset/models/core.py` around line 634.
2. Prepare `engine_kwargs` to include a `future` key, e.g. `engine_kwargs =
{"future":
True}`, which is realistic since `engine_kwargs` are intended to carry
user/config-supplied engine options (e.g. from `SQLALCHEMY_ENGINE_OPTIONS`).
3. Call the internal engine creation routine so that
`_get_sqla_engine(sqlalchemy_url,
schema=None, source=None, cacheable=False, engine_kwargs=engine_kwargs)` is
executed and
reaches line 647: `engine = create_engine(sqlalchemy_url, future=True,
**engine_kwargs)`.
4. Observe a runtime `TypeError: create_engine() got multiple values for
keyword argument
'future'` raised at `superset/models/core.py:647`, preventing engine
creation and causing
any operation depending on this database connection (queries, migrations,
dashboards) to
fail.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=969d76456fd944059dc298b672497e70&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=969d76456fd944059dc298b672497e70&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:** 647:647
**Comment:**
*Possible Bug: Adding `future=True` directly while also expanding
`**engine_kwargs` can raise a runtime `TypeError` when `engine_kwargs` already
contains a `future` key (for example from user-provided engine options), which
will prevent engine creation for those existing configurations. Ensure `future`
is injected only when absent, or normalize/remove duplicate keys before calling
`create_engine`.
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%2F42277&comment_hash=4a809a0de5c8ce05568961f72869a6d7b253de76f7422c53f32583ddf97d3bac&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42277&comment_hash=4a809a0de5c8ce05568961f72869a6d7b253de76f7422c53f32583ddf97d3bac&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]
