rusackas commented on code in PR #35541:
URL: https://github.com/apache/superset/pull/35541#discussion_r4028019625
##########
superset/daos/tag.py:
##########
@@ -122,7 +122,7 @@ def get_by_name(name: str, type_: TagType = TagType.custom)
-> Tag:
.first()
)
if not tag:
- tag = get_tag(name, db.session, type_)
+ tag = get_or_create_tag(name, db.session, type_)
return tag
Review Comment:
Renamed this to `get_or_create_by_name` and tightened the docstring too, it
was saying "none otherwise" and then "creates one if not found" in the same
breath. Updated the callers and tests to match.
##########
superset/commands/utils.py:
##########
@@ -183,9 +183,12 @@ def update_tags(
]
if tag_ids_to_add:
tags_to_add = TagDAO.find_by_ids(tag_ids_to_add)
- TagDAO.create_custom_tagged_objects(
- object_type, object_id, [tag.name for tag in tags_to_add]
- )
+ # Only add custom tags - system tags are managed automatically
+ custom_tags_to_add = [tag for tag in tags_to_add if tag.type ==
TagType.custom]
+ if custom_tags_to_add:
+ TagDAO.create_custom_tagged_objects(
+ object_type, object_id, [tag.name for tag in
custom_tags_to_add]
+ )
Review Comment:
I think raising here would be the better call. Silently dropping a system
tag ID off the payload means the caller has no idea it didn't take. Feels like
a good follow-up if it's not worth holding this PR up for though.
##########
superset/tags/models.py:
##########
@@ -130,17 +130,24 @@ def __str__(self) -> str:
return f"<TaggedObject: {self.object_type}:{self.object_id}
TAG:{self.tag_id}>"
-def get_tag(
+def get_or_create_tag(
name: str,
session: orm.Session, # pylint: disable=disallowed-name
type_: TagType,
) -> Tag:
+ from sqlalchemy.exc import IntegrityError
+
tag_name = name.strip()
tag = session.query(Tag).filter_by(name=tag_name, type=type_).one_or_none()
if tag is None:
tag = Tag(name=escape(tag_name), type=type_)
session.add(tag)
- session.commit()
+ try:
+ session.commit()
+ except IntegrityError:
+ # Another transaction created the tag concurrently, fetch it
+ session.rollback()
+ tag = session.query(Tag).filter_by(name=tag_name, type=type_).one()
Review Comment:
I'd be a little wary of threading a commit flag through here without also
checking the event-listener call sites, a couple of those fire outside a
command's @transaction and need the tag to exist right away. Seems like a
follow-up rather than something to untangle in this PR.
--
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]
