You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hy144328 (via GitHub)" <gi...@apache.org> on 2026/05/19 23:21:12 UTC
[PR] refactor: update Connection.execute() to use queries with text() [superset]
hy144328 opened a new pull request, #40277:
URL: https://github.com/apache/superset/pull/40277
See #40273 .
Depends on #40274 .
### SUMMARY
Enable errors for deprecated Connection.execute() syntax, and fix them all.
In addition to fixing all broken tests, I manually went through all results of `git grep execute(`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
```
$ SQLALCHEMY_WARN_20=1 TZ=UTC python3 -m pytest tests/unit_tests/
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434320364
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4741520140
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434284014
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
+ error:Passing a string to Connection.execute\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:"Query" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SavedQuery" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlaTable" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlMetric" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TableColumn" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TaggedObject" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``as_declarative\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The autoload parameter is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The connection.execute\(\) method:sqlalchemy.exc.RemovedIn20Warning
+# error:The current statement is being autocommitted using implicit autocommit:sqlalchemy.exc.RemovedIn20Warning
+# error:The `database` package is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``declarative_base\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The Engine.execute\(\) method is considered legacy:sqlalchemy.exc.RemovedIn20Warning
+# error:The legacy calling style of select\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The "whens" argument to case:sqlalchemy.exc.RemovedIn20Warning
+# error:"User" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
Review Comment:
See https://github.com/apache/superset/pull/40274#discussion_r3284491199 .
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4747528039
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434271059
##########
tests/integration_tests/conftest.py:
##########
@@ -213,12 +214,12 @@ def setup_presto_if_needed():
database = get_example_database()
with database.get_sqla_engine() as engine:
drop_from_schema(engine, CTAS_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}"))
drop_from_schema(engine, ADMIN_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}"))
Review Comment:
Beyond the scope of this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4746778393
[9f8e279](https://github.com/apache/superset/pull/40277/commits/9f8e2799b09bfc836ef69c71df2e2728749619ef) fixes broken integration tests, which I were not able to run locally.
Otherwise, I think the PR is good to go, too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4774088463
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434317498
##########
superset/migrations/versions/2018-03-30_14-00_d6ffdf31bdd4_add_published_column_to_dashboards.py:
##########
@@ -33,7 +33,7 @@
def upgrade():
with op.batch_alter_table("dashboards") as batch_op:
batch_op.add_column(sa.Column("published", sa.Boolean(), nullable=True))
- op.execute("UPDATE dashboards SET published='1'")
+ op.execute(sa.text("UPDATE dashboards SET published='1'"))
Review Comment:
**Suggestion:** Add a return type annotation to the modified migration function so the updated function is fully typed (for example, annotate `upgrade` with `-> None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The migration file is being modified and the `upgrade` function remains untyped, with no return annotation. This matches the custom rule requiring newly added or modified Python functions to be fully typed when surrounding code is changed, so the suggestion is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6c1260bac1eb4d38a7c75100204970dd&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=6c1260bac1eb4d38a7c75100204970dd&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/migrations/versions/2018-03-30_14-00_d6ffdf31bdd4_add_published_column_to_dashboards.py
**Line:** 33:37
**Comment:**
*Custom Rule: Add a return type annotation to the modified migration function so the updated function is fully typed (for example, annotate `upgrade` with `-> None`).
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%2F40277&comment_hash=b1b0b0e533efe899984d3a6f5298b1ec612d1fcaf32b664e9a27ac813a8dead7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40277&comment_hash=b1b0b0e533efe899984d3a6f5298b1ec612d1fcaf32b664e9a27ac813a8dead7&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434272028
##########
tests/integration_tests/conftest.py:
##########
@@ -213,12 +214,12 @@ def setup_presto_if_needed():
database = get_example_database()
with database.get_sqla_engine() as engine:
drop_from_schema(engine, CTAS_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}"))
drop_from_schema(engine, ADMIN_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}"))
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3432645307
##########
tests/integration_tests/conftest.py:
##########
@@ -213,12 +214,12 @@ def setup_presto_if_needed():
database = get_example_database()
with database.get_sqla_engine() as engine:
drop_from_schema(engine, CTAS_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}"))
drop_from_schema(engine, ADMIN_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate with_feature_flags decorator</b></div>
<div id="fix">
The `with_feature_flags` decorator implementation is duplicated across integration and unit test conftest files. Consider centralizing this utility in a shared location to maintain a single source of truth for feature flag mocking behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -1,38 +1,8 @@
-# Duplicated decorator in tests/integration_tests/conftest.py (lines 222-257)
-# and tests/unit_tests/conftest.py (lines 182-217)
-
-def with_feature_flags(**mock_feature_flags):
- """
- Use this decorator to mock feature flags in tests.integration_tests.
-
- Usage:
-
- class TestYourFeature(SupersetTestCase):
-
- @with_feature_flags(YOUR_FEATURE=True)
- def test_your_feature_enabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), True)
-
- @with_feature_flags(YOUR_FEATURE=False)
- def test_your_feature_disabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), False)
- """
-
- def mock_get_feature_flags():
- feature_flags = feature_flag_manager._feature_flags or {}
- return {**feature_flags, **mock_feature_flags}
-
- def decorate(test_fn):
- def wrapper(*args, **kwargs):
- with patch.object(
- feature_flag_manager,
- "get_feature_flags",
- side_effect=mock_get_feature_flags,
- ):
- test_fn(*args, **kwargs)
-
- return functools.update_wrapper(wrapper, test_fn)
-
- return decorate
+from tests.shared.feature_flag_helpers import with_feature_flags
+
+__all__ = [
+ "with_feature_flags",
+ # ... other exports
+]
```
</div>
</details>
</div>
<div><details><summary><b>Review Rule</b></summary><div>Bito created the rule <strong><code>Extract duplicated test utilities into shared modules</code></strong> for <strong>repo:</strong> <code>apache/superset</code>, <strong>language:</strong> <code>Python</code>. Bito will avoid suggestions that match this rule. You can manage review rules <a href=https://alpha.bito.ai/home/ai-agents/review-rules>here</a>.</div></details></div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434273602
##########
superset/db_engine_specs/databricks.py:
##########
@@ -746,7 +746,7 @@ def get_catalog_names(
database: Database,
inspector: Inspector,
) -> set[str]:
- return {catalog for (catalog,) in inspector.bind.execute("SHOW CATALOGS")}
+ return {catalog for (catalog,) in inspector.bind.execute(text("SHOW CATALOGS"))}
Review Comment:
Beyond the scope of this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434324076
##########
superset/migrations/versions/2018-03-30_14-00_d6ffdf31bdd4_add_published_column_to_dashboards.py:
##########
@@ -33,7 +33,7 @@
def upgrade():
with op.batch_alter_table("dashboards") as batch_op:
batch_op.add_column(sa.Column("published", sa.Boolean(), nullable=True))
- op.execute("UPDATE dashboards SET published='1'")
+ op.execute(sa.text("UPDATE dashboards SET published='1'"))
Review Comment:
Beyond the scope of this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4501105527
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40277?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `48.00000%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.31%. Comparing base ([`8d2b655`](https://app.codecov.io/gh/apache/superset/commit/8d2b655c22d8e678b25f8be1cea8602523bd6f90?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4bf3630`](https://app.codecov.io/gh/apache/superset/commit/4bf3630957d64bc2e8fe4bcd7c1cb7ebc970bcad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 90 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40277?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/databricks.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fdatabricks.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFicmlja3MucHk=) | 20.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/starrocks.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fstarrocks.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3N0YXJyb2Nrcy5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/mock\_data.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Futils%2Fmock_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/doris.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fdoris.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RvcmlzLnB5) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/gsheets.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fgsheets.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2dzaGVldHMucHk=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/hive.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fhive.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/presto.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fpresto.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/encrypt.py](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&filepath=superset%2Futils%2Fencrypt.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvZW5jcnlwdC5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40277?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (8d2b655) and HEAD (4bf3630). Click for more details.
>
> <details><summary>HEAD has 182 uploads less than BASE</summary>
>
>| Flag | BASE (8d2b655) | HEAD (4bf3630) |
>|------|------|------|
>|python|168|1|
>|presto|16|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40277 +/- ##
==========================================
- Coverage 64.16% 55.31% -8.85%
==========================================
Files 2591 2591
Lines 138162 138226 +64
Branches 32048 32059 +11
==========================================
- Hits 88647 76463 -12184
- Misses 47986 61098 +13112
+ Partials 1529 665 -864
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.13% <48.00%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.13% <48.00%> (-19.54%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40277/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40277?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3435728791
##########
tests/integration_tests/sqllab_tests.py:
##########
@@ -184,18 +185,18 @@ def test_sql_json_cta_dynamic_db(self, ctas_method: CTASMethod) -> None:
examples_db = get_example_database()
with examples_db.get_sqla_engine() as engine:
data = engine.execute(
- f"SELECT * FROM admin_database.{tmp_table_name}" # noqa: S608
+ text(f"SELECT * FROM admin_database.{tmp_table_name}") # noqa: S608
).fetchall()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>SQLAlchemy engine.execute deprecated</b></div>
<div id="fix">
`engine.execute()` is deprecated in SQLAlchemy 1.4 (connectionless execution). Use `with engine.connect() as conn: conn.execute(text(...))` instead. This applies to all 5 changed `engine.execute()` calls in this diff.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- tests/integration_tests/sqllab_tests.py (lines 186-202) ---
186: with examples_db.get_sqla_engine() as engine:
187: - data = engine.execute(
188: - text(f"SELECT * FROM admin_database.{tmp_table_name}") # noqa: S608
189: - ).fetchall()
190: - names_count = engine.execute(
191: - text(f"SELECT COUNT(*) FROM birth_names") # noqa: F541, S608
192: - ).first()
193: + with engine.connect() as conn:
194: + data = conn.execute(
195: + text(f"SELECT * FROM admin_database.{tmp_table_name}") # noqa: S608
196: + ).fetchall()
197: + names_count = conn.execute(
198: + text(f"SELECT COUNT(*) FROM birth_names") # noqa: F541, S608
199: + ).first()
200: assert names_count[0] == len(
201: data
202: ) # SQL_MAX_ROW not applied due to the SQLLAB_CTAS_NO_LIMIT set to True
203:
204: # cleanup
205: - engine.execute(
206: - text(f"DROP {ctas_method.name} admin_database.{tmp_table_name}")
207: - )
208: + with engine.connect() as conn:
209: + conn.execute(
210: + text(f"DROP {ctas_method.name} admin_database.{tmp_table_name}")
211: + )
212: examples_db.allow_ctas = old_allow_ctas
213: db.session.commit()
--- tests/integration_tests/sqllab_tests.py (lines 296-302) ---
296: with examples_db.get_sqla_engine() as engine:
297: - engine.execute(
298: - text(f"""
299: - CREATE TABLE IF NOT EXISTS {CTAS_SCHEMA_NAME}.test_table AS
300: - SELECT 1 as c1, 2 as c2
301: - """) # noqa: E501
302: - )
303: + with engine.connect() as conn:
304: + conn.execute(
305: + text(f"""
306: + CREATE TABLE IF NOT EXISTS {CTAS_SCHEMA_NAME}.test_table AS
307: + SELECT 1 as c1, 2 as c2
308: + """) # noqa: E501
309: + )
--- tests/integration_tests/sqllab_tests.py (lines 320-322) ---
320: db.session.query(Query).delete()
321: with get_example_database().get_sqla_engine() as engine:
322: - engine.execute(text(f"DROP TABLE IF EXISTS {CTAS_SCHEMA_NAME}.test_table"))
323: + with engine.connect() as conn:
324: + conn.execute(text(f"DROP TABLE IF EXISTS {CTAS_SCHEMA_NAME}.test_table"))
325: db.session.commit()
```
</div>
</details>
</div>
<small><i>Code Review Run #3d1d0b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
The `reset_postgres_id_sequence` function is dead code — it is defined but never called. The `text` import added at line 32 is only used by this unused function. Both should be removed to avoid maintenance confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++ superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -29,7 +29,7 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import select, text
+from sqlalchemy import select
from sqlalchemy.exc import NoSuchModuleError
from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy.orm import backref, relationship, Session
@@ -889,16 +889,6 @@
from sqlalchemy.orm import aliased
-def reset_postgres_id_sequence(table: str) -> None:
- """Reset PostgreSQL sequence ID for a table's id column."""
- op.execute(
- text("""
- SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-89: SQL Parameter Binding Table Name</b></div>
<div id="fix">
The `text()` wrapper alone does not fix the fundamental issue: `:table` bind parameters cannot be used for table names in FROM clauses in SQLAlchemy. The parameter binding only works for VALUES, not identifiers like table names. This migration will fail at runtime on PostgreSQL with an error like "can't adapt type 'str'" or similar. Use f-string interpolation with quoted identifiers instead. (See also: [CWE-89](https://cwe.mitre.org/data/definitions/89.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++ b/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -892,12 +892,10 @@ def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- text("""
+ text(f"""
SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )+ pg_get_serial_sequence('{table}', 'id'),
+ COALESCE(max(id) + 1, 1),
+ false
+ )
+ FROM \"{table}\";
+ """),
+ )
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-89: SQL Identifier Binding Error</b></div>
<div id="fix">
PostgreSQL does not allow table names as bind parameters. Instead of `FROM :table`, build the SQL with the table name interpolated as an identifier. For example, use SQLAlchemy's `quoted_name` or `identifier` helpers and something like:
```python
sql = text(f"""
SELECT setval(
pg_get_serial_sequence('{table}', 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM {quoted_name(table)};
""")
op.execute(sql)
```
This ensures the table name is a literal identifier and avoids runtime errors. (See also: [CWE-89](https://cwe.mitre.org/data/definitions/89.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- tests/integration_tests/db_engine_specs/hive_tests.py (lines 201, 229) ---
201: from sqlalchemy import text
201: mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS {table_name}"))
229: mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS {schema}.{table_name}"))
```
</div>
</details>
</div>
<small><i>Code Review Run #3d1d0b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40277:
URL: https://github.com/apache/superset/pull/40277
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434285417
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
+ error:Passing a string to Connection.execute\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:"Query" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SavedQuery" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlaTable" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlMetric" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TableColumn" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TaggedObject" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``as_declarative\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The autoload parameter is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The connection.execute\(\) method:sqlalchemy.exc.RemovedIn20Warning
+# error:The current statement is being autocommitted using implicit autocommit:sqlalchemy.exc.RemovedIn20Warning
+# error:The `database` package is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``declarative_base\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The Engine.execute\(\) method is considered legacy:sqlalchemy.exc.RemovedIn20Warning
+# error:The legacy calling style of select\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The "whens" argument to case:sqlalchemy.exc.RemovedIn20Warning
+# error:"User" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate to address. The commented-out rules in the configuration file are currently inactive and do not provide functional value, which can lead to confusion regarding which warnings should be treated as errors. Removing or activating these rules improves the maintainability and clarity of the configuration.
**pytest.ini**
```
# error:"Query" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
# error:"SavedQuery" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3436945363
##########
tests/integration_tests/sqllab_tests.py:
##########
@@ -184,18 +185,18 @@ def test_sql_json_cta_dynamic_db(self, ctas_method: CTASMethod) -> None:
examples_db = get_example_database()
with examples_db.get_sqla_engine() as engine:
data = engine.execute(
- f"SELECT * FROM admin_database.{tmp_table_name}" # noqa: S608
+ text(f"SELECT * FROM admin_database.{tmp_table_name}") # noqa: S608
).fetchall()
Review Comment:
This warning will be handled in another PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4772912837
> LGTM! One tiny thing before I merge: the `assert_*_with_text` helper is copy-pasted into presto/doris/trino/hive test files. Might be worth pulling into a shared test util so we're not maintaining four copies. Happy to take it as a follow-up rather than block on it, or you can tackle it here if you have the time.
@rusackas Thanks for the feedback. I have learnt about [DAMP](https://abseil.io/resources/swe-book/html/ch12.html#shared_helpers_and_validation) tests the other day. But DRY is fine by me, too.
[5859ecb](https://github.com/apache/superset/pull/40277/commits/5859ecb8fbba30a250df3fec76a75673a96f90c1) places the assertion helpers into a shared module. Hope this will do.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4773707532
Thanks again! This is all coming along nicely :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3270352189
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -407,7 +408,7 @@ def validate_parameters(
try:
url = url.replace('"', '""')
- results = conn.execute(f'SELECT * FROM "{url}" LIMIT 1') # noqa: S608
+ results = conn.execute(text(f'SELECT * FROM "{url}" LIMIT 1')) # noqa: S608
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Loop variable overwritten by assignment</b></div>
<div id="fix">
The loop variable `url` is overwritten at line 410 within the loop body. Use a different variable name (e.g., `escaped_url`) to avoid shadowing the loop variable.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
try:
escaped_url = url.replace('"', '""')
results = conn.execute(text(f'SELECT * FROM "{escaped_url}" LIMIT 1')) # noqa: S608
````
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>filterwarnings order prevents always directive</b></div>
<div id="fix">
The bare `ignore` directive on line 29 will suppress all warnings before the `always::sqlalchemy.exc.RemovedIn20Warning` directive on line 30 can match them. Per pytest's filterwarnings behavior, the first matching directive wins. This means the 'always' directive is effectively dead code — RemovedIn20Warning messages will be silently ignored instead of being printed.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- pytest.ini (lines 25-31) ---
25: +# `ignore` virtually reproduces to `-p no:warnings`.
26: +# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
27: +# Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression.
28: +filterwarnings =
29: + always::sqlalchemy.exc.RemovedIn20Warning
30: + ignore
31: + error:Passing a string to Connection.execute\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
The `reset_postgres_id_sequence` function is dead code — it is defined but never called. The `text` import added at line 32 is only used by this unused function. Both should be removed to avoid maintenance confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++ superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -29,7 +29,7 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import select, text
+from sqlalchemy import select
from sqlalchemy.exc import NoSuchModuleError
from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy.orm import backref, relationship, Session
@@ -889,16 +889,6 @@
from sqlalchemy.orm import aliased
-def reset_postgres_id_sequence(table: str) -> None:
- """Reset PostgreSQL sequence ID for a table's id column."""
- op.execute(
- text("""
- SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4736212141
Rebased and force-pushed e63e7cc in order to resolve merge conflict.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434319054
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
Beyond the scope of this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434274290
##########
superset/db_engine_specs/databricks.py:
##########
@@ -746,7 +746,7 @@ def get_catalog_names(
database: Database,
inspector: Inspector,
) -> set[str]:
- return {catalog for (catalog,) in inspector.bind.execute("SHOW CATALOGS")}
+ return {catalog for (catalog,) in inspector.bind.execute(text("SHOW CATALOGS"))}
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3432645313
##########
superset/db_engine_specs/databricks.py:
##########
@@ -746,7 +746,7 @@ def get_catalog_names(
database: Database,
inspector: Inspector,
) -> set[str]:
- return {catalog for (catalog,) in inspector.bind.execute("SHOW CATALOGS")}
+ return {catalog for (catalog,) in inspector.bind.execute(text("SHOW CATALOGS"))}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate adjust_engine_params method</b></div>
<div id="fix">
The `adjust_engine_params` method contains duplicate logic across Databricks and Presto engine specs for handling catalog and schema parameters. Consider refactoring into a shared base class or utility to reduce duplication and ensure consistent behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -1,15 +1,10 @@
-# Duplicated in both databricks.py (lines 749-759) and presto.py (lines 329-339)
-
- return {catalog for (catalog,) in inspector.bind.execute(text("SHOW CATALOGS"))}
-
- @classmethod
- def adjust_engine_params(
- cls,
- uri: URL,
- connect_args: dict[str, Any],
- catalog: str | None = None,
- schema: str | None = None,
- ) -> tuple[URL, dict[str, Any]]:
- if catalog:
+ @classmethod
+ def adjust_engine_params(
+ cls,
+ uri: URL,
+ connect_args: dict[str, Any],
+ catalog: str | None = None,
+ schema: str | None = None,
+ ) -> tuple[URL, dict[str, Any]]:
+ return cls._adjust_engine_params_with_catalog(uri, connect_args, catalog, schema)
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/db_engine_specs/hive.py:
##########
@@ -234,7 +234,7 @@ def df_to_sql(
catalog=table.catalog,
schema=table.schema,
) as engine:
- engine.execute(f"DROP TABLE IF EXISTS {str(table)}")
+ engine.execute(text(f"DROP TABLE IF EXISTS {str(table)}"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertion mismatch</b></div>
<div id="fix">
In `tests/integration_tests/db_engine_specs/hive_tests.py`, import `text` from SQLAlchemy and change the two `mock_execute.assert_any_call(f"DROP TABLE IF EXISTS …")` assertions to `mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS …"))` so they match the code change.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/db_engine_specs/hive.py
+++ b/superset/db_engine_specs/hive.py
@@ -237 +237 @@
-mock_execute.assert_any_call(f"DROP TABLE IF EXISTS {table_name}")
+mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS {table_name}"))
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
The `reset_postgres_id_sequence` function is dead code — it is defined but never called. The `text` import added at line 32 is only used by this unused function. Both should be removed to avoid maintenance confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++ superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -29,7 +29,7 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import select, text
+from sqlalchemy import select
from sqlalchemy.exc import NoSuchModuleError
from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy.orm import backref, relationship, Session
@@ -889,16 +889,6 @@
from sqlalchemy.orm import aliased
-def reset_postgres_id_sequence(table: str) -> None:
- """Reset PostgreSQL sequence ID for a table's id column."""
- op.execute(
- text("""
- SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-89: SQL Parameter Binding Table Name</b></div>
<div id="fix">
The `text()` wrapper alone does not fix the fundamental issue: `:table` bind parameters cannot be used for table names in FROM clauses in SQLAlchemy. The parameter binding only works for VALUES, not identifiers like table names. This migration will fail at runtime on PostgreSQL with an error like "can't adapt type 'str'" or similar. Use f-string interpolation with quoted identifiers instead. (See also: [CWE-89](https://cwe.mitre.org/data/definitions/89.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++ b/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -892,12 +892,10 @@ def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- text("""
+ text(f"""
SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )+ pg_get_serial_sequence('{table}', 'id'),
+ COALESCE(max(id) + 1, 1),
+ false
+ )
+ FROM \"{table}\";
+ """),
+ )
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/integration_tests/conftest.py:
##########
@@ -213,12 +214,12 @@ def setup_presto_if_needed():
database = get_example_database()
with database.get_sqla_engine() as engine:
drop_from_schema(engine, CTAS_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}"))
drop_from_schema(engine, ADMIN_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate with_feature_flags decorator</b></div>
<div id="fix">
The `with_feature_flags` decorator implementation is duplicated across integration and unit test conftest files. Consider centralizing this utility in a shared location to maintain a single source of truth for feature flag mocking behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -1,38 +1,8 @@
-# Duplicated decorator in tests/integration_tests/conftest.py (lines 222-257)
-# and tests/unit_tests/conftest.py (lines 182-217)
-
-def with_feature_flags(**mock_feature_flags):
- """
- Use this decorator to mock feature flags in tests.integration_tests.
-
- Usage:
-
- class TestYourFeature(SupersetTestCase):
-
- @with_feature_flags(YOUR_FEATURE=True)
- def test_your_feature_enabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), True)
-
- @with_feature_flags(YOUR_FEATURE=False)
- def test_your_feature_disabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), False)
- """
-
- def mock_get_feature_flags():
- feature_flags = feature_flag_manager._feature_flags or {}
- return {**feature_flags, **mock_feature_flags}
-
- def decorate(test_fn):
- def wrapper(*args, **kwargs):
- with patch.object(
- feature_flag_manager,
- "get_feature_flags",
- side_effect=mock_get_feature_flags,
- ):
- test_fn(*args, **kwargs)
-
- return functools.update_wrapper(wrapper, test_fn)
-
- return decorate
+from tests.shared.feature_flag_helpers import with_feature_flags
+
+__all__ = [
+ "with_feature_flags",
+ # ... other exports
+]
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned `filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
+ error:Passing a string to Connection.execute\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:"Query" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SavedQuery" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlaTable" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlMetric" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TableColumn" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TaggedObject" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``as_declarative\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The autoload parameter is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The connection.execute\(\) method:sqlalchemy.exc.RemovedIn20Warning
+# error:The current statement is being autocommitted using implicit autocommit:sqlalchemy.exc.RemovedIn20Warning
+# error:The `database` package is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``declarative_base\(\)`` function is now available:sqlalchemy.exc.RemovedIn20Warning
+# error:The Engine.execute\(\) method is considered legacy:sqlalchemy.exc.RemovedIn20Warning
+# error:The legacy calling style of select\(\) is deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The "whens" argument to case:sqlalchemy.exc.RemovedIn20Warning
+# error:"User" object is being merged into a Session:sqlalchemy.exc.RemovedIn20Warning
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code: commented-out rules</b></div>
<div id="fix">
Dead code: 16 commented-out `error:` rules added in commit 13cef4fd84 remain unused. These create confusion about which warnings should be converted to errors and add maintenance burden. Either activate them (uncomment) or remove them entirely.
</div>
</div>
<small><i>Code Review Run #d38cb5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4737133700
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434317588
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -407,7 +408,7 @@ def validate_parameters(
try:
url = url.replace('"', '""')
- results = conn.execute(f'SELECT * FROM "{url}" LIMIT 1') # noqa: S608
+ results = conn.execute(text(f'SELECT * FROM "{url}" LIMIT 1')) # noqa: S608
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3434316979
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -407,7 +408,7 @@ def validate_parameters(
try:
url = url.replace('"', '""')
- results = conn.execute(f'SELECT * FROM "{url}" LIMIT 1') # noqa: S608
+ results = conn.execute(text(f'SELECT * FROM "{url}" LIMIT 1')) # noqa: S608
Review Comment:
Beyond the scope of this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4493007936
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 4bf3630957d64bc2e8fe4bcd7c1cb7ebc970bcad |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a0cf0692871c5000805388b |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40277--superset-docs-preview.netlify.app](https://deploy-preview-40277--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#HcxRDoIwDADQu_TbxaoBpzfwGKUUIdnWWWoCMd7dxb_39T5QTXN1uAMcYDAqPDebTMSudlxfiRLPkveQl6eRL1oCaynCf8om_HYJLpu3YJSadH-MregJecL-do7XE3eIGLG7xDjA9wc) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40277--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: update Connection.execute() to use queries with text() [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40277:
URL: https://github.com/apache/superset/pull/40277#issuecomment-4492985792
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org