Bug with the tag uniqueness check

zverok commented 2 weeks ago

Bug description

NB: I can’t currently run Superset 4 locally, but we backported the TagDAO.create_custom_tagged_objects to our Superset 3, and I assume that the bug affects Superset 4 too (considering the code logic).

Basically, the reproduction is to call POST /api/v1/tags/2/<chart_id> {"properties": {"tags": ["<some-tag>"]}} twice (with the same chart and the same tag). Superset will fail with the tag uniqueness check.

The problem seems to be here:

in line 60, a new TaggedObject is created unconditionally (I think this is wrong)
...then, there is an existence check, and then in line 72, the same object is created conditionally.

At least when ported to the Superset 3 codebase, this fails predictably. Maybe Superset 4 somehow prevents that on another layer, but the method looks wrong in any case. (Or I am missing something.)

Screenshots/recordings

No response

Superset version

master / latest-dev

Python version

3.10

Node version

16

Browser

Chrome

Additional context

No response

Checklist

[X] I have searched Superset docs and Slack and didn't find a solution to my problem.
[X] I have searched the GitHub issue tracker and didn't find a similar bug report.
[ ] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

dosubot[bot] commented 2 weeks ago

Hey there, @zverok! I'm here to help you with any bugs, questions, or contributions you have related to Superset. Let's tackle this issue together!

Yes, there is indeed a bug in the TagDAO.create_custom_tagged_objects method related to the tag uniqueness check. The method incorrectly appends a TaggedObject to the tagged_objects list before checking if the association already exists. This results in the same TaggedObject being added twice if it does not already exist in the database. The correct logic should be to append the TaggedObject only if it does not already exist.

Here is the problematic code:

tagged_objects.append(
    TaggedObject(object_id=object_id, object_type=object_type, tag=tag)
)

# Check if the association already exists
existing_tagged_object = (
    db.session.query(TaggedObject)
    .filter_by(object_id=object_id, object_type=object_type, tag=tag)
    .first()
)

if not existing_tagged_object:
    tagged_objects.append(
        TaggedObject(object_id=object_id, object_type=object_type, tag=tag)
    )

The first tagged_objects.append should be removed, and the TaggedObject should only be appended if existing_tagged_object is None ^[1].

To continue talking to Dosu, mention @dosu.

rusackas commented 2 weeks ago

@eschutho is anyone looking at tags much these days? Maybe @hughhhh?

apache / superset