Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Adding Table Context to Order Hash #12610

Open
wants to merge 1 commit into from

4 participants

@prpetten

This commit fixes #12604

Prepend original table name to order hash

With the models

    class Post
      has_many :comments
      scope :dewey, -> { order(subject: :desc) }
    end

    class Comment
      belongs_to :post
    end

And records

    p1 = Post.create(subject: 'Awesomeness')
    p2 = Post.create(subject: 'Zaniness')
    c1 = Comment.create(type: 'Cool', post_id: p1.id)
    c2 = Comment.create(type: 'Cool', post_id: p2.id)

The call

    Comment.where(type: 'Cool').joins(:post).merge(Post.dewey)

Should yield

    [c2, c1]
paulp@devicemagic.com Adding Table Context to Order Hash
This commit fixes #12604

Prepend original table name to order hash

    With the models

        class Post
          has_many :comments
          scope :dewey, -> { order(subject: :desc) }
        end

        class Comment
          belongs_to :post
        end

    And records

        p1 = Post.create(subject: 'Awesomeness')
        p2 = Post.create(subject: 'Zaniness')
        c1 = Comment.create(type: 'Cool', post_id: p1.id)
        c2 = Comment.create(type: 'Cool', post_id: p2.id)

    The call

        Comment.where(type: 'Cool').joins(:post).merge(Post.dewey)

    Should yield

        [c2, c1]
03e8964
@prpetten

This is my first pull request into rails core, and I'd be happy to hear any feedback.

NB: Apologies the previous pull request isn't showing up as my github account, although that email should be associated with my account.

@rafaelfranca rafaelfranca commented on the diff
activerecord/test/models/post.rb
@@ -19,6 +19,7 @@ def greeting
scope :containing_the_letter_a, -> { where("body LIKE '%a%'") }
scope :ranked_by_comments, -> { order("comments_count DESC") }
+ scope :ranked_by_comments_hash, -> { order(comments_count: :desc) }
@rafaelfranca Owner

Maybe we could add a new test case for hashes with multiple elements. WDYT?

Makes sense to me, and would probably break my fix, assuming you mean:

scope :ranked_by_comments_hash, -> { order(comments_count: :desc, subject: :asc) }

@rafaelfranca Owner

Yes. this is what I'm meaning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pftg pftg commented on the diff
activerecord/CHANGELOG.md
@@ -862,4 +862,34 @@
*Slava Markevich*
+* Prepend original table name to order hash
@pftg
pftg added a note

Please move this changelog entry to the first line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pftg

:+1: also, should table name auto-added to references? Please review: https://github.com/prpetten/rails/blob/03e8964c7e0d1c370e924cc93e1fb73c3158ee9b/activerecord/lib/active_record/relation/query_methods.rb#L1036

And one moment is not clear for me: why you are expecting only one field?

@prpetten
@rafaelfranca rafaelfranca modified the milestone: 4.0.5, 4.0.4, 4.0.6
@rafaelfranca rafaelfranca modified the milestone: 4.0.10, 4.1.7
@rafaelfranca rafaelfranca modified the milestone: 4.1.9, 4.0.13
@rafaelfranca rafaelfranca modified the milestone: 4.0.13, 4.1.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 23, 2013
  1. Adding Table Context to Order Hash

    paulp@devicemagic.com authored
    This commit fixes #12604
    
    Prepend original table name to order hash
    
        With the models
    
            class Post
              has_many :comments
              scope :dewey, -> { order(subject: :desc) }
            end
    
            class Comment
              belongs_to :post
            end
    
        And records
    
            p1 = Post.create(subject: 'Awesomeness')
            p2 = Post.create(subject: 'Zaniness')
            c1 = Comment.create(type: 'Cool', post_id: p1.id)
            c2 = Comment.create(type: 'Cool', post_id: p2.id)
    
        The call
    
            Comment.where(type: 'Cool').joins(:post).merge(Post.dewey)
    
        Should yield
    
            [c2, c1]
This page is out of date. Refresh to see the latest.
View
30 activerecord/CHANGELOG.md
@@ -862,4 +862,34 @@
*Slava Markevich*
+* Prepend original table name to order hash
@pftg
pftg added a note

