codeant-ai-for-open-source[bot] commented on code in PR #37120:
URL: https://github.com/apache/superset/pull/37120#discussion_r3609540774
##########
tests/integration_tests/dashboards/commands_tests.py:
##########
@@ -533,6 +533,100 @@ def test_export_dashboard_command_unicode_chars(self,
mock_g1, mock_g2):
{"dashboard_title": "World Bank's Data"},
)
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @patch("superset.security.manager.g")
+ @patch("superset.views.base.g")
+ def test_export_dashboard_cross_database_charts(self, mock_g1, mock_g2):
+ """
+ Test that dashboards with charts from multiple databases export
correctly.
+ This reproduces issue #37113 where charts from different databases
were missing.
+ """
+ mock_g1.user = security_manager.find_user("admin")
+ mock_g2.user = security_manager.find_user("admin")
+
+ # Create a second database for testing
+ second_db = Database(database_name="test_db_2",
sqlalchemy_uri="sqlite://")
+ db.session.add(second_db)
+
+ # Create a dataset in the second database
+ second_dataset = SqlaTable(
+ table_name="second_dataset",
+ database=second_db,
+ database_id=second_db.id,
+ columns=[],
+ )
+ db.session.add(second_dataset)
+
+ # Create a chart using the second database's dataset
+ chart_from_second_db = Slice(
+ slice_name="Chart from Second Database",
+ datasource_type="table",
+ datasource_id=second_dataset.id,
+ datasource_name=second_dataset.table_name,
+ viz_type="bar",
+ params=json.dumps({"viz_type": "bar"}),
+ )
Review Comment:
**Suggestion:** The chart is created with `datasource_id` taken from
`second_dataset.id` before the dataset is flushed, so this value is still
`None` at construction time and the chart is not linked to the new dataset.
That causes export logic to skip the second dataset/database chain and makes
the test assert the wrong behavior. Flush the session before creating the chart
(or set the chartโs table relationship directly) so the datasource reference is
valid. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Cross-database dashboard export test never links chart dataset.
- โ Second databaseโs dataset not exported via chart relationship.
- โ ๏ธ Multi-database dashboard regression coverage remains incomplete.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Run the test `test_export_dashboard_cross_database_charts` in
`tests/integration_tests/dashboards/commands_tests.py` (around lines 37โ124
in the current
file). Inside this test, a second dataset is created and added to the
session at lines
52โ59 (PR hunk lines 552โ558): `second_dataset = SqlaTable(...);
db.session.add(second_dataset)` without any intervening `db.session.flush()`
or
`db.session.commit()`, so `second_dataset.id` remains `None` at this point.
2. Observe that immediately after adding the dataset, the chart is
instantiated at PR hunk
lines 561โ568 with `datasource_id=second_dataset.id` (see same file, lines
61โ69 in the
current version): `chart_from_second_db = Slice(...,
datasource_id=second_dataset.id,
...)`. Because the dataset has not yet been flushed, `second_dataset.id` is
still `None`,
so the new `Slice` object is constructed with `datasource_id=None`.
3. Inspect the Slice model in `superset/models/slice.py` (lines 53โ59 and
97โ105):
`datasource_id = Column(Integer)` and the `table` relationship is defined
with
`foreign_keys=[datasource_id]` and a `primaryjoin` on `Slice.datasource_id ==
SqlaTable.id` when `datasource_type == 'table'`. Since `datasource_id` was
persisted as
`NULL` for `chart_from_second_db`, `chart_from_second_db.table` will be
`None` even after
`db.session.commit()` at PR hunk line 582.
4. Follow the export path: `ExportDashboardsCommand._export` in
`superset/commands/dashboard/export.py` (lines 133โ139) collects `chart_ids
= [chart.id
for chart in model.slices]` and calls
`ExportChartsCommand(chart_ids).run(seen=seen)`. In
`superset/commands/chart/export.py` (lines 95โ110),
`ExportChartsCommand._export` checks
`if model.table and export_related:` and only then calls
`ExportDatasetsCommand([model.table.id]).run(seen=seen)`. Because
`chart_from_second_db.table` is `None` due to the `datasource_id=None` bug,
the dataset
and database for the second DB are never exported, so the assertions in the
test at PR
hunk lines 588โ621 that expect the second database and dataset YAML files to
be present
will fail to reflect the intended โchart linked to datasetโ scenario.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4316db8047146958d361dcb443912fb&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=c4316db8047146958d361dcb443912fb&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/dashboards/commands_tests.py
**Line:** 558:568
**Comment:**
*Incorrect Variable Usage: The chart is created with `datasource_id`
taken from `second_dataset.id` before the dataset is flushed, so this value is
still `None` at construction time and the chart is not linked to the new
dataset. That causes export logic to skip the second dataset/database chain and
makes the test assert the wrong behavior. Flush the session before creating the
chart (or set the chartโs table relationship directly) so the datasource
reference is valid.
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%2F37120&comment_hash=0a0994f11d5503ce2f333c80d4a7d5cac45c89cb26e35c2741e8986d8da8bf68&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37120&comment_hash=0a0994f11d5503ce2f333c80d4a7d5cac45c89cb26e35c2741e8986d8da8bf68&reaction=dislike'>๐</a>
##########
tests/integration_tests/dashboards/commands_tests.py:
##########
@@ -533,6 +533,100 @@ def test_export_dashboard_command_unicode_chars(self,
mock_g1, mock_g2):
{"dashboard_title": "World Bank's Data"},
)
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @patch("superset.security.manager.g")
+ @patch("superset.views.base.g")
+ def test_export_dashboard_cross_database_charts(self, mock_g1, mock_g2):
+ """
+ Test that dashboards with charts from multiple databases export
correctly.
+ This reproduces issue #37113 where charts from different databases
were missing.
+ """
+ mock_g1.user = security_manager.find_user("admin")
+ mock_g2.user = security_manager.find_user("admin")
+
+ # Create a second database for testing
+ second_db = Database(database_name="test_db_2",
sqlalchemy_uri="sqlite://")
+ db.session.add(second_db)
+
+ # Create a dataset in the second database
+ second_dataset = SqlaTable(
+ table_name="second_dataset",
+ database=second_db,
+ database_id=second_db.id,
+ columns=[],
+ )
+ db.session.add(second_dataset)
+
+ # Create a chart using the second database's dataset
+ chart_from_second_db = Slice(
+ slice_name="Chart from Second Database",
+ datasource_type="table",
+ datasource_id=second_dataset.id,
+ datasource_name=second_dataset.table_name,
+ viz_type="bar",
+ params=json.dumps({"viz_type": "bar"}),
+ )
+ db.session.add(chart_from_second_db)
+
+ # Get the example dashboard and add the new chart
+ example_dashboard = (
+ db.session.query(Dashboard).filter_by(slug="world_health").one()
+ )
+
+ # Store original charts count
+ original_charts_count = len(example_dashboard.slices)
+
+ # Add the new chart from different database to the dashboard
+ example_dashboard.slices.append(chart_from_second_db)
+ db.session.commit()
+
+ # Export the dashboard
+ command = ExportDashboardsCommand([example_dashboard.id])
+ contents = dict(command.run())
+
+ # Verify all databases are exported
+ db_files = [key for key in contents.keys() if
key.startswith("databases/")]
+ assert len(db_files) >= 2, f"Expected at least 2 database files, got
{db_files}"
+
+ # Verify the second database is included
+ assert "databases/test_db_2.yaml" in contents.keys(), (
+ f"Second database not found in export. Keys:
{list(contents.keys())}"
+ )
+
+ # Verify all charts are exported (original + new one)
+ chart_files = [key for key in contents.keys() if
key.startswith("charts/")]
+ assert len(chart_files) == original_charts_count + 1, (
+ f"Expected {original_charts_count + 1} charts, got
{len(chart_files)}"
+ )
+
+ # Verify the new chart from second database is included
+ chart_from_second_db_file = None
+ for key in chart_files:
+ if f"Chart_from_Second_Database_{chart_from_second_db.id}" in key:
+ chart_from_second_db_file = key
+ break
+
+ assert chart_from_second_db_file is not None, (
+ f"Chart from second database not found in export. "
+ f"Chart files: {chart_files}"
+ )
+
+ # Verify the dataset from second database is included
+ dataset_files = [key for key in contents.keys() if
key.startswith("datasets/")]
+ second_dataset_file = (
+ f"datasets/test_db_2/second_dataset_{second_dataset.id}.yaml"
+ )
+ assert second_dataset_file in contents.keys(), (
+ f"Second dataset not found. Dataset files: {dataset_files}"
+ )
+
+ # Clean up
+ example_dashboard.slices.remove(chart_from_second_db)
+ db.session.delete(chart_from_second_db)
+ db.session.delete(second_dataset)
+ db.session.delete(second_db)
+ db.session.commit()
Review Comment:
**Suggestion:** The cleanup is only executed at the end of the happy path,
so any assertion failure or exception before that point leaves the temporary
dashboard/chart/dataset/database records in the test DB and can make later
tests flaky. Move cleanup into a `finally` block to guarantee teardown on all
paths. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ Failing test can leave extra Database and Slice rows.
- โ ๏ธ Subsequent integration tests run against polluted database state.
- โ ๏ธ Debugging regressions complicated by residual test artifacts.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In `tests/integration_tests/dashboards/commands_tests.py`, inspect
`test_export_dashboard_cross_database_charts` (lines 37โ124 of the current
file). The test
creates a temporary `Database` (`second_db`), `SqlaTable`
(`second_dataset`), and `Slice`
(`chart_from_second_db`), then appends the chart to the existing
`example_dashboard` and
commits (PR hunk lines 548โ582).
2. Note that cleanup is implemented only at the end of the test body (PR
hunk lines
623โ628): `example_dashboard.slices.remove(chart_from_second_db);
db.session.delete(chart_from_second_db); db.session.delete(second_dataset);
db.session.delete(second_db); db.session.commit()`. There is no
`try`/`finally` around the
assertions at PR hunk lines 588โ621, so any assertion failure (for example,
if the
expected dataset file key is missing) raises `AssertionError` and prevents
the cleanup
block from running.
3. Confirm that the global integration test infrastructure does not roll
back database
state between tests: `SupersetTestCase.tearDown` in
`tests/integration_tests/base_tests.py` (lines 23โ25) only calls
`self.logout()` and does
not touch the database, and the session-scoped `setup_sample_data` fixture in
`tests/integration_tests/conftest.py` (lines 120โ147) only drops tables once
at the end of
the entire test session, not per test. This means any `second_db`,
`second_dataset`, and
`chart_from_second_db` left behind by a failing test persist into later
tests.
4. Run the test suite and introduce a temporary failure in
`test_export_dashboard_cross_database_charts` (for example by modifying one
of the export
assertions at PR hunk lines 588โ621 to fail). Observe that after the
failure, subsequent
tests in the same session still see the additional `Database` named
`"test_db_2"` and its
dataset/chart (since no rollback or cleanup ran), providing a concrete path
for cross-test
contamination if other tests assume only the example database and its
original dashboard
content exist.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cfd51916415a46aebc76ed0740836094&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=cfd51916415a46aebc76ed0740836094&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/dashboards/commands_tests.py
**Line:** 623:628
**Comment:**
*Missing Cleanup: The cleanup is only executed at the end of the happy
path, so any assertion failure or exception before that point leaves the
temporary dashboard/chart/dataset/database records in the test DB and can make
later tests flaky. Move cleanup into a `finally` block to guarantee teardown on
all paths.
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%2F37120&comment_hash=7fc23b370cf1195479164b3b45085f835f4ee98e8a5427ab690d6ac98e211db5&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37120&comment_hash=7fc23b370cf1195479164b3b45085f835f4ee98e8a5427ab690d6ac98e211db5&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]
