ActiveRecord Select statement returns negative sum #14337

Closed
HangingClowns opened this Issue Mar 10, 2014 · 12 comments

Comments

Projects
None yet
6 participants

I was trying to find a way to kind of "cache" a select statement when I pull models from the database and ran into a strange bug where my summed data was returning a negative value, when it should be a positive value:

class WineListing
  belongs_to :stock_wine

  def quantity
    self[:quantity] || self.stock_wine.stock_moves.sum(:quantity)
  end
end

class StockWine
  has_many :stock_moves

  def quantity
    self[:quantity] || self.stock_moves.sum(:quantity)
  end
end

class StockMove
  belongs_to :stock_wine
end

describe StockWine
  context "get quantity" do # for stock wine, works, but only after i moved around some values
    before :each do
      @stock_wine = FactoryGirl.create(:stock_wine)
      FactoryGirl.create(:stock_move, product:@stock_wine, quantity: 2, move_type:StockMove.purchase, move_date:Time.now)
      FactoryGirl.create(:stock_move, product:@stock_wine, quantity: 1, move_type:StockMove.sale, move_date:Time.now)
    end
    it "should have a virtual attribute of quantity" do
      expect(@stock_wine.quantity).to eq 1
    end
    it "should use selected attribute over calculation" do
      wine = StockWine.joins(:stock_moves).where(id:@stock_wine.id).select('sum(stock_moves.quantity) as quantity').order('quantity').first
      expect(wine).to_not receive :stock_wines
      expect(wine.quantity).to eq @stock_wine.quantity #passes, after some moving around of the calls (weirdly, enough)
    end
  end
end

describe WineListing do
  context "get quantity" do # doesn't work at all
    before :each do
      wine = FactoryGirl.create(:stock_wine)
      @listing = FactoryGirl.create(:wine_listing,stock_wine:wine, user:wine.user)
      FactoryGirl.create(:stock_move, product:wine, quantity: 2, move_type:StockMove.purchase, move_date:Time.now) # purchases are positive values
      FactoryGirl.create(:stock_move, product:wine, quantity: 1, move_type:StockMove.sale, move_date:Time.now) # callback on the stock move to change sales into negative values (as it's a stock out)
    end
    it "should have a virtual attribute of quantity" do
      expect(@listing.quantity).to eq 1
    end
    it "should be able to use the selected attribute before it does it's own query" do
      pending
      listing = WineListing.joins(stock_wine: :stock_moves).where(id:@listing.id).select("sum(stock_moves.quantity) as quantity").group('wine_listings.id, quantity').first
      expect(listing).to_not receive :stock_wine
      expect(listing.quantity).to eq @listing.quantity
    end
  end
end

Here's the error I get from RSpec:

expected: #<BigDecimal:7feed487dfa0,'0.1E1',9(18)>                                                           
            got: #<BigDecimal:7feed487b110,'-0.1E1',9(18)>

Looks like I"m getting a negative value brought in (which I verified by checking the attributes using raise .inspect). Any idea why this is happening? Is there a way I can kind of "cache" this with a sql select? Is this a bug? The SQL Query looked fine when I ran it.

Contributor

pftg commented Mar 10, 2014

What rails version did you use?

Sorry about that. Rails 4.0.3 on ruby 2.1.1

Member

robin850 commented Mar 22, 2014

Hello @HangingClowns,

Could you please provide an executable gist to showcase your problem ? We do not use RSpec nor Factory Girl. Thank you for reporting anyway!

So what kind of format do you want? I can see if I can replicate it
somehow.
On Mar 22, 2014 11:41 PM, "Robin Dupret" notifications@github.com wrote:

Hello @HangingClowns https://github.com/HangingClowns,

Could you please provide an executable gisthttps://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_gem.rbto showcase your problem ? We do not use RSpec nor Factory Girl. Thank you
for reporting anyway!

Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/14337#issuecomment-38354759
.

Member

robin850 commented Mar 23, 2014

So what kind of format do you want?

I'm not sure to understand what do you mean by "format" but you should use this template to showcase that the bug comes from Active Record. Instead of using factories, you can just use Model.create!(key: value) and instead of using RSpec, you should use Minitest. Moreover, your gist should define, in the schema, only the necessary stuff to reproduce the bug.

If you have any problem you can ask me. :-)

Owner

senny commented Mar 26, 2014

@HangingClowns any chance to create that executable test case?

No, sorry. I didn't see you replied with the format/template until, now. Quite busy at work. Let me try it tomorrow for you guys. Sorry about the wait. I'll be testing it against 4.1.0.rc2, now.

Not getting a negative sum, but getting a stack error.

# Activate the gem you are reporting the issue against.
gem 'activerecord', '4.1.0.rc2'
require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table "stock_moves", force: true do |t|
    t.integer  "stock_wine_id"
    t.decimal  "quantity",           precision: 4, scale: 1
  end
  create_table "stock_wines", force: true do |t|
  end
end

class StockMove < ActiveRecord::Base
  belongs_to :stock_wine
end

class StockWine < ActiveRecord::Base
  has_many :stock_moves

  def quantity
    self[:quantity] || self.quantity.sum(:quantity)
  end
end

class BugTest < Minitest::Test
  def test_select_statement
    wine = StockWine.create!
    wine.stock_moves.create!(quantity:2.0)
    assert_equal wine.quantity, StockWine.where(id:wine.id).select("stock_wines.id, sum(stock_moves.quantity) as quantity").group('stock_wines.id').joins(:stock_moves).first.quantity
  end
end
SystemStackError: stack level too deep
Contributor

andrielfn commented Mar 27, 2014

The stack level too deep is happening because you are calling self.quantity.sum(:quantity) inside the StockWine, which will try to call StockWine#quantity again, and again, and again.

Try to change the quantity method to the following and let me know if it works:

def quantity
  self[:quantity] || self.stock_moves.sum(:quantity)
end

Hmm, that's embarassing. anyways, seems to be working okay, actually. I wonder what could be causing the issue, then? Any thoughts? I have basically this kind of setup in my test. dont' think I'm missing anything.

Owner

rafaelfranca commented Mar 27, 2014

@HangingClowns maybe is something in your code that is causing the issue and you could not replicate in the script.

True. In any case, since it's not a rails bug, I have no other choice but to close this issue. Thanks for your diligence.

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