Please move this changelog entry to the first line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ With the models
+
+ class Post
+ has_many :comments
+ scope :dewey, -> { order(subject: :desc) }
+ end
+
+ class Comment
+ belongs_to :post
+ end
+
+ And records
+
+ p1 = Post.create(subject: 'Awesomeness')
+ p2 = Post.create(subject: 'Zaniness')
+ c1 = Comment.create(type: 'Cool', post_id: p1.id)
+ c2 = Comment.create(type: 'Cool', post_id: p2.id)
+
+ The call
+
+ Comment.where(type: 'Cool').joins(:post).merge(Post.dewey)
+
+ Should yield
+
+ [c2, c1]
+
+ *Paul Pettengill*
+
Please check [4-0-stable](https://github.com/rails/rails/blob/4-0-stable/activerecord/CHANGELOG.md) for previous changes.
View
21 activerecord/lib/active_record/relation/query_methods.rb
@@ -1012,7 +1012,7 @@ def build_order(arel)
when Symbol
table[order].asc
when Hash
- order.map { |field, dir| table[field].send(dir) }
+ order.values[0] == :asc ? table[order].asc : table[order].desc
else
order
end
@@ -1037,9 +1037,24 @@ def preprocess_order_args(order_args)
references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
references!(references) if references.any?
- # if a symbol is given we prepend the quoted table name
+ # if a symbol or hash is given we prepend the quoted table name
order_args.map! do |arg|
- arg.is_a?(Symbol) ? Arel::Nodes::Ascending.new(klass.arel_table[arg]) : arg
+ if arg.is_a?(Symbol)
+ prepend_table_sort(arg, :asc)
+ elsif arg.is_a?(Hash)
+ prepend_table_sort(arg.keys[0], arg.values[0])
+ else
+ arg
+ end
+ end
+ end
+
+ def prepend_table_sort(field, direction)
+ case direction
+ when :asc
+ Arel::Nodes::Ascending.new(klass.arel_table[field])
+ when :desc
+ Arel::Nodes::Descending.new(klass.arel_table[field])
end
end
View
19 activerecord/test/cases/scoping/relation_scoping_test.rb
@@ -206,6 +206,25 @@ def test_merge_inner_scope_has_priority
end
end
+ def test_merge_order_hash
+ merge_sql = Comment.where(type: "Comment").joins(:post).
+ merge(Post.ranked_by_comments_hash).to_sql
+ assert_match '"posts"."comments_count" DESC', merge_sql
+
+ merge_results = Comment.where(type: "Comment").joins(:post).
+ merge(Post.ranked_by_comments_hash).
+ order("comments.id")
+ c1 = Comment.joins(:post).find(1)
+ c2 = Comment.joins(:post).find(2)
+ c8 = Comment.joins(:post).find(8)
+ c9 = Comment.joins(:post).find(9)
+ assert_equal merge_results.size, 4
+ assert_equal merge_results.first, c8
+ assert_equal merge_results.limit(2).last, c9
+ assert_equal merge_results.limit(3).last, c1
+ assert_equal merge_results.last, c2
+ end
+
def test_replace_options
Developer.where(:name => 'David').scoping do
Developer.unscoped do
View
2  activerecord/test/fixtures/posts.yml
@@ -28,6 +28,7 @@ authorless:
sti_comments:
id: 4
author_id: 1
+ comments_count: 4
title: sti comments
body: hello
type: Post
@@ -35,6 +36,7 @@ sti_comments:
sti_post_and_comments:
id: 5
author_id: 1
+ comments_count: 3
title: sti me
body: hello
type: StiPost
View
3  activerecord/test/models/post.rb
@@ -19,6 +19,7 @@ def greeting
scope :containing_the_letter_a, -> { where("body LIKE '%a%'") }
scope :ranked_by_comments, -> { order("comments_count DESC") }
+ scope :ranked_by_comments_hash, -> { order(comments_count: :desc) }
@rafaelfranca Owner

Maybe we could add a new test case for hashes with multiple elements. WDYT?

Makes sense to me, and would probably break my fix, assuming you mean:

scope :ranked_by_comments_hash, -> { order(comments_count: :desc, subject: :asc) }

@rafaelfranca Owner

Yes. this is what I'm meaning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
scope :limit_by, lambda {|l| limit(l) }
@@ -40,6 +41,8 @@ def first_comment
scope :with_comments, -> { preload(:comments) }
scope :with_tags, -> { preload(:taggings) }
+ scope :comment, -> { order(type: :desc) }
+
has_many :comments do
def find_most_recent
order("id DESC").first
Something went wrong with that request. Please try again.