-
-
Notifications
You must be signed in to change notification settings - Fork 785
✨ Allow specifying the foreign_key using an instance of ForeignKey
#894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b1ecba8 to
1865cf2
Compare
foreign_key_args and foreign_key_kwargs arguments to Field…sa_foreign_key_args and sa_foreign_key_kwargs arguments to Field…
YuriiMotov
left a comment
There was a problem hiding this 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.
…the use to pass foreign_key=ForeignKey(...)
|
@YuriiMotov , thanks for a useful suggestion. Indeed, the approach you described is working and provides more type safety. I updated the PR. |
|
Docs will need to be updated, too, to make the users aware that |
|
@earshinov, please review the changes I made (commits and PR description). Is everything Ok from your point of view? |
sa_foreign_key_args and sa_foreign_key_kwargs arguments to Field…foreign_key using an instance of ForeignKey
YuriiMotov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Yes, looks good to me! Thank you for the edits |
…to let the user define additionalsqlalchemy.orm.ForeignKeyattributes, such asondeleteandonupdate, for foreign keys defined in a base model.UPD YuriiMotov:
Currently we can specify
foreign_keyas an instance ofsqlalchemy.ForeignKeypassing it viasa_column_args:But in current implementation it doesn't work if we use inheritance:
The code above will lead to "This ForeignKey already has a parent" error.
This PR fixes that error and allows passing an instance of
ForeignKeyas a value offoreign_keyparameter ofField: