Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

total_count gives wrong result after a calling map on an empty AR::Relation instance #845

Closed
denislins opened this issue Jan 2, 2017 · 10 comments

Comments

@denislins
Copy link

denislins commented Jan 2, 2017

Hey guys,

I think I've found a very weird bug. It appears that calling #map on a activerecord relation instance breaks #total_count, but only when the relation is empty. I've successfully reproduced it using both PostgreSQL and SQLite in their latest versions.

You can get a better vision of the issue in the code below:

def get_coupons(request, options)
  coupons = coupons(request)

  CouponsResponse.new(
    coupons: parsed_coupons(coupons), total: coupons.total_count,
    results_per_page: 10)
end

private

def coupons(request)
  Coupon.valid_at(request.valid_from).page(request.page).per(10)
end

def parsed_coupons(coupons)
  coupons.map do |coupon|
    ...
  end
end

The expected result is:

{ coupons: [...], total: 2, results_per_page: 10 }

While the actual result is:

{ coupons: [...], total: 10, results_per_page: 10 }

I tried searching the issues but didn't find anything related. Please forgive me if this is a known issue, or if it's my fault and therefore not an issue at all.

@yuki24
Copy link
Member

yuki24 commented Jan 2, 2017

@denislins Could you provide an example with a failing test using this bug report template?

@denislins
Copy link
Author

@yuki24 I tried reproducing the bug using the template, but failed. The issue persists on my application though. I'm currently reading the source to better understand the issue. I'll keep you posted.

@denislins
Copy link
Author

denislins commented Jan 3, 2017

@yuki24 I found the difference between my app and the code in the report template.

When I make a call to #each or to #map, the query is executed as usual, in both scenarios. However, when I call #total_count after, my application doesn't execute another query, whereas the code in the report template will.

For some reason, it appears that the @total_count instance variable is being set with a wrong value when calling #each or #map, and thus returning this value when called.

Also, in both scenarios, if I call #total_count before calling the other methods, the result is always correct.

My application doesn't use Rails, so I thought maybe my setup is the problem. I see that in the bug report template the gem is loaded using Kaminari::Hooks.init, while in my application I'm using this code:

require 'kaminari/core'
require 'kaminari/activerecord'

Could this be the issue?

@yuki24
Copy link
Member

yuki24 commented Jan 3, 2017

It seems like you are using the master branch on GitHub in your app. You'd have to change the gemfile(true) do ... end to use the same version of kaminari in the script. Also, feel free to change the table definitions, models and tests if necessary to reproduce the issue.

@denislins
Copy link
Author

Hey @yuki24, I managed to replicate the bug using the template. You can find it here: https://gist.github.com/denislins/59578a0a9151dedce8a8debd9ff3883f

The problem is that I require kaminari-activerecord in my app, not kaminari. Doing that actually fetches the beta version, as my Gemfile.lock shows:

    kaminari-activerecord (1.0.0.rc1)
      activerecord
      kaminari-core (= 1.0.0.rc1)

When using only kaminari, the download version is the 0.17, thus making the codebase different.

I usually require vendor specific gems when available, as it decreases the size and codebase of my dependencies. Which raises the question: should I require kaminari as a whole in my Gemfile?

@amatsuda
Copy link
Member

amatsuda commented Jan 4, 2017

@denislins Thank you for finding and reporting this! This was obviously a newly introduced bug in the next 1.0.0 release, and it's been fixed via 11d4aaf.
Feel free to raise another issue if you're still seeing any problem.
Thank you again for making our first stable release better :)

@denislins
Copy link
Author

Sure @amatsuda, happy to help :)

I'm still a bit lost, though. How should I proceed with my application? Should I require the gem as a whole, or is there a way to require the 0.17 version of the kaminari-activerecord gem?

I tried specifying the version as usual but it didn't work.

@yuki24
Copy link
Member

yuki24 commented Jan 21, 2017

@denislins Sorry for my late reply. As of the version 1.0.0, we've split up the gem into small gems. We used to organize all the files in a single gem (kaminari) until 0.17.0, and thus 0.17.0 of kaminari-activerecord doesn't exist. Starting 1.0.0, you'd have to require individual gems depending on the framework or the ORM you are using. For more details, please read 5 Major Changes in Kaminari 1.0.0. Thanks!

@MoonShining
Copy link

I use 1.1.1 in my rails app, bug still exist, but only when the page is 2

page 1、3、4 run the count sql and get correct total_count

page 2 skip the sql, thus total_count is wrong

@yuki24
Copy link
Member

yuki24 commented Apr 10, 2018

@MoonShining please provide example code or a test case that replicates your issue and file a new ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants