Rails core - Aug 2012 - 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.

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.

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.

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.

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.

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.