Skip to content

chore(export): Added ability to export chart YAML files with Unicode characters, fix #20331 #28008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xyb wants to merge 4 commits into
base: master
Choose a base branch
from
Open

chore(export): Added ability to export chart YAML files with Unicode characters, fix #20331 #28008

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a8524f
chore(export): Added ability to export chart YAML files with Unicode …
xyb Apr 12, 2024
32a723e
test: add tests for hunman-readable unicode chars in exported yaml files
xyb May 22, 2024
a526bf9
Merge branch 'master' into patch-1
rusackas Jun 11, 2026
a274ae4
test(export): harden unicode export test isolation and cleanup
xyb Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset/commands/chart/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def _file_content(model: Slice) -> str:
if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
tags = getattr(model, "tags", [])
payload["tags"] = [tag.name for tag in tags if tag.type == TagType.custom]
file_content = yaml.safe_dump(payload, sort_keys=False)
file_content = yaml.safe_dump(payload, sort_keys=False, allow_unicode=True)
return file_content

_include_tags: bool = True # Default to True
Expand Down
2 changes: 1 addition & 1 deletion superset/commands/dashboard/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ def _file_content(model: Dashboard) -> str:
tags = model.tags if hasattr(model, "tags") else []
payload["tags"] = [tag.name for tag in tags if tag.type == TagType.custom]

file_content = yaml.safe_dump(payload, sort_keys=False)
file_content = yaml.safe_dump(payload, sort_keys=False, allow_unicode=True)
return file_content

@staticmethod
Expand Down
7 changes: 5 additions & 2 deletions superset/commands/database/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def _file_content(model: Database) -> str:

payload["version"] = EXPORT_VERSION

file_content = yaml.safe_dump(payload, sort_keys=False)
file_content = yaml.safe_dump(payload, sort_keys=False, allow_unicode=True)
return file_content

