Fix count method to work with select statement.

[edariedl]

count method raises ActiveRecord::StatementInvalid if select method is used in Rails 4.1. This commit fixes will paginate and if work if select method is used.

[cyrusstoller]

+1

[simonmorley]

👍

[quek]

👍

[tkuisma]

+1

[frenkel]

Works great, thanks!

[ledermann]

👍

[O-I]

Awesome!

[mhenrixon]

👍

[cyrusstoller]

I'm guessing test coverage is the reason this hasn't been merged.

[edariedl]

Yes, it is possible, I had an issue with installing mysql gem, so I cannot run the tests. But it doesn't matter. It looks, that this error was already fixed in ActiveRecord so the fix of will_paginate should not be needed anymore after Rails 4.1.1 release.

From ActiveRecord changelog:

Fixed error for aggregate methods (empty?, any?, count) with select which created invalid SQL.

Fixes #13648.

Simon Woker

[O-I]

Can anyone confirm that updating to Rails 4.1.1 fixes this issue? I still get the ActiveRecord::StatementInvalid error after updating.

[edariedl]

According to blog Rails 4.1.1 is only security fix and includes only code related to security issues. It does not fixes this issue.

[O-I]

OK. Thanks for the clarification.

[aperkins81]

FYI, bug still exists using 4.1.2.rc1 (rails, active_, action_)

[nathany]

#303 is a cleaner fix for the count method.

There is still an issue with calls to count within will_paginate itself, which need to be count(:all) in Rails 4.1.

[edariedl]

Yes, maybe it is a little cleaner, I just took params from rails count() method but it still does not support select statement. And I really don' know if it is cleaner to write everywhere count(:all) or if count itself should handle this. I really have no idea :).

[nathany]

Yah, I don't know either. :-) Also getting other fun test failures when trying to run specs agains Rails 4.1.1. 😦

[edariedl]

Yes I have some issue with installing mysql gem, maybe its because I don't have installed MySQL database. So I even cannot run specs :).

[edariedl]

But it's still weird Rails 4.1.1 should be only security fix.

[nathany]

The Gemfile was locked at 4.0.0rc2, so this is 4.1 in general that the specs aren't running on. Lots of stuff with the fixture. Trying to get it working atm.

[edariedl]

Yes, it makes sense, also error fixed in this pull request was created by Rails 4.1.0.

[nathany]

Maybe this commit rails/rails@afd4d82 in Rails 4.1.2 will fix things. rails/rails#13648. I'll try the rc in a minute.

[nathany]

nope.

[edariedl]

I thought, according to change log, it will fix it, but after looking into source code. They just fixed size and any? methods. count will be still broken.

[nathany]

Maybe total_entries should use size instead of count. I'm not sure. Waiting for @mislav.

[edariedl]

IMHO you cannot use size, if you look into method definition:

def size
  loaded? ? @records.length : count(:all)
end

If you have loaded records from database, which is most of the time, if you have pagination on the bottom of the page. It will count only your loaded records, not all of them.

And this is also the reason for redefinition of count method in will_paginate:

def count
  if limit_value
    excluded = [:order, :limit, :offset, :reorder]
    excluded << :includes unless eager_loading?
    rel = self.except(*excluded)
    # TODO: hack. decide whether to keep
    rel = rel.apply_finder_options(@wp_count_options) if defined? @wp_count_options
    rel.count
  else
    super
  end
end

On the third line excluded = [:order, :limit, :offset, :reorder] it defines all methods used by will_paginate and removes it before original count is called. If you use only size, you get back only per_page value almost every time.

[mislav]

@edariedl Sorry for the late reply. See my comment in #348 (comment)

This fix looks simple enough, but I will need a test case for two reasons:

1. A failing test would confirm that this bug exists
2. The test should pass with this fix on our CI (Rails versions 3.0 – 4.2. alpha)

Otherwise I can't risk breaking will_paginate for older Rails versions.

[nathany]

Afaik, this is resolved with 0c6d08e

[temochka]

@nathany no, I don’t think so. It’s basically a problem in ActiveRecord, but it isn’t manageable when using will_paginate (since calls to #count are implicit). Example (ActiveRecord 4.1 & will_paginate 3.0.5):

User.select(:login, :created_at).count
# Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' created_at) FROM `users`' at line 1: SELECT COUNT(login, created_at) FROM `users`

# In my code I can call
User.select(:login, :created_at).except(:select).count

# or
User.select(:login, :created_at).count(:login)

# When using will_paginate, I can’t
User.select(:login, :created_at).limit(5).page(2).total_pages
# Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds ... (same as above)

I checked both 3.0.5 and 0c6d08e.

[nathany]

@temochka Oh. We should probably start with a failing spec like #389. Now that the specs are running/passing with Rails 4.1 (on master), that should be fairly trivial.

[edariedl]

@mislav It's ok and I agree that test should be here from the beginning. I add the test and according to CI, everything looks fine for all required versions of rails.

[nathany]

@edariedl Have you merged master to your branch? See #387.

I added a very similar spec at #389, which may not be needed if you get this working. :-D

[edariedl]

@nathany Yes, I merged master to the branch. Nice thing to see passing tests and they are broken. I went through all the tests on the Travis CI and everything looks fine, except some deprecations from used libraries.

[nathany]

this looks very much like the code removed in rails/rails#10710, which I suppose is as good a solution as any.

[nathany]

Thanks @edariedl. I'm looking forward to the next will_paginate release and Rails 4.1. Whoot!

[mislav]

Gonna trust all of your's judgement and go ahead with this. Thanks for the help!

[nathany]

Tomorrow I can give master another try from our app to see if all is well before uploading a new gem. Thanks @mislav.

[nathany]

@mislav All is working well here. When are you planning a new gem release?

[nathany]

@mislav Is this fix included in the "3.0.6" gem release? I'm seeing errors when using total_entries since switching from master to 3.0.6.

[mislav]

@nathany Seems like I forgot to backport this to 3-0-stable, I'm sorry. Cutting a v3.0.7 as we speak

[nathany]

No worries. Thanks @mislav!