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

attribute_changed? returns true when assigning a float to a BigDecimal #33801

Closed
matiasgarcia opened this issue Sep 5, 2018 · 2 comments
Closed

Comments

@matiasgarcia
Copy link

matiasgarcia commented Sep 5, 2018

Steps to reproduce

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "5.1.4"
  gem "sqlite3"
end

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 :prices, force: true do |t|
    t.decimal :amount, precision: 63, scale: 30
  end
end

class Price < ActiveRecord::Base

end

class BugTest < Minitest::Test
  def test_big_decimal_change
    amount = 0.00005
    price = Price.create!(amount: amount)
    price.amount = amount
    assert_equal false, price.attribute_changed?(:amount)
  end

  def test_previous_value_and_new_value
    amount = 0.00005
    price = Price.create!(amount: amount)
    price.amount = amount
    previous_value, new_value = price.changes[:amount]
    refute_equal previous_value, new_value
  end
end

Result of running this:

  1) Failure:
BugTest#test_big_decimal_change [test.rb:46]:
Expected: false
  Actual: true

  2) Failure:
BugTest#test_previous_value_and_new_value [test.rb:54]:
Expected 0.5e-4 to not be equal to 0.5e-4.

Expected behavior

attribute_changed?(:amount) should return false.

Actual behavior

attribute_changed?(:amount) returns true despite previous value and new_value are the same.

System configuration

Rails version: 5.1.4

Ruby version: 2.4.1

@yskkin
Copy link
Contributor

yskkin commented Sep 6, 2018

diff --git a/activemodel/lib/active_model/type/helpers/numeric.rb b/activemodel/lib/active_model/type/helpers/numeric.rb
index 16e14f9e5f..724dc06d2d 100644
--- a/activemodel/lib/active_model/type/helpers/numeric.rb
+++ b/activemodel/lib/active_model/type/helpers/numeric.rb
@@ -29,7 +29,7 @@ def non_numeric_string?(value)
             # 'wibble'.to_i will give zero, we want to make sure
             # that we aren't marking int zero to string zero as
             # changed.
-            value.to_s !~ /\A-?\d+\.?\d*\z/
+            value.to_s !~ /\A-?\d+\.?\d*(e[-+]\d+)?\z/
           end
       end
     end

This seems to fix the issue.

yskkin added a commit to yskkin/rails that referenced this issue Sep 7, 2018
For example, dirty checking was not right for the following case:

```
model.int_column = "+5"
model.float_column = "0.5E+1"
model.decimal_column = "0.5e-3"
```

It is enough to see whether leading character is a digit for avoiding
invalid numeric expression like 'wibble' to be type-casted to 0, as
this method's comment says.

Fixes rails#33801
@matiasgarcia
Copy link
Author

That fix makes sense I guess. Hope it's merged soon.

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

3 participants