codeant-ai-for-open-source[bot] commented on code in PR #41126:
URL: https://github.com/apache/superset/pull/41126#discussion_r3449988141
##########
superset/tags/models.py:
##########
@@ -111,12 +111,14 @@ class TaggedObject(Model, AuditMixinNullable):
__tablename__ = "tagged_object"
id = Column(Integer, primary_key=True)
tag_id = Column(Integer, ForeignKey("tag.id"))
- object_id = Column(
- Integer,
- ForeignKey("dashboards.id"),
- ForeignKey("slices.id"),
- ForeignKey("saved_query.id"),
- )
+ # ``object_id`` is a polymorphic reference disambiguated by
``object_type``;
+ # the same value can point at a dashboard, chart or saved query. It must
not
+ # carry a foreign key to any single table: declaring FKs to dashboards,
+ # slices and saved_query at once is unsatisfiable (a row would have to
exist
+ # in all three) and breaks tagging, e.g. tagging a dashboard fails the
+ # slices FK. The original migration (c82ee8a39623) defined no FK here. See
+ # issue #35941.
+ object_id = Column(Integer)
Review Comment:
**Suggestion:** Updating the ORM model alone does not remediate databases
that were already created with the old unsatisfiable foreign keys on
`tagged_object.object_id`; those constraints will remain in the physical schema
and tagging will keep failing at runtime. Add an Alembic migration that
explicitly drops any existing FK constraints on `tagged_object.object_id` (with
dialect-safe checks) so existing environments are actually repaired.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Tag REST API fails creating tags on legacy schemas.
- ❌ Automatic tag relationships fail, blocking TAGGING_SYSTEM usage.
- ⚠️ Developers must manually drop broken FKs on databases.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start from a Superset deployment whose database schema was originally
created from the
pre-PR ORM models using SQLAlchemy `create_all`, so the `tagged_object`
table has
`object_id` defined with multiple foreign keys to dashboards, slices, and
saved_query (as
described in the PR summary). After applying the current PR, note that the
ORM model now
defines `object_id = Column(Integer)` with no foreign key in
`superset/tags/models.py:19-40`, but existing database constraints are
unchanged because
migrations do not touch them.
2. Confirm that no Alembic migration drops foreign keys on
`tagged_object.object_id`: the
initial table is created in
`superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py:73-89`
with `object_id = Column(Integer)` and no FK; later migrations
`def97f26fdfb_add_index_to_tagged_object.py:32-35`,
`07f9a902af1b_drop_postgres_enum_constrains_for_tags.py:33-41`, and
`96164e3017c6_tagged_object_unique_constraint.py:37-50` only add an index,
change enum
types, and add a unique constraint, respectively, and never drop any FK
constraints that
may already exist on `tagged_object.object_id` in a schema created via
`create_all`.
3. Upgrade the Superset application code to this PR version without manually
altering the
database schema, enable the `TAGGING_SYSTEM` feature flag, and create a
custom tag for an
existing dashboard via the Tag REST API endpoint `POST api/v1/tag/`. This
request is
handled by `TagRestApi.post` in `superset/tags/api.py:152-199`, which invokes
`CreateCustomTagWithRelationshipsCommand.run`
(`superset/commands/tag/create.py:102-121`),
and that in turn calls `TagDAO.create_tag_relationship`
(`superset/daos/tag.py:71-129`) to
create `TaggedObject` rows.
4. In `TagDAO.create_tag_relationship`, observe that Superset instantiates
`TaggedObject(object_id=object_id, object_type=object_type, tag=tag)` at
`superset/daos/tag.py:106-111` and adds it to the session; when the session
flushes, the
INSERT into `tagged_object` still hits the pre-existing database-level
foreign keys on
`object_id`. Because the dashboard id does not simultaneously exist in
`slices` and
`saved_query`, the database raises a `ForeignKeyViolation` and tag creation
fails,
demonstrating that removing the FK from the ORM at
`superset/tags/models.py:32` without an
Alembic migration leaves previously created schemas broken.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1060af8e17c043b3bc3196ecb87d913c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1060af8e17c043b3bc3196ecb87d913c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tags/models.py
**Line:** 121:121
**Comment:**
*Incomplete Implementation: Updating the ORM model alone does not
remediate databases that were already created with the old unsatisfiable
foreign keys on `tagged_object.object_id`; those constraints will remain in the
physical schema and tagging will keep failing at runtime. Add an Alembic
migration that explicitly drops any existing FK constraints on
`tagged_object.object_id` (with dialect-safe checks) so existing environments
are actually repaired.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41126&comment_hash=504d4bd66fe5bab854fce2947b0c5e49f36d79456db1fce339a6bbe1ae3cffaf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41126&comment_hash=504d4bd66fe5bab854fce2947b0c5e49f36d79456db1fce339a6bbe1ae3cffaf&reaction=dislike'>👎</a>
--
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]
