Improve documentation for adding foreign keys #1025
Conversation
Add explanation about necessity to define relations when adding foreign keys.
README.md
Outdated
| @@ -86,6 +86,17 @@ for each table that includes a `resource_owner_id` column: | |||
| add_foreign_key :table_name, :users, column: :resource_owner_id | |||
| ``` | |||
|
|
|||
| Remember to add associations to your model so the related records are deleted. | |||
| If you don't do this a `ActiveRecord::InvalidForeignKey`-error will be raised | |||
There was a problem hiding this comment.
Let's write "an ActiveRecord::InvalidForeignKey", ok?
README.md
Outdated
| @@ -86,6 +86,17 @@ for each table that includes a `resource_owner_id` column: | |||
| add_foreign_key :table_name, :users, column: :resource_owner_id | |||
| ``` | |||
|
|
|||
| Remember to add associations to your model so the related records are deleted. | |||
| If you don't do this a `ActiveRecord::InvalidForeignKey`-error will be raised | |||
| when you try to delete a model with related access grants or access tokens. | |||
There was a problem hiding this comment.
delete method will ignore callbacks for associations, so let's write "try to destroy the model" instead to avoid misunderstanding.
README.md
Outdated
|
|
||
| ```ruby | ||
| class User < ApplicationRecord | ||
| has_many :oauth_access_grants, class_name: "Doorkeeper::AccessGrant", foreign_key: :resource_owner_id, dependent: :destroy |
There was a problem hiding this comment.
Does it really required to name associations like oauth_access_grants ? Doorkeeper::AccessGrant already has table name prefixes, so Rails must recognize the name of the table for associations.
Maybe it will be better to name them like:
has_many :access_grants, ...
has_many :access_tokens, ...Also I suggest to change dependent: :destroy option to dependent: :delete_all - it will delete all the associated records in one SQL query instead of N+1. But we can save :destroy variant in comment, like " # or :destroy if you need callbacks"
What do you think?
|
Hi @fschwahn . Can you please take a look at my comments above? Also please add a small changelog entry to NEWS.md and rebase commits to squash them into one. Thanks for your work! |
|
Hi @fschwahn . Any updates here? |
|
Had a busy week, thanks for reminding me. I incorporated all your suggestions, all of them make sense. I did all the editing using Github, I don't have the repository checked out, so I can't easily squash. But you can Squash & Merge using Github UI. |
|
Thank you @fschwahn, good work! 👍 |
Add explanation about necessity to define relations when adding foreign keys as mentioned in #1007