betodealmeida commented on code in PR #30833:
URL: https://github.com/apache/superset/pull/30833#discussion_r2021169493
##########
superset/commands/dashboard/export.py:
##########
@@ -173,7 +183,14 @@ def _export(
if export_related:
chart_ids = [chart.id for chart in model.slices]
+ dashboard_ids = model.id
+ ExportChartsCommand.disable_tag_export()
yield from ExportChartsCommand(chart_ids).run()
+ ExportChartsCommand.enable_tag_export()
Review Comment:
Setting the class attribute can be problematic if we have multiple exports
happening at the same time in the same process, there could be a race condtion.
It would be better to instantiate the command and set it there to prevent this:
```python
command = ExportChartsCommand(chart_ids)
command.disable_tag_export()
yield from command.run()
command.enable_tag_export()
```
But I think it's better and clearer if we just pass a parameter when we
instantiate the command:
```suggestion
yield from ExportChartsCommand(chart_ids,
include_tags=True).run()
```
And then we update `ExportChartsCommand` accordingly:
```python
class ExportChartsCommand(ExportModelsCommand):
def __init__(self, *args: Any, include_tags: bool = True, **kwargs: Any):
self._include_tags = include_tags
super(*args, **kwargs)
```
##########
superset/commands/importers/v1/utils.py:
##########
@@ -214,3 +219,88 @@ 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
+# ruff: noqa: C901
+def import_tag(
+ target_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_assocs = (
+ db_session.query(TaggedObject)
+ .filter_by(object_id=object_id, object_type=object_type)
+ .all()
+ )
+
+ existing_tags = {
+ tag.name: tag
+ for tag in db_session.query(Tag).filter(Tag.name.in_(target_tag_names))
+ }
+ for tag_name in target_tag_names:
+ try:
+ tag = existing_tags.get(tag_name)
+
+ # If tag does not exist, create it
+ if tag is None:
+ description = tag_descriptions.get(tag_name, None)
+ tag = Tag(name=tag_name, description=description,
type="custom")
+ db_session.add(tag)
+ db_session.commit()
Review Comment:
Let's not commit or rollback here, we should leave that to the command via
the `@transaction` decorator. See
https://github.com/apache/superset/issues/25108.
--
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]
