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

Rounding Decimal on Assignment before validation #23400

Closed
Dfenniak opened this Issue Feb 1, 2016 · 13 comments

Comments

Projects
None yet
3 participants
@Dfenniak
Copy link

Dfenniak commented Feb 1, 2016

Hey, I think this is a breaking change when upgrading from 4.2.4 to 4.2.5.

It looks like previously, when assigning a decimal value, no rounding or changes occurred. It looks like after upgrading rounding is occurring before validation, which prevents us from adding errors to the object and informing users that their input was invalid.

steps to reproduce (can run this in pry), setup:

 ActiveRecord::Migration.create_table(:broken_things) { |t| t.decimal :my_decimal_column, :precision => 8, :scale => 2 }

class BrokenThing < ActiveRecord::Base
  attr_accessible :my_decimal_column
  validate :decimal_place
  def decimal_place
      errors.add(:my_decimal_column, ' this should be there') unless my_decimal_column.to_s =~ /\A\d*(\.\d{1,2})?\Z/
  end
end

the actual problem

x = BrokenThing.new
x.my_decimal_column = 55.555555555555
x
=> #<BrokenThing:0x00000111e873c8 id: nil, my_decimal_column: 55.56>
x.valid?
=> true

in 4.2.4 this would have assigned the decimal place to 55.5555555555 and valid? would return false.

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 1, 2016

Just trying to add a test case now... not really sure how to contribute, sorry if I'm not following the rules correctly

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Feb 1, 2016

No problem -- there are templates available here for bug templates. Working on one too, right now.

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Feb 1, 2016

@Dfenniak - what should the expected value for my_decimal_column be? 55.555555555555 ?

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Feb 1, 2016

This is what I have so far right now -- https://gist.github.com/maclover7/4f882df0615151e0a9c4

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 1, 2016

@maclover7 that's right. in 4.2.4 after assignment the attribute's value would be 55.555555555555 now it's rounding it

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 1, 2016

@maclover7 I added a comment to your gist to clarify, thanks for the help :)

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 1, 2016

this is what I think the test should be (try toggling between 4.2.4 and 4.2.5 to see what I'm whining about!):

require 'bundler/inline'

gemfile(true) do
  gem 'rails', '4.2.5'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: 'cookie_store_key'
  secrets.secret_token    = 'secret_token'
  secrets.secret_key_base = 'secret_key_base'

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :things do |t|
    t.decimal :my_decimal_column, :precision => 8, :scale => 2
  end
end

class Thing < ActiveRecord::Base
  validate :decimal_place

  def decimal_place
    errors.add(:my_decimal_column, 'this should be there') unless my_decimal_column.to_s =~ /\A\d*(\.\d{1,2})?\Z/
  end
end

class BugTest < Minitest::Test
  def test_returns_success
    x = Thing.new
    x.my_decimal_column = 55.555555555555
    assert_equal(x.my_decimal_column, BigDecimal.new(55.555555555555, 8)) # this fails on 4.2.5 passes on 4.2.4
    assert(!x.valid?) # false on 4.2.4,  true on 4.2.5
    assert(!x.save) # false on 4.2.4,  true on 4.2.5
  end
end

@maclover7 maclover7 self-assigned this Feb 2, 2016

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Feb 2, 2016

I think the issue here is that you have the scale for the decimal column set to 2, which means that Active Record will truncate down 55.55.555555555555 to 55.56.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Feb 2, 2016

@maclover7 is correct, this is working as intended. There was previously a bug where precision and scale were not properly interpreted, which was fixed.

@sgrif sgrif closed this Feb 2, 2016

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 2, 2016

@sgrif changing when rounding is occurring might be for the best, but I think changing the API like this shouldn't happen in a patch? For me it's not actually a great change as it prevents us from validating the data and adding errors to the objects. (would have happened in 4.2.4) now instead we'll need to add front-end code to deal with that

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Feb 2, 2016

@Dfenniak This wasn't a change in our decision of when to round, it was fixing a bug. One of the guarantees that we provide in our type system is

value_before_save = model.attribute
model.save
model.reload
value_after_save = model.attribute

assert_equal value_before_save, value_after_save

This was specifically the contract that needed to be upheld.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Feb 2, 2016

Also keep in mind that for validations, you should probably use my_decimal_column_before_type_cast, which will have all of the information you're looking for, and is what we use in our numericality validations

@Dfenniak

This comment has been minimized.

Copy link
Author

Dfenniak commented Feb 2, 2016

@sgrif thanks for taking the time to explain further. had no idea that was a thing. will start implementing it on all my projects!

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