betodealmeida commented on code in PR #30833:
URL: https://github.com/apache/superset/pull/30833#discussion_r1892579367
##########
superset/commands/importers/v1/utils.py:
##########
@@ -214,3 +218,80 @@ def get_contents_from_bundle(bundle: ZipFile) -> dict[str,
str]:
for file_name in bundle.namelist()
if is_valid_config(file_name)
}
+
+
+# pylint: disable=consider-using-transaction
+def import_tag(
+ new_tag_names: list[str],
Review Comment:
This name is a bit misleading, since these are not only new tag names —
there might be some that already exist, right? Let's call this something else,
like just `target_tag_names`, or just `tag_names`.
##########
superset/commands/importers/v1/examples.py:
##########
@@ -129,7 +130,7 @@ def _import( # pylint: disable=too-many-locals,
too-many-branches
dataset = import_dataset(
config,
overwrite=overwrite,
- force_data=force_data,
+ force_data=bool(force_data),
Review Comment:
`force_data` is already a boolean, so this shouldn't be needed. Is `mypy`
complaining about this?
##########
superset/commands/importers/v1/utils.py:
##########
@@ -214,3 +218,80 @@ def get_contents_from_bundle(bundle: ZipFile) -> dict[str,
str]:
for file_name in bundle.namelist()
if is_valid_config(file_name)
}
+
+
+# pylint: disable=consider-using-transaction
+def import_tag(
+ new_tag_names: list[str],
+ contents: dict[str, Any],
+ object_id: int,
+ object_type: str,
+ db_session: Session,
+) -> list[int]:
+ """Handles the import logic for tags for charts and dashboards"""
+
+ if not feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ return []
+
+ tag_descriptions = {}
+ new_tag_ids = []
+ if "tags.yaml" in contents:
+ try:
+ tags_config = yaml.safe_load(contents["tags.yaml"])
+ except yaml.YAMLError as err:
+ logger.error("Error parsing tags.yaml: %s", err)
+ tags_config = {}
+
+ for tag_info in tags_config.get("tags", []):
+ tag_name = tag_info.get("tag_name")
+ description = tag_info.get("description", None)
+ if tag_name:
+ tag_descriptions[tag_name] = description
+ existing_tags = (
Review Comment:
Same here, these are tag associations, maybe call this `existing_assocs`.
##########
superset/commands/importers/v1/utils.py:
##########
@@ -214,3 +218,80 @@ def get_contents_from_bundle(bundle: ZipFile) -> dict[str,
str]:
for file_name in bundle.namelist()
if is_valid_config(file_name)
}
+
+
+# pylint: disable=consider-using-transaction
+def import_tag(
+ new_tag_names: list[str],
+ contents: dict[str, Any],
+ object_id: int,
+ object_type: str,
+ db_session: Session,
+) -> list[int]:
+ """Handles the import logic for tags for charts and dashboards"""
+
+ if not feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ return []
+
+ tag_descriptions = {}
+ new_tag_ids = []
+ if "tags.yaml" in contents:
+ try:
+ tags_config = yaml.safe_load(contents["tags.yaml"])
+ except yaml.YAMLError as err:
+ logger.error("Error parsing tags.yaml: %s", err)
+ tags_config = {}
+
+ for tag_info in tags_config.get("tags", []):
+ tag_name = tag_info.get("tag_name")
+ description = tag_info.get("description", None)
+ if tag_name:
+ tag_descriptions[tag_name] = description
+ existing_tags = (
+ db_session.query(TaggedObject)
+ .filter_by(object_id=object_id, object_type=object_type)
+ .all()
+ )
+
+ for tag_name in new_tag_names:
+ try:
+ tag = db_session.query(Tag).filter_by(name=tag_name).first()
Review Comment:
This is going to be potentially super slow, better to move the query outside
of the loop. Something like:
```python
existing_tags = {tag.name: tag for tag in
db_session.query(Tag).filter(tag.name.in_(new_tag_names))}
```
Then you can do:
```python
for tag_name in new_tag_names:
tag = existing_tags.get(tag_name)
if tag is None:
tag = Tag(...)
```
##########
superset/commands/tag/export.py:
##########
@@ -0,0 +1,131 @@
+# 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.
+# isort:skip_file
+
+
+from typing import Any, Callable, List, Optional, Union
+from collections.abc import Iterator
+
+import yaml
+from superset.daos.chart import ChartDAO
+from superset.daos.dashboard import DashboardDAO
+from superset.extensions import feature_flag_manager
+from superset.tags.models import TagType
+from superset.commands.tag.exceptions import TagNotFoundError
+
+
+# pylint: disable=too-few-public-methods
+class ExportTagsCommand:
+ not_found = TagNotFoundError
+
+ @staticmethod
+ def _file_name() -> str:
+ # Use the model to determine the filename
+ return "tags.yaml"
+
+ @staticmethod
+ def _merge_tags(
+ dashboard_tags: List[dict[str, Any]], chart_tags: List[dict[str, Any]]
+ ) -> List[dict[str, Any]]:
+ # Create a dictionary to prevent duplicates based on tag name
+ tags_dict = {tag["tag_name"]: tag for tag in dashboard_tags}
+
+ # Add chart tags, preserving unique tag names
+ for tag in chart_tags:
+ if tag["tag_name"] not in tags_dict:
+ tags_dict[tag["tag_name"]] = tag
+
+ # Return merged tags as a list
+ return list(tags_dict.values())
+
+ @staticmethod
+ def _file_content(
+ dashboard_ids: Optional[Union[int, List[Union[int, str]]]] = None,
+ chart_ids: Optional[Union[int, List[Union[int, str]]]] = None,
+ ) -> str:
+ payload: dict[str, list[dict[str, Any]]] = {"tags": []}
+
+ dashboard_tags = []
+ chart_tags = []
+
+ # Fetch dashboard tags if provided
+ if dashboard_ids:
+ # Ensure dashboard_ids is a list
+ if isinstance(dashboard_ids, int):
+ dashboard_ids = [
+ dashboard_ids
+ ] # Convert single int to list for consistency
+
+ dashboards = [
+ dashboard
+ for dashboard in (
+ DashboardDAO.find_by_id(dashboard_id)
+ for dashboard_id in dashboard_ids
+ )
+ if dashboard is not None
+ ]
+
+ for dashboard in dashboards:
+ tags = dashboard.tags if hasattr(dashboard, "tags") else []
+ filtered_tags = [
+ {"tag_name": tag.name, "description": tag.description}
+ for tag in tags
+ if tag.type == TagType.custom
+ ]
+ dashboard_tags.extend(filtered_tags)
+
+ # Fetch chart tags if provided
+ if chart_ids:
+ # Ensure chart_ids is a list
+ if isinstance(chart_ids, int):
+ chart_ids = [chart_ids] # Convert single int to list for
consistency
+
+ charts = [
+ chart
+ for chart in (ChartDAO.find_by_id(chart_id) for chart_id in
chart_ids)
+ if chart is not None
+ ]
+
+ for chart in charts:
+ tags = chart.tags if hasattr(chart, "tags") else []
+ filtered_tags = [
+ {"tag_name": tag.name, "description": tag.description}
+ for tag in tags
+ if "type:" not in tag.name and "owner:" not in tag.name
+ ]
+ chart_tags.extend(filtered_tags)
+
+ # Merge the tags from both dashboards and charts
+ merged_tags = ExportTagsCommand._merge_tags(dashboard_tags, chart_tags)
+ payload["tags"].extend(merged_tags)
+
+ # Convert to YAML format
+ file_content = yaml.safe_dump(payload, sort_keys=False)
+ return file_content
+
+ @staticmethod
+ def export(
+ dashboard_ids: Optional[Union[int, List[Union[int, str]]]] = None,
+ chart_ids: Optional[Union[int, List[Union[int, str]]]] = None,
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if not feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ yield from iter([])
Review Comment:
I think we can just `return` here.
--
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]
