From 394fb2f2d0e05f27ced88e8ff4fc6994696cab68 Mon Sep 17 00:00:00 2001 From: EugeneTorap Date: Wed, 16 Nov 2022 21:53:30 +0300 Subject: [PATCH] fix: slug is empty if filename is non-ASCII (#22118) --- superset/charts/commands/export.py | 8 ++--- superset/dashboards/commands/export.py | 8 ++--- superset/databases/commands/export.py | 16 ++++++---- superset/datasets/commands/export.py | 16 ++++++---- superset/utils/file.py | 23 ++++++++++++++ tests/unit_tests/utils/test_file.py | 44 ++++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 22 deletions(-) create mode 100644 superset/utils/file.py create mode 100644 tests/unit_tests/utils/test_file.py diff --git a/superset/charts/commands/export.py b/superset/charts/commands/export.py index 9b3a06c47358..bb594591e2bb 100644 --- a/superset/charts/commands/export.py +++ b/superset/charts/commands/export.py @@ -21,7 +21,6 @@ from typing import Iterator, Tuple import yaml -from werkzeug.utils import secure_filename from superset.charts.commands.exceptions import ChartNotFoundError from superset.charts.dao import ChartDAO @@ -29,6 +28,7 @@ from superset.commands.export.models import ExportModelsCommand from superset.models.slice import Slice from superset.utils.dict_import_export import EXPORT_VERSION +from superset.utils.file import get_filename logger = logging.getLogger(__name__) @@ -44,8 +44,8 @@ class ExportChartsCommand(ExportModelsCommand): @staticmethod def _export(model: Slice, export_related: bool = True) -> Iterator[Tuple[str, str]]: - chart_slug = secure_filename(model.slice_name) - file_name = f"charts/{chart_slug}_{model.id}.yaml" + file_name = get_filename(model.slice_name, model.id) + file_path = f"charts/{file_name}.yaml" payload = model.export_to_dict( recursive=False, @@ -70,7 +70,7 @@ def _export(model: Slice, export_related: bool = True) -> Iterator[Tuple[str, st payload["dataset_uuid"] = str(model.table.uuid) file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content if model.table and export_related: yield from ExportDatasetsCommand([model.table.id]).run() diff --git a/superset/dashboards/commands/export.py b/superset/dashboards/commands/export.py index 2d131d8f84e1..c17555694387 100644 --- a/superset/dashboards/commands/export.py +++ b/superset/dashboards/commands/export.py @@ -23,7 +23,6 @@ from typing import Any, Dict, Iterator, Optional, Set, Tuple import yaml -from werkzeug.utils import secure_filename from superset.charts.commands.export import ExportChartsCommand from superset.dashboards.commands.exceptions import DashboardNotFoundError @@ -35,6 +34,7 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.utils.dict_import_export import EXPORT_VERSION +from superset.utils.file import get_filename logger = logging.getLogger(__name__) @@ -111,8 +111,8 @@ class ExportDashboardsCommand(ExportModelsCommand): def _export( model: Dashboard, export_related: bool = True ) -> Iterator[Tuple[str, str]]: - dashboard_slug = secure_filename(model.dashboard_title) - file_name = f"dashboards/{dashboard_slug}_{model.id}.yaml" + file_name = get_filename(model.dashboard_title, model.id) + file_path = f"dashboards/{file_name}.yaml" payload = model.export_to_dict( recursive=False, @@ -163,7 +163,7 @@ def _export( payload["version"] = EXPORT_VERSION file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content if export_related: chart_ids = [chart.id for chart in model.slices] diff --git a/superset/databases/commands/export.py b/superset/databases/commands/export.py index 9e8cb7e37442..4d3bb7f99f25 100644 --- a/superset/databases/commands/export.py +++ b/superset/databases/commands/export.py @@ -21,13 +21,13 @@ from typing import Any, Dict, Iterator, Tuple import yaml -from werkzeug.utils import secure_filename from superset.databases.commands.exceptions import DatabaseNotFoundError from superset.databases.dao import DatabaseDAO from superset.commands.export.models import ExportModelsCommand from superset.models.core import Database from superset.utils.dict_import_export import EXPORT_VERSION +from superset.utils.file import get_filename logger = logging.getLogger(__name__) @@ -58,8 +58,8 @@ class ExportDatabasesCommand(ExportModelsCommand): def _export( model: Database, export_related: bool = True ) -> Iterator[Tuple[str, str]]: - database_slug = secure_filename(model.database_name) - file_name = f"databases/{database_slug}.yaml" + db_file_name = get_filename(model.database_name, model.id, skip_id=True) + file_path = f"databases/{db_file_name}.yaml" payload = model.export_to_dict( recursive=False, @@ -90,12 +90,14 @@ def _export( payload["version"] = EXPORT_VERSION file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content if export_related: for dataset in model.tables: - dataset_slug = secure_filename(dataset.table_name) - file_name = f"datasets/{database_slug}/{dataset_slug}.yaml" + ds_file_name = get_filename( + dataset.table_name, dataset.id, skip_id=True + ) + file_path = f"datasets/{db_file_name}/{ds_file_name}.yaml" payload = dataset.export_to_dict( recursive=True, @@ -107,4 +109,4 @@ def _export( payload["database_uuid"] = str(model.uuid) file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content diff --git a/superset/datasets/commands/export.py b/superset/datasets/commands/export.py index be9210a06c66..b71a95936ab1 100644 --- a/superset/datasets/commands/export.py +++ b/superset/datasets/commands/export.py @@ -21,13 +21,13 @@ from typing import Iterator, Tuple import yaml -from werkzeug.utils import secure_filename from superset.commands.export.models import ExportModelsCommand from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.exceptions import DatasetNotFoundError from superset.datasets.dao import DatasetDAO from superset.utils.dict_import_export import EXPORT_VERSION +from superset.utils.file import get_filename logger = logging.getLogger(__name__) @@ -43,9 +43,11 @@ class ExportDatasetsCommand(ExportModelsCommand): def _export( model: SqlaTable, export_related: bool = True ) -> Iterator[Tuple[str, str]]: - database_slug = secure_filename(model.database.database_name) - dataset_slug = secure_filename(model.table_name) - file_name = f"datasets/{database_slug}/{dataset_slug}.yaml" + db_file_name = get_filename( + model.database.database_name, model.database.id, skip_id=True + ) + ds_file_name = get_filename(model.table_name, model.id, skip_id=True) + file_path = f"datasets/{db_file_name}/{ds_file_name}.yaml" payload = model.export_to_dict( recursive=True, @@ -75,11 +77,11 @@ def _export( payload["database_uuid"] = str(model.database.uuid) file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content # include database as well if export_related: - file_name = f"databases/{database_slug}.yaml" + file_path = f"databases/{db_file_name}.yaml" payload = model.database.export_to_dict( recursive=False, @@ -98,4 +100,4 @@ def _export( payload["version"] = EXPORT_VERSION file_content = yaml.safe_dump(payload, sort_keys=False) - yield file_name, file_content + yield file_path, file_content diff --git a/superset/utils/file.py b/superset/utils/file.py new file mode 100644 index 000000000000..f1bdac897eab --- /dev/null +++ b/superset/utils/file.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from werkzeug.utils import secure_filename + + +def get_filename(model_name: str, model_id: int, skip_id: bool = False) -> str: + slug = secure_filename(model_name) + filename = slug if skip_id else f"{slug}_{model_id}" + return filename if slug else str(model_id) diff --git a/tests/unit_tests/utils/test_file.py b/tests/unit_tests/utils/test_file.py new file mode 100644 index 000000000000..de20402e5c21 --- /dev/null +++ b/tests/unit_tests/utils/test_file.py @@ -0,0 +1,44 @@ +# -*- coding: utf-8 -*- +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import pytest + +from superset.utils.file import get_filename + + +@pytest.mark.parametrize( + "model_name,model_id,skip_id,expected_filename", + [ + ("Energy Sankey", 132, False, "Energy_Sankey_132"), + ("Energy Sankey", 132, True, "Energy_Sankey"), + ("folder1/Energy Sankey", 132, True, "folder1_Energy_Sankey"), + ("D:\\Charts\\Energy Sankey", 132, True, "DChartsEnergy_Sankey"), + ("🥴🥴🥴", 4751, False, "4751"), + ("🥴🥴🥴", 4751, True, "4751"), + ("Energy Sankey 🥴🥴🥴", 4751, False, "Energy_Sankey_4751"), + ("Energy Sankey 🥴🥴🥴", 4751, True, "Energy_Sankey"), + ("你好", 475, False, "475"), + ("你好", 475, True, "475"), + ("Energy Sankey 你好", 475, False, "Energy_Sankey_475"), + ("Energy Sankey 你好", 475, True, "Energy_Sankey"), + ], +) +def test_get_filename( + model_name: str, model_id: int, skip_id: bool, expected_filename: str +) -> None: + original_filename = get_filename(model_name, model_id, skip_id) + assert expected_filename == original_filename