@staticmethod
Expand Down Expand Up @@ -140,6 +140,9 @@ def _export(
yield (
file_path,
functools.partial( # type: ignore
yaml.safe_dump, payload, sort_keys=False
yaml.safe_dump,
payload,
sort_keys=False,
allow_unicode=True,
),
)
7 changes: 5 additions & 2 deletions superset/commands/dataset/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def _file_content(model: SqlaTable) -> str:
# serialize. Convert all keys to regular strings to fix YAML serialization.
payload = {str(key): value for key, value in payload.items()}

file_content = yaml.safe_dump(payload, sort_keys=False)
file_content = yaml.safe_dump(payload, sort_keys=False, allow_unicode=True)
return file_content

@staticmethod
Expand Down Expand Up @@ -128,4 +128,7 @@ def _export(

payload["version"] = EXPORT_VERSION

yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False)
yield (
file_path,
lambda: yaml.safe_dump(payload, sort_keys=False, allow_unicode=True),
)
24 changes: 24 additions & 0 deletions tests/integration_tests/charts/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
)

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Severity Level: Major ⚠️ 
- ❌ Failing chart export test leaves renamed slice in metadata.
- ⚠️ Later energy-dashboard fixtures can accumulate unexpected duplicate slices.
Steps of Reproduction ✅ 
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`.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
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
👍 | 👎



class TestImportChartsCommand(SupersetTestCase):
@patch("superset.utils.core.g")
Expand Down
23 changes: 23 additions & 0 deletions tests/integration_tests/dashboards/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,29 @@ def test_export_dashboard_command_no_related(self, mock_g1, mock_g2):
}
assert expected_paths == set(contents.keys())

@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
@patch("superset.security.manager.g")
@patch("superset.views.base.g")
def test_export_dashboard_command_unicode_chars(self, mock_g1, mock_g2):
mock_g1.user = security_manager.find_user("admin")
mock_g2.user = security_manager.find_user("admin")
db.session.query(Dashboard).filter_by(slug="world_health").update(
{"dashboard_title": "中文"},
)
example_dashboard = (
db.session.query(Dashboard).filter_by(dashboard_title="中文").one()
)

command = ExportDashboardsCommand([example_dashboard.id])
contents = dict(command.run())

path = f"dashboards/{example_dashboard.id}.yaml"
assert path in set(contents.keys())
yaml_content = contents[path]()
metadata = yaml.safe_load(yaml_content)
assert metadata["dashboard_title"] == "中文"
assert "dashboard_title: 中文" in yaml_content

@bito-code-review bito-code-review Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test isolation failure

This test modifies shared fixture data (dashboard_title changed from "World Bank's Data" to "中文") without restoring it. The fixture cleanup deletes by ID, not by title, so the modification persists. If test_export_dashboard_command_no_related runs after this test, it will fail at line 506 because the exported filename will be dashboards/{id}.yaml instead of dashboards/World_Banks_Data_{id}.yaml.

Code suggestion
Check the AI-generated fix before applying
 --- tests/integration_tests/dashboards/commands_tests.py (lines 528-531) ---
 528:         yaml_content = contents[path]()
 529:         metadata = yaml.safe_load(yaml_content)
 530:         assert metadata["dashboard_title"] == "中文"
 531:         assert "dashboard_title: 中文" in yaml_content
 +        # Restore original dashboard title for test isolation
 +        db.session.query(Dashboard).filter_by(slug="world_health").update(
 +            {"dashboard_title": "World Bank's Data"},
 +        )
 +        db.session.commit()

Code Review Run #e26422

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them



class TestImportDashboardsCommand(SupersetTestCase):
def test_import_v0_dashboard_cli_export(self):
Expand Down
18 changes: 18 additions & 0 deletions tests/integration_tests/databases/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Severity Level: Major ⚠️ 
- ⚠️ Temporary "中文" database can remain committed during test session.
- ⚠️ Other tests using separate sessions may see unexpected extra database.
Steps of Reproduction ✅ 
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.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
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
👍 | 👎



class TestImportDatabasesCommand(SupersetTestCase):
@patch("superset.security.manager.g")
Expand Down
32 changes: 32 additions & 0 deletions tests/integration_tests/datasets/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,37 @@ def test_export_dataset_command_no_related(self, mock_g):
f"datasets/examples/energy_usage_{example_dataset.id}.yaml",
]

@patch("superset.security.manager.g")
def test_export_dataset_command_unicode_chars(self, mock_g):
mock_g.user = security_manager.find_user("admin")
examples_db = get_example_database()
with examples_db.get_sqla_engine() as engine:
engine.execute("DROP TABLE IF EXISTS 中文")
engine.execute("CREATE TABLE 中文 AS SELECT 2 as col")
if db.session.query(SqlaTable).filter_by(table_name="中文").count():
db.session.query(SqlaTable).filter_by(table_name="中文").delete()

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The cleanup query deletes datasets by table_name only, without scoping to the current example database. If another database contains a dataset with the same table name, this test will delete unrelated rows and can make other tests fail unpredictably. Restrict the deletion to the target database (for example by database_id) before creating the new dataset. [logic error]

Severity Level: Major ⚠️ 
- ⚠️ Other databases' datasets named 中文 deleted during test cleanup.
- ⚠️ Integration tests may fail when sharing table_name 中文.
Steps of Reproduction ✅ 
1. Before running the dataset tests, create a dataset with `table_name="中文"` on a
non-example database by calling `CreateDatasetCommand({"table_name": "中文", "database":
<other_db_id>}).run()` (implemented in `superset/commands/dataset/create.py:3-13`, which
persists a new `SqlaTable` row with `database_id=<other_db_id>`).

2. Run the integration test `test_export_dataset_command_unicode_chars` defined at
`tests/integration_tests/datasets/commands_tests.py:276-305`, which sets up a physical
table `中文` on `examples_db` and prepares to create a dataset for it.

3. Inside this test, the cleanup guard at
`tests/integration_tests/datasets/commands_tests.py:281-285` executes

   `if db.session.query(SqlaTable).filter_by(table_name="中文").count():
   db.session.query(SqlaTable).filter_by(table_name="中文").delete()`

   (the delete is at line 284), issuing a DELETE scoped only by `table_name` and not by
   `database_id`.

4. After the test finishes, run `db.session.query(SqlaTable).filter_by(table_name="中文",
database_id=<other_db_id>).one_or_none()` in the same test process and observe that it
returns `None`, showing that the unrelated dataset on the other database was deleted by
this test's cleanup.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** tests/integration_tests/datasets/commands_tests.py
**Line:** 284:284
**Comment:**
	*Logic Error: The cleanup query deletes datasets by `table_name` only, without scoping to the current example database. If another database contains a dataset with the same table name, this test will delete unrelated rows and can make other tests fail unpredictably. Restrict the deletion to the target database (for example by `database_id`) before creating the new dataset.

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
👍 | 👎

with override_user(security_manager.find_user("admin")):
example_dataset = CreateDatasetCommand(
{
"table_name": "中文",
"database": examples_db.id,
}
).run()

command = ExportDatasetsCommand([example_dataset.id], export_related=False)
contents = dict(command.run())

path = f"datasets/examples/{example_dataset.id}.yaml"
assert path in set(contents.keys())
yaml_content = contents[path]()
assert "table_name: 中文" in yaml_content

db.session.delete(example_dataset)
db.session.commit()
with examples_db.get_sqla_engine() as engine:
engine.execute("DROP TABLE 中文")
db.session.commit()
Comment on lines +298 to +310

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The dataset/table teardown runs only on the happy path; if any assertion above fails, cleanup is skipped and leaves persistent DB artifacts that can break later tests. Wrap the body in try/finally and move deletion/drop logic into the finally block. [missing cleanup]

Severity Level: Major ⚠️ 
- ⚠️ Failed unicode dataset test leaves dataset \"中文\" persisted.
- ⚠️ Failed test leaves physical table \"中文\" in examples DB.
- ⚠️ Later tests see unexpected tables in shared example database.
Steps of Reproduction ✅ 
1. The test `test_export_dataset_command_unicode_chars` in
`tests/integration_tests/datasets/commands_tests.py:27-61` creates a physical table named
`中文` in the example database via `examples_db.get_sqla_engine().execute("CREATE TABLE 中文
AS SELECT 2 as col")` at lines 31-33, and then creates a corresponding `SqlaTable` dataset
via `CreateDatasetCommand(...).run()` at lines 41-47.

2. The test exports this dataset using `ExportDatasetsCommand([example_dataset.id],
export_related=False)` at line 49, builds `path =
f"datasets/examples/{example_dataset.id}.yaml"` at line 52, and asserts the YAML contains
`table_name: 中文` at line 55.

3. If any of the operations or assertions from lines 49-55 fail (for example due to a
regression in dataset export formatting or path construction), Python raises an exception
and the test function exits before executing the cleanup block at lines 57-61
(`db.session.delete(example_dataset)`, `db.session.commit()`, `DROP TABLE 中文`,
`db.session.commit()`).

4. Because there is no surrounding `try/finally`, in that failure scenario the `SqlaTable`
row for `table_name="中文"` and the physical table `中文` remain in the shared example
database returned by `get_example_database()` (`superset/utils/database.py`), unlike the
analogous database-unicode test in
`tests/integration_tests/databases/commands_tests.py:19-41` which wraps cleanup in
`finally`; these leaked artifacts can then be seen by subsequent tests that rely on the
example database contents, breaking assumptions about test isolation.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** tests/integration_tests/datasets/commands_tests.py
**Line:** 298:310
**Comment:**
	*Missing Cleanup: The dataset/table teardown runs only on the happy path; if any assertion above fails, cleanup is skipped and leaves persistent DB artifacts that can break later tests. Wrap the body in `try/finally` and move deletion/drop logic into the `finally` block.

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
👍 | 👎



class TestImportDatasetsCommand(SupersetTestCase):
@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
Expand Down Expand Up @@ -602,6 +633,7 @@ def test_create_dataset_command(self):
assert [owner.username for owner in table.owners] == ["admin"]

db.session.delete(table)
db.session.commit()
Comment thread
xyb marked this conversation as resolved.
with examples_db.get_sqla_engine() as engine:
engine.execute("DROP TABLE test_create_dataset_command")
db.session.commit()
Expand Down
Loading