-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Added option to validate which models are allowed to be associated to a polymorphic belongs_to #51738
base: main
Are you sure you want to change the base?
Added option to validate which models are allowed to be associated to a polymorphic belongs_to #51738
Conversation
7a2078c
to
5116d51
Compare
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.
I like this feature and the code and tests look good to me.
Presuming a Core team member will also approve, can you also add something to the "Polymorphic Associations" section in guides/source/association_basics.md
?
250ddc9
to
e22524b
Compare
I've not done many references in markdown. I tried to follow other parts of the guide. I think I linked the text to https://edgeguides.rubyonrails.org/active_record_validations.html#inclusion correctly. I'm not quite sure of the best way to preview the guides, so I'm sort of flying blind on that. |
e22524b
to
018fcb1
Compare
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.
This looks good to me. Needs to be rebased but as soon as you do that, I'll tag it for review by one of the core team.
018fcb1
to
317773f
Compare
@flavorjones I've rebased the branch |
317773f
to
b8688b6
Compare
Just fixed another merge conflict on the changelog. The ActiveRecord changelog is a hot file. I guess this is why the Rubocop repo has PRs add a new file to a changelogs folder, and then they generate a CHANGELOG.md at each release. |
@natematykiewicz Thanks! I've poked the core team to review. |
@@ -121,6 +121,13 @@ def self.define_validations(model, reflection) | |||
required = !reflection.options[:optional] | |||
end | |||
|
|||
case reflection.options[:polymorphic] | |||
when Array, String, Symbol |
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.
Should we accept String and Symbol? If there is only one possible value people should just pass an array with one element
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.
Are you thinking we simply support arrays of strings? I was trying to be flexible, but only supporting an array of strings is certainly easier to document and explain.
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.
Or are you saying I just support an array, and then map the values to string still (which would allow an array of strings/symbols)?
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.
I think Rafael is suggesting the latter: just support Array[String|Symbol]
and not support passing a single string or symbol.
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.
I've made the changes, but I'm not sure what to do about the changelog. I see @rafaelfranca changed the milestone to 8.0.0. So I need to rebase this to main's recently wiped changelog instead of 7.2.0's changelog?
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.
@natematykiewicz Yes, please do add it to the new (empty or nearly-empty) CHANGELOG file, this would now land in 8.0.
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.
Done @flavorjones!
b8688b6
to
c1f6bf4
Compare
@natematykiewicz Please check the rubocop failure on the latest push, https://buildkite.com/rails/rails/builds/107357#018f8f2d-ae4b-4d92-8ca4-579e697c5149/1253-1260 |
c1f6bf4
to
cf3fbda
Compare
Sorry about that. I've fixed it. @flavorjones |
cf3fbda
to
53b603b
Compare
FYI, the buildkite failure is unrelated:
|
53b603b
to
7f622c4
Compare
I just fixed the merge conflict on the changelog |
7f622c4
to
83cb6cb
Compare
This overlaps a little with the delegated_type declaration that was recently added. That class method accepts a Is there an opportunity to make the interfaces more consistent? Is there an opportunity for this PR to also change the |
@seanpdoyle I was thinking the exact same thing, but figured maintainers would want 2 PRs. I'd gladly add it to this one, though. |
1 similar comment
@seanpdoyle I was thinking the exact same thing, but figured maintainers would want 2 PRs. I'd gladly add it to this one, though. |
👍! I can't weigh-in on this, and definitely defer to maintainers. I just didn't want |
83cb6cb
to
8e180be
Compare
… a polymorphic belongs_to
8e180be
to
f6f46fd
Compare
@flavorjones I rebased to main again @rafaelfranca I removed support for String/Symbol and made it only support an Array of Strings and/or Symbols. Is that what you were looking for? |
This is maybe not a strongly shared opinion, but validations are meant for the user, not developers. What is a user going to do with this error? Would you go through the trouble of putting a constraint on your database for this? |
My primary use-case is for a JSON API. I do not control the UI, so I can't make it impossible for someone to try to insert wrong values. I don't want 500 errors in my error monitor because someone provided an invalid item_type. Maybe a lot of people won't find a need for this, but I have like 5 places in my Rails app that this would immediately help me. I also really like the idea of being able to see the allowed types in my model, rather than having to turn this into a Postgres enum. |
But can't you already add this validation yourself?
Without the need to add this for your specific use-case. |
That's not quite the same. If the association is non-optional, a nil "type" will provide two validation errors on one field, which in my opinion is silly. Also, I find this to be a nice developer experience improvement for a few lines of code. I've frequently wished I could tell the polymorphic association which types are allowed, just like you tell a https://api.rubyonrails.org/classes/ActiveRecord/DelegatedType.html |
As @seanpdoyle pointed out (and I was also thinking before opening the PR), it could make a lot of sense to make delegated types pass the |
This and,
Are still open questions, which I guess is the point of this PR. But to make APIs appear to be consistent not such a strong argument IMO. |
You're right though. I can and most certainly do manually add a validation. But I think that it's a nice developer experience improvement for the polymorphic association to be in charge of that validation, automatically using the appropriate foreign key column and automatically applying an Considering that |
Motivation / Background
When using a polymorphic belongs_to, I usually have a list of allowed classes that I expect this association to use. Oftentimes there's even certain views that need to handle each expected option. Unexpected types in the table would cause problems and potentially pose a security risk.
I've manually added an inclusion validator before, but wished I could just tell the association what the allowed options are.
Detail
This Pull Request changes
belongs_to
'spolymorphic
option to allow an array of Strings/Symbols of allowed model names. When provided, an inclusion validator is added to the model to ensure the_type
field is in the list of allowed class names.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]