Skip to content

Conversation

@earshinov
Copy link

@earshinov earshinov commented Apr 12, 2024

…to let the user define additional sqlalchemy.orm.ForeignKey attributes, such as ondelete and onupdate, for foreign keys defined in a base model.

UPD YuriiMotov:

Currently we can specify foreign_key as an instance of sqlalchemy.ForeignKey passing it via sa_column_args:

    class Item(SQLModel, table=True):
        owner_id: Optional[int] = Field(
            default=None, sa_column_args=(ForeignKey("user.id", ondelete="SET NULL"),)
        )

But in current implementation it doesn't work if we use inheritance:

    class Base(SQLModel):
        owner_id: Optional[int] = Field(
            default=None, sa_column_args=(ForeignKey("user.id", ondelete="SET NULL"),)
        )

    class Asset(Base, table=True):
        id: Optional[int] = Field(default=None, primary_key=True)

    class Document(Base, table=True):
        id: Optional[int] = Field(default=None, primary_key=True)

The code above will lead to "This ForeignKey already has a parent" error.

This PR fixes that error and allows passing an instance of ForeignKey as a value of foreign_key parameter of Field:

    class Item(SQLModel, table=True):
        owner_id: Optional[int] = Field(
            default=None,
            foreign_key=ForeignKey("user.id", ondelete="SET NULL"),
        )

@earshinov earshinov force-pushed the fk-attrs branch 3 times, most recently from b1ecba8 to 1865cf2 Compare April 12, 2024 16:45
@earshinov earshinov changed the title ✨Add foreign_key_args and foreign_key_kwargs arguments to Field ✨Add sa_foreign_key_args and sa_foreign_key_kwargs arguments to Field Apr 12, 2024
@alejsdev alejsdev added the feature New feature or request label Jul 5, 2024
Copy link
Member

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@earshinov, thanks for your interest and efforts!

IMO, the option with sa_column_args=[ForeignKey(...)] looks better than with sa_foreign_key_kwargs={} - in first case we have type hints and it's more readable.

As I understand, the problem is that in case of inheritance the same ForeignKey instance is being re-used in several models and it causes that exception.
What if we approach to solve this in a different way - use copy of ForeignKey object instead of that object itself?
I mean in get_column_from_field extract ForeignKey from sa_column_args (if exists) and pass its copy instead

…...)` to let the user define additional `sqlalchemy.orm.ForeignKey` attributes, such as `ondelete` and `onupdate`, for foreign keys defined in a base model.
@earshinov
Copy link
Author

@YuriiMotov , thanks for a useful suggestion. Indeed, the approach you described is working and provides more type safety. I updated the PR.

@earshinov
Copy link
Author

earshinov commented Sep 2, 2025

Docs will need to be updated, too, to make the users aware that foreign_key can now be given as a sqlalchemy.schema.ForeignKey instance instead of a string. I am not sure what would be the best place to mention this.

@YuriiMotov
Copy link
Member

YuriiMotov commented Oct 27, 2025

@earshinov, please review the changes I made (commits and PR description). Is everything Ok from your point of view?

@YuriiMotov YuriiMotov changed the title ✨Add sa_foreign_key_args and sa_foreign_key_kwargs arguments to Field ✨ Allow specifying the foreign_key using an instance of ForeignKey Oct 27, 2025
Copy link
Member

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@earshinov
Copy link
Author

@earshinov, please review the changes I made (commits and PR description). Is everything Ok from your point of view?

Yes, looks good to me! Thank you for the edits

@github-actions github-actions bot added the conflicts Automatically generated when a PR has a merge conflict label Dec 26, 2025
@github-actions
Copy link
Contributor

This pull request has a merge conflict that needs to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conflicts Automatically generated when a PR has a merge conflict feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants