codeant-ai-for-open-source[bot] commented on code in PR #28008:
URL: https://github.com/apache/superset/pull/28008#discussion_r3392418271
##########
tests/integration_tests/databases/commands_tests.py:
##########
@@ -395,6 +395,24 @@ def test_export_database_command_no_related(self, mock_g):
assert "databases" in prefixes
assert "datasets" not in prefixes
+ @patch("superset.security.manager.g")
+ def test_export_database_command_unicode_chars(self, mock_g):
+ mock_g.user = security_manager.find_user("admin")
+ db.session.query(Database).filter_by(database_name="中文").delete()
+ command = CreateDatabaseCommand(
+ {"database_name": "中文", "sqlalchemy_uri": "sqlite:///:memory:"},
+ )
+ example_db = command.run()
+
+ command = ExportDatabasesCommand([example_db.id], export_related=False)
+ contents = dict(command.run())
+
+ path = f"databases/{example_db.id}.yaml"
+ assert path in set(contents.keys())
+ yaml_content = contents[path]()
+ assert "database_name: 中文" in yaml_content
+ db.session.query(Database).filter_by(database_name="中文").delete()
Review Comment:
**Suggestion:** The test deletes the temporary `中文` database at the end but
never commits that deletion. Since creation is committed inside
`CreateDatabaseCommand.run()`, the row can remain in the database and leak into
later tests. Commit after the delete (or wrap creation/deletion in a guaranteed
cleanup transaction). [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Temporary "中文" database can remain committed during test session.
- ⚠️ Other tests using separate sessions may see unexpected extra database.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test
`TestExportDatabasesCommand.test_export_database_command_unicode_chars` in
`tests/integration_tests/databases/commands_tests.py:99-115`, which uses the
SQLAlchemy
session `superset.db.session`.
2. At the start of the test, any existing `"中文"` databases are removed via
`db.session.query(Database).filter_by(database_name="中文").delete()` at line
401, and then
`CreateDatabaseCommand({"database_name": "中文", ...}).run()` is called at
403-106.
3. `CreateDatabaseCommand.run()` in
`superset/commands/database/create.py:52-68` is
decorated with `@transaction(...)`, and the `transaction` decorator is
verified by
`tests/unit_tests/utils/test_decorators.py:300-312` to call
`db.session.commit()` on
success, so the new `"中文"` `Database` row is fully committed to the metadata
database.
4. At the end of the test, cleanup is attempted with
`db.session.query(Database).filter_by(database_name="中文").delete()` at
`tests/integration_tests/databases/commands_tests.py:414`, but there is no
subsequent
`db.session.commit()` in this test; until some unrelated code later calls
`db.session.commit()` (for example
`TestImportDatabasesCommand.test_import_v1_database` at
87-88 or the session-level `setup_sample_data` teardown commit in
`tests/integration_tests/conftest.py:136-145), the committed `"中文"` database
remains in
the underlying store and can be observed by any code using a separate
session/connection,
meaning this test's temporary database is not reliably cleaned up in
isolation.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c14123edc90544bdbf175fc8ad8e5cae&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c14123edc90544bdbf175fc8ad8e5cae&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/databases/commands_tests.py
**Line:** 414:414
**Comment:**
*Missing Cleanup: The test deletes the temporary `中文` database at the
end but never commits that deletion. Since creation is committed inside
`CreateDatabaseCommand.run()`, the row can remain in the database and leak into
later tests. Commit after the delete (or wrap creation/deletion in a guaranteed
cleanup transaction).
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%2F28008&comment_hash=69242c7379494e505803afd98366c59f4f3883021485dafb061959c28c70b459&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F28008&comment_hash=69242c7379494e505803afd98366c59f4f3883021485dafb061959c28c70b459&reaction=dislike'>👎</a>
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -177,6 +177,30 @@ def test_export_chart_command_no_related(self, mock_g):
]
assert expected == list(contents.keys())
+ @patch("superset.security.manager.g")
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ def test_export_chart_command_unicode_chars(self, mock_g):
+ """Test that they keys in the YAML have the same order as
export_fields"""
+ mock_g.user = security_manager.find_user("admin")
+ db.session.query(Slice).filter_by(slice_name="Energy Sankey").update(
+ {"slice_name": "中文"},
+ )
+ example_chart =
db.session.query(Slice).filter_by(slice_name="中文").one()
+
+ command = ExportChartsCommand([example_chart.id])
+ contents = dict(command.run())
+
+ path = f"charts/{example_chart.id}.yaml"
+ assert path in set(contents.keys())
+ yaml_content = contents[path]()
+ metadata = yaml.safe_load(yaml_content)
+ assert metadata["slice_name"] == "中文"
+ assert "slice_name: 中文" in yaml_content
+ # make sure the tearDown method works well
+ db.session.query(Slice).filter_by(slice_name="中文").update(
+ {"slice_name": "Energy Sankey"},
+ )
Review Comment:
**Suggestion:** The state reset for the renamed chart is placed after
assertions instead of in a guaranteed cleanup path. If any assertion fails, the
chart name remains changed and fixture teardown can fail because cleanup
expects `Energy Sankey` to exist. Move the rename-back logic into a `finally`
block so cleanup always runs. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Failing chart export test leaves renamed slice in metadata.
- ⚠️ Later energy-dashboard fixtures can accumulate unexpected duplicate
slices.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test
`TestExportChartsCommand.test_export_chart_command_unicode_chars` defined
in `tests/integration_tests/charts/commands_tests.py:41-63`, which is marked
with
`@pytest.mark.usefixtures("load_energy_table_with_slice")`.
2. The `load_energy_table_with_slice` fixture in
`tests/integration_tests/fixtures/energy_dashboard.py:8-13` calls
`_create_energy_table()`
(lines 21-40, 54-66) to create and COMMIT slices including one named
`"Energy Sankey"`,
then yields control and later calls `_cleanup()` on teardown.
3. Inside the test, the slice `"Energy Sankey"` is renamed to `"中文"` via
`db.session.query(Slice).filter_by(slice_name="Energy
Sankey").update({"slice_name":
"中文"})` at `charts/commands_tests.py:45-48`; the name is only restored back
to `"Energy
Sankey"` by the `update(...)` block at lines 60-63 (197-202) if all
preceding assertions
at 197-199 succeed.
4. If any assertion in this test fails (for example `assert "slice_name: 中文"
in
yaml_content` at 198 when a regression reintroduces escaping), the restore
`update()` at
200-202 is never executed; pytest still invokes the fixture teardown
`_cleanup()`
(energy_dashboard.py:23-38), which tries to delete slices by the original
`"Energy
Sankey"` title and thus leaves the renamed `"中文"` slice in the metadata
database,
polluting state for any later tests that rely on
`load_energy_table_with_slice`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1d19ce3f46b646ed887c7942c7cefd75&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1d19ce3f46b646ed887c7942c7cefd75&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/commands_tests.py
**Line:** 197:202
**Comment:**
*Missing Cleanup: The state reset for the renamed chart is placed after
assertions instead of in a guaranteed cleanup path. If any assertion fails, the
chart name remains changed and fixture teardown can fail because cleanup
expects `Energy Sankey` to exist. Move the rename-back logic into a `finally`
block so cleanup always runs.
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%2F28008&comment_hash=bcc7207e697ced871635d16f8dc2b64a849d98cce61ec9c9fbfae840d87e2185&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F28008&comment_hash=bcc7207e697ced871635d16f8dc2b64a849d98cce61ec9c9fbfae840d87e2185&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]
