Peter Brown
2012-Aug-07 12:23 UTC
Looking for feedback and review of changes to AR predicate builder
Hi,
Guillermo asked me to post this here for
feedback: https://github.com/rails/rails/pull/7273
The pull request has some examples and rationale for the change, but
basically it would let you *specify the model name in queries when
you''d
normally use a foreign key*.
These queries would then be equivalent:
Post.where(:author_id => author)
Post.where(:author => author)
and so would these:
Author.where(:posts => {:author_id => author}).joins(:posts)
Author.where(:posts => {:author => author}).joins(:posts)
TL;DR
- API is more consistent with other parts of ActiveRecord
- Works with polymorphic relationships
- Better for legacy schemas that don''t follow Rails''
foreign key
conventions (you don''t need to remember column names)
--
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
Josh Susser
2012-Aug-07 14:28 UTC
Re: Looking for feedback and review of changes to AR predicate builder
I think this makes a lot of sense. I tried to do this for the old find_by_x dynamic finders a few years ago but was thwarted by the amount of refactoring it would require. Using association names instead of the underlying foreign key is more natural and keeps things at the right level of abstraction, and is especially helpful for polymorphic associations. If the new ARel-based query API enables this to be done without changing a bunch of other things, I''m all for it. -- Josh Susser On Tuesday, August 7, 2012 at 8:23 AM, Peter Brown wrote:> Hi, > > Guillermo asked me to post this here for feedback: https://github.com/rails/rails/pull/7273 > > The pull request has some examples and rationale for the change, but basically it would let you specify the model name in queries when you''d normally use a foreign key. > > These queries would then be equivalent: > > Post.where(:author_id => author) > Post.where(:author => author) > > and so would these: > > Author.where(:posts => {:author_id => author}).joins(:posts) > Author.where(:posts => {:author => author}).joins(:posts) > > TL;DR > API is more consistent with other parts of ActiveRecord > Works with polymorphic relationships > Better for legacy schemas that don''t follow Rails'' foreign key conventions (you don''t need to remember column names) > > > > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. > To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ. > To post to this group, send email to rubyonrails-core@googlegroups.com (mailto:rubyonrails-core@googlegroups.com). > To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com (mailto:rubyonrails-core+unsubscribe@googlegroups.com). > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
thiagocifani
2012-Aug-07 14:32 UTC
Re: Looking for feedback and review of changes to AR predicate builder
Great! But is it still returning a Arel Relation or a array like in dynamic finds? 2012/8/7 Josh Susser <josh@hasmanythrough.com>> I think this makes a lot of sense. I tried to do this for the old > find_by_x dynamic finders a few years ago but was thwarted by the amount of > refactoring it would require. Using association names instead of the > underlying foreign key is more natural and keeps things at the right level > of abstraction, and is especially helpful for polymorphic associations. If > the new ARel-based query API enables this to be done without changing a > bunch of other things, I''m all for it. > > -- > Josh Susser > > On Tuesday, August 7, 2012 at 8:23 AM, Peter Brown wrote: > > Hi, > > Guillermo asked me to post this here for feedback: > https://github.com/rails/rails/pull/7273 > > The pull request has some examples and rationale for the change, but > basically it would let you *specify the model name in queries when you''d > normally use a foreign key*. > > These queries would then be equivalent: > > Post.where(:author_id => author) > Post.where(:author => author) > > and so would these: > > Author.where(:posts => {:author_id => author}).joins(:posts) > Author.where(:posts => {:author => author}).joins(:posts) > > TL;DR > > - API is more consistent with other parts of ActiveRecord > - Works with polymorphic relationships > - Better for legacy schemas that don''t follow Rails'' foreign key > conventions (you don''t need to remember column names) > > -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To view this discussion on the web visit > https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to > rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. > > > -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to > rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. >-- thiagocifani http://about.me/thiagocifani <http://del.icio.us/thiagocifani> <http://del.icio.us/thiagocifani> -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Peter Brown
2012-Aug-07 14:50 UTC
Re: Looking for feedback and review of changes to AR predicate builder
Somebody pointed out in the pull request that it doesn''t set the polymorphic type column, so it only solves half that problem. I''m a little curious what it would take to get it to fully work with polymorphic relationships, but maybe that''s outside the scope of this change. On Tuesday, August 7, 2012 8:23:25 AM UTC-4, Peter Brown wrote:> > Hi, > > Guillermo asked me to post this here for feedback: > https://github.com/rails/rails/pull/7273 > > The pull request has some examples and rationale for the change, but > basically it would let you *specify the model name in queries when you''d > normally use a foreign key*. > > These queries would then be equivalent: > > Post.where(:author_id => author) > Post.where(:author => author) > > and so would these: > > Author.where(:posts => {:author_id => author}).joins(:posts) > Author.where(:posts => {:author => author}).joins(:posts) > > TL;DR > > - API is more consistent with other parts of ActiveRecord > - Works with polymorphic relationships > - Better for legacy schemas that don''t follow Rails'' foreign key > conventions (you don''t need to remember column names) > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/9LkJhKUjWXIJ. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Peter Brown
2012-Aug-08 01:40 UTC
Re: Looking for feedback and review of changes to AR predicate builder
Josh, I appreciate the feedback. The consensus was that adding polymorphic support would have the most value so I went ahead and added support for the cases I could think of. I was hoping you could take a look at the test cases<https://github.com/beerlington/rails/blob/0df2f082005ce0f57a544c30c646d1bae45bbcf8/activerecord/test/cases/relation/where_test.rb#L24-50> I wrote and see if they''re what you had in mind. Thanks! - Pete On Tuesday, August 7, 2012 10:28:04 AM UTC-4, Josh Susser wrote:> > I think this makes a lot of sense. I tried to do this for the old > find_by_x dynamic finders a few years ago but was thwarted by the amount of > refactoring it would require. Using association names instead of the > underlying foreign key is more natural and keeps things at the right level > of abstraction, and is especially helpful for polymorphic associations. If > the new ARel-based query API enables this to be done without changing a > bunch of other things, I''m all for it. > > -- > Josh Susser > > On Tuesday, August 7, 2012 at 8:23 AM, Peter Brown wrote: > > Hi, > > Guillermo asked me to post this here for feedback: > https://github.com/rails/rails/pull/7273 > > The pull request has some examples and rationale for the change, but > basically it would let you *specify the model name in queries when you''d > normally use a foreign key*. > > These queries would then be equivalent: > > Post.where(:author_id => author) > Post.where(:author => author) > > and so would these: > > Author.where(:posts => {:author_id => author}).joins(:posts) > Author.where(:posts => {:author => author}).joins(:posts) > > TL;DR > > - API is more consistent with other parts of ActiveRecord > - Works with polymorphic relationships > - Better for legacy schemas that don''t follow Rails'' foreign key > conventions (you don''t need to remember column names) > > -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To view this discussion on the web visit > https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ. > To post to this group, send email to rubyonra...@googlegroups.com<javascript:> > . > To unsubscribe from this group, send email to > rubyonrails-co...@googlegroups.com <javascript:>. > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. > > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/7KVzezeMguAJ. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Peter Brown
2012-Aug-10 05:49 UTC
Re: Looking for feedback and review of changes to AR predicate builder
I added a few more test cases to account for polymorphic STI associations, so it should be ready for re-review. - Pete On Tuesday, August 7, 2012 9:40:11 PM UTC-4, Peter Brown wrote:> > Josh, > > I appreciate the feedback. The consensus was that adding polymorphic > support would have the most value so I went ahead and added support for the > cases I could think of. I was hoping you could take a look at the test > cases<https://github.com/beerlington/rails/blob/0df2f082005ce0f57a544c30c646d1bae45bbcf8/activerecord/test/cases/relation/where_test.rb#L24-50> I > wrote and see if they''re what you had in mind. > > Thanks! > > - Pete > > On Tuesday, August 7, 2012 10:28:04 AM UTC-4, Josh Susser wrote: >> >> I think this makes a lot of sense. I tried to do this for the old >> find_by_x dynamic finders a few years ago but was thwarted by the amount of >> refactoring it would require. Using association names instead of the >> underlying foreign key is more natural and keeps things at the right level >> of abstraction, and is especially helpful for polymorphic associations. If >> the new ARel-based query API enables this to be done without changing a >> bunch of other things, I''m all for it. >> >> -- >> Josh Susser >> >> On Tuesday, August 7, 2012 at 8:23 AM, Peter Brown wrote: >> >> Hi, >> >> Guillermo asked me to post this here for feedback: >> https://github.com/rails/rails/pull/7273 >> >> The pull request has some examples and rationale for the change, but >> basically it would let you *specify the model name in queries when you''d >> normally use a foreign key*. >> >> These queries would then be equivalent: >> >> Post.where(:author_id => author) >> Post.where(:author => author) >> >> and so would these: >> >> Author.where(:posts => {:author_id => author}).joins(:posts) >> Author.where(:posts => {:author => author}).joins(:posts) >> >> TL;DR >> >> - API is more consistent with other parts of ActiveRecord >> - Works with polymorphic relationships >> - Better for legacy schemas that don''t follow Rails'' foreign key >> conventions (you don''t need to remember column names) >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Ruby on Rails: Core" group. >> To view this discussion on the web visit >> https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ. >> To post to this group, send email to rubyonra...@googlegroups.com. >> To unsubscribe from this group, send email to >> rubyonrails-co...@googlegroups.com. >> For more options, visit this group at >> http://groups.google.com/group/rubyonrails-core?hl=en. >> >> >>-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/9tmr_VOzqTQJ. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.