Skip to content

Loading…

Exclude #reorder for count queries in Rails 3.1 #309

Closed
wants to merge 1 commit into from

2 participants

@pawelpacana

It turns out reorder behaves differently on Rails 3.1 and requires
explicit exclude to not pollute count query with order clause.

This is a especially a problem when reordering on aggregate column form original query, i.e.:


>> Shop.with_ticket_types.reorder('ticket_types_count').page(1)

SELECT shops.*, COUNT(product_types.id) ticket_types_count FROM `shops` INNER JOIN `product_types` ON `product_types`.`shop_id` = `shops`.`id` GROUP BY shops.id ORDER BY ticket_types_count LIMIT 30 OFFSET 0
SELECT COUNT(*) AS count_all, shops.id AS shops_id FROM `shops` INNER JOIN `product_types` ON `product_types`.`shop_id` = `shops`.`id` GROUP BY shops.id ORDER BY ticket_types_count

Mysql2::Error: Unknown column 'ticket_types_count' in 'order clause': SELECT COUNT(*) AS count_all, shops.id AS shops_id FROM `shops` INNER JOIN `product_types` ON `product_types`.`shop_id` = `shops`.`id` GROUP BY shops.id ORDER BY ticket_types_count
@pawelpacana pawelpacana Exclude reorder for count queries.
It turns out reorder behaves differently on Rails 3.1 and requires
explicit exclude to not pollute count query with order clause.
afec2ff
@mislav
Owner

Thanks! pulled

@mislav mislav closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 15, 2013
  1. @pawelpacana

    Exclude reorder for count queries.

    pawelpacana committed
    It turns out reorder behaves differently on Rails 3.1 and requires
    explicit exclude to not pollute count query with order clause.
Showing with 41 additions and 36 deletions.
  1. +5 −5 lib/will_paginate/active_record.rb
  2. +36 −31 spec/finders/active_record_spec.rb
