[BUG] UndefinedTable Error during Alembic Migration with PostgeSQL

movchan74 commented 1 month ago

Bug Description

When attempting to run Alembic migrations to set up a PostgreSQL database, a ProgrammingError occurs indicating that the video table does not exist. This error is encountered during the creation of the media table, which includes a foreign key reference to the video table.

Steps to Reproduce

Configure SDK to use the PostgreSQL database.
Run the Alembic migration (start any example).

Expected Behavior

The Alembic migration should execute successfully, creating the necessary tables (media, video, captions) along with their respective foreign key constraints without any errors.

Actual Behavior

The migration fails with a ProgrammingError, specifically indicating that the video table does not exist. This error occurs because the media table attempts to create a foreign key constraint referencing the video table before the video table has been created.

Environment

I've noticed the issue while working on task queue: 0cb7697b09e8dce3f979a74b9ac47fba6b56af00 But it is not specific to the task queue changes and I assume it affects the main branch as well.

Error Traceback

UndefinedTable: relation "video" does not exist

[SQL: 
CREATE TABLE media (
    id VARCHAR NOT NULL, 
    media_type VARCHAR, 
    video_id INTEGER, 
    create_ts TIMESTAMP WITH TIME ZONE DEFAULT (CURRENT_TIMESTAMP), 
    update_ts TIMESTAMP WITH TIME ZONE, 
    CONSTRAINT pk_media PRIMARY KEY (id), 
    CONSTRAINT fk_media_video_id_video FOREIGN KEY(video_id) REFERENCES video (id)
)
]

(Background on this error at: https://sqlalche.me/e/20/f405)

Additional Notes

Insight from Evan: There might be an issue with the mutual foreign key dependency between the media and video tables. While PostgreSQL supports foreign key constraints, circular dependencies can cause issues during table creation.

movchan74 commented 1 month ago

Here is the proposed change to simplify media and video relationship.

Remove video ID and use media ID as primary key for the video.
Media ID will be used as primary key for both media and video.
ID in video is also foreign key to the ID of media.

Things to consider:

There might be some performance implication of using strings as primary keys but we already used it as primary key for media.
Right now I just use String as type but will probably limit the lenght and use MediaIdSqlType as alias.

I've tested it and it seems to be working just fine. Cascade deletion works and it works with postgres.

Here is the code of proposed changes:

Before:

class MediaEntity(BaseEntity, TimeStampEntity):
    """Table for media items."""

    __tablename__ = "media"
    id = Column(MediaIdSqlType, primary_key=True)
    media_type = Column(String, comment="The type of media")
    video_id = Column(
        Integer,
        ForeignKey("video.id", ondelete="CASCADE"),
        nullable=True,
        comment="If media_type is `video`, the id of the video this entry represents.",
    )

    video = relationship(
        "VideoEntity",
        backref=backref("media", passive_deletes=True, uselist=False),
        cascade="all, delete",
        uselist=False,
        post_update=True,
        foreign_keys=[video_id],
    )

After:

class MediaEntity(BaseEntity, TimeStampEntity):
    """Table for media items."""

    __tablename__ = "media"
    id = Column(String, primary_key=True, default=lambda: str(uuid4()), comment="Unique identifier for the media")
    media_type = Column(String, comment="The type of media")

    video = relationship(
        "VideoEntity",
        backref=backref("media", passive_deletes=True),
        cascade="all, delete",
        uselist=True,
    )

Before:

class VideoEntity(BaseEntity, TimeStampEntity):
    """ORM class for video file (video, etc)."""

    __tablename__ = "video"

    id = Column(Integer, primary_key=True)
    media_id = Column(
        MediaIdSqlType,
        ForeignKey("media.id"),
        nullable=False,
        comment="Foreign key to media table",
    )
    duration = Column(Float, comment="Media duration in seconds")
    <...>

After:

class VideoEntity(BaseEntity, TimeStampEntity):
    """ORM class for video file (video, etc)."""

    __tablename__ = "video"

    id = Column(String, ForeignKey("media.id"), primary_key=True)
    duration = Column(Float, comment="Media duration in seconds")
    <...>

@ashwinnair14 @evanderiel @HRashidi What do you think?

HRashidi commented 1 month ago

It's ok to remove the video_id from the media. But why we need to set the id of the VideoEntity as a Foreign key to the media_id. Can we just work with the media_id and use the video from it? (removing the video_id) video = relationship( 'Video', back_populates="video", lazy="dynamic", cascade="all, delete", )

HRashidi commented 1 month ago

Can we just remove the video_id from the media_id (but keep the relationship there)? Also we can use the media_id inside the video table as it is and add an index for it for being fast (instead of using media_id as it's primary id)

movchan74 commented 1 month ago

Can we just remove the video_id from the media_id (but keep the relationship there)? Also we can use the media_id inside the video table as it is and add an index for it for being fast (instead of using media_id as it's primary id)

Can you write the code for MediaEntity and VideoEntity to illustrate your point? I don't get it.

evanderiel commented 1 month ago

Having the primary key also be a foreign key is unusual but I guess it's fine.

HRashidi commented 1 month ago

class MediaEntity(BaseEntity, TimeStampEntity):
    """Table for media items."""

    __tablename__ = "media"
    id = Column(MediaIdSqlType, primary_key=True)
    media_type = Column(String, comment="The type of media")

    video = relationship(
        'VideoEntity',
        back_populates="video",
        lazy="dynamic",
        cascade="all, delete",
    )

class VideoEntity(BaseEntity, TimeStampEntity):
    """ORM class for video file (video, etc)."""

    __tablename__ = "video"

    id = Column(Integer, primary_key=True)
    media_id = Column(
        MediaIdSqlType,
        ForeignKey("media.id"),
        nullable=False,
        comment="Foreign key to media table",
        index=True
    )
    media = relationship('MediaEntity')
    ...

movchan74 commented 1 month ago

@HRashidi Thanks for the code! I understand what you mean now.

Here are a few reasons why I believe my proposed changes might be more beneficial:

Simplicity and Clarity: By using the same ID for both MediaEntity and VideoEntity, we simplify the relationship and avoid redundancy. The ID clearly represents the same media item in both tables, making the schema easier to understand and maintain.
Consistency in ID Types: Using the same ID type for both tables maintains consistency and avoids potential issues related to mismatched key types.
True 1-to-1 Relationship: This design enforces a true 1-to-1 relationship between media items and videos, which is more intuitive and aligns with the concept that a media item of type video should have only one corresponding video entity.

movchan74 commented 4 weeks ago

Done: https://github.com/mobiusml/aana_sdk/pull/115

mobiusml / aana_sdk