View
10 lib/will_paginate/active_record.rb
@@ -5,10 +5,10 @@
module WillPaginate
# = Paginating finders for ActiveRecord models
- #
+ #
# WillPaginate adds +paginate+, +per_page+ and other methods to
# ActiveRecord::Base class methods and associations.
- #
+ #
# In short, paginating finders are equivalent to ActiveRecord finders; the
# only difference is that we start with "paginate" instead of "find" and
# that <tt>:page</tt> is required parameter:
@@ -80,7 +80,7 @@ def total_entries
def count
if limit_value
- excluded = [:order, :limit, :offset]
+ excluded = [:order, :limit, :offset, :reorder]
excluded << :includes unless eager_loading?
rel = self.except(*excluded)
# TODO: hack. decide whether to keep
@@ -184,7 +184,7 @@ module BaseMethods
# +per_page+.
#
# Example:
- #
+ #
# @developers = Developer.paginate_by_sql ['select * from developers where salary > ?', 80000],
# :page => params[:page], :per_page => 3
#
@@ -192,7 +192,7 @@ module BaseMethods
# supply <tt>:total_entries</tt>. If you experience problems with this
# generated SQL, you might want to perform the count manually in your
# application.
- #
+ #
def paginate_by_sql(sql, options)
pagenum = options.fetch(:page) { raise ArgumentError, ":page parameter required" } || 1
per_page = options[:per_page] || self.per_page
View
67 spec/finders/active_record_spec.rb
@@ -6,22 +6,22 @@
abort unless ActiverecordTestConnector.able_to_connect
describe WillPaginate::ActiveRecord do
-
+
extend ActiverecordTestConnector::FixtureSetup
-
+
fixtures :topics, :replies, :users, :projects, :developers_projects
-
+
it "should integrate with ActiveRecord::Base" do
ActiveRecord::Base.should respond_to(:paginate)
end
-
+
it "should paginate" do
lambda {
users = User.paginate(:page => 1, :per_page => 5).to_a
users.length.should == 5
}.should run_queries(2)
end
-
+
it "should fail when encountering unknown params" do
lambda {
User.paginate :foo => 'bar', :page => 1, :per_page => 4
@@ -151,7 +151,7 @@
topics.should_not be_empty
}.should run_queries(1)
end
-
+
it "support empty? for grouped queries" do
topics = Topic.group(:project_id).paginate :page => 1, :per_page => 3
lambda {
@@ -200,11 +200,16 @@
Developer.group(:salary).page(1).total_entries.should == 4
end
+ it "removes :reorder for count with group" do
+ Project.group(:id).reorder(:id).page(1).total_entries
+ $query_sql.last.should_not =~ /\ORDER\b/
+ end
+
it "should not have zero total_pages when the result set is empty" do
Developer.where("1 = 2").page(1).total_pages.should == 1
end
end
-
+
it "should not ignore :select parameter when it says DISTINCT" do
users = User.select('DISTINCT salary').paginate :page => 2
users.total_entries.should == 5
@@ -262,11 +267,11 @@
options = { :page => 1 }
options.expects(:delete).never
options_before = options.dup
-
+
Topic.paginate(options)
options.should == options_before
end
-
+
it "should get first page of Topics with a single query" do
lambda {
result = Topic.paginate :page => nil
@@ -276,7 +281,7 @@
result.size.should == 4
}.should run_queries(1)
end
-
+
it "should get second (inexistent) page of Topics, requiring 2 queries" do
lambda {
result = Topic.paginate :page => 2
@@ -284,13 +289,13 @@
result.should be_empty
}.should run_queries(2)
end
-
+
it "should paginate with :order" do
result = Topic.paginate :page => 1, :order => 'created_at DESC'
result.should == topics(:futurama, :harvey_birdman, :rails, :ar).reverse
result.total_pages.should == 1
end
-
+
it "should paginate with :conditions" do
result = Topic.paginate :page => 1, :order => 'id ASC',
:conditions => ["created_at > ?", 30.minutes.ago]
@@ -300,14 +305,14 @@
it "should paginate with :include and :conditions" do
result = Topic.paginate \
- :page => 1,
- :include => :replies,
- :conditions => "replies.content LIKE 'Bird%' ",
+ :page => 1,
+ :include => :replies,
+ :conditions => "replies.content LIKE 'Bird%' ",
:per_page => 10
- expected = Topic.find :all,
- :include => 'replies',
- :conditions => "replies.content LIKE 'Bird%' ",
+ expected = Topic.find :all,
+ :include => 'replies',
+ :conditions => "replies.content LIKE 'Bird%' ",
:limit => 10
result.should == expected
@@ -321,27 +326,27 @@
:order => 'replies.created_at asc, topics.created_at asc').to_a
}.should run_queries(2)
- expected = Topic.find :all,
- :include => 'replies',
- :order => 'replies.created_at asc, topics.created_at asc',
+ expected = Topic.find :all,
+ :include => 'replies',
+ :order => 'replies.created_at asc, topics.created_at asc',
:limit => 10
result.should == expected
result.total_entries.should == 4
end
-
+
describe "associations" do
it "should paginate with include" do
project = projects(:active_record)
result = project.topics.paginate \
- :page => 1,
- :include => :replies,
+ :page => 1,
+ :include => :replies,
:conditions => ["replies.content LIKE ?", 'Nice%'],
:per_page => 10
- expected = Topic.find :all,
- :include => 'replies',
+ expected = Topic.find :all,
+ :include => 'replies',
:conditions => ["project_id = ? AND replies.content LIKE ?", project.id, 'Nice%'],
:limit => 10
@@ -368,7 +373,7 @@
lambda {
dhh.projects.find(:all, :order => 'projects.id', :limit => 4)
}.should_not raise_error
-
+
result = dhh.projects.paginate(:page => 1, :per_page => 4).reorder('projects.id')
result.should == expected_id_ordered
@@ -390,7 +395,7 @@
}.should run_queries(1)
end
end
-
+
it "should paginate with joins" do
result = nil
join_sql = 'LEFT JOIN developers_projects ON users.id = developers_projects.developer_id'
@@ -480,7 +485,7 @@
Developer.paginate :readonly => true, :page => 1
}.should_not raise_error
end
-
+
it "should not paginate an array of IDs" do
lambda {
Developer.paginate((1..8).to_a, :per_page => 3, :page => 2, :order => 'id')
@@ -493,9 +498,9 @@
Project.page(307445734561825862)
}.should raise_error(WillPaginate::InvalidPage, "invalid offset: 9223372036854775830")
end
-
+
protected
-
+
def ignore_deprecation
ActiveSupport::Deprecation.silence { yield }
end
Something went wrong with that request. Please try again.