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

Already on GitHub? Sign in to your account

ActiveSupport 4 breaks BigDecimal#to_yaml #12467

Closed
jarmo opened this Issue Oct 8, 2013 · 10 comments

Comments

Projects
None yet
4 participants

jarmo commented Oct 8, 2013

When using ActiveSupport 4 then BigDecimal#to_yaml does not work as expected.

In plain old IRB:

irb(main):001:0> require "yaml"
=> true
irb(main):002:0> require "bigdecimal"
=> true
irb(main):003:0> BigDecimal("13.37").to_yaml
=> "--- !ruby/object:BigDecimal 18:0.1337E2\n...\n"
irb(main):004:0> require "active_support/all"
=> true
irb(main):005:0> BigDecimal("13.37").to_yaml
=> "--- 13.37\n...\n"

When BigDecimal is serialized like this, then deserializing will create a Float instead of BigDecimal. This is not okay when dealing with floating point arithmetics.

The offending code is here https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/big_decimal/conversions.rb#L8-L11

Removing that method, makes BigDecimal work again:

irb(main):006:0> class BigDecimal
irb(main):007:1> remove_method :encode_with
irb(main):008:1> end
=> BigDecimal
irb(main):009:0> BigDecimal("13.37").to_yaml
=> "--- !ruby/object:BigDecimal 18:0.1337E2\n...\n"

The problem appears on Ruby 1.9.3 and 2.0 too.

What's the proper way to fix this problem?

Member

senny commented Oct 9, 2013

The method was added with 2570c85 but judging from the changes to the test file it stripped the BigDecimal also before that commit.

/cc @tenderlove

Owner

tenderlove commented Oct 9, 2013

I think the commit was just to be backwards compatible with what was in
Rails before. We should just remove any custom serialization for
BigDecimal IMO.

On Wed, Oct 9, 2013 at 2:15 AM, Yves Senn notifications@github.com wrote:

The method was added with 2570c852570c85 judging from the changes to the test file it stripped the
BigDecimal also before that commit.

/cc @tenderlove https://github.com/tenderlove


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

Aaron Patterson
http://tenderlovemaking.com/

jarmo commented Oct 9, 2013

That makes sense, because if i look at the diff then i see that the serialization was not working before too. By not serializing BigDecimal's as BigDecimal's seems like a quite big problem.

jarmo commented Oct 10, 2013

Yup, i also found that, but it is quite old and outdated and not relevant today anymore. As @tenderlove said - i have to agree that ActiveSupport should not do any custom serialization with BigDecimal and should just leave that to Ruby. If older Ruby versions do not handle it also then it's the problem of Ruby and not some external library.

Owner

tenderlove commented Oct 10, 2013

Why don't we change it such that requiring activesupport/all will not load custom YAML serializations for big decimal. We can add another file that has the custom serialization stuff, so if people need it for backwards compatibility, they can require it manually. We should also add a deprecation warning to that file so we can rm it in the future.

How does that sound?

jarmo commented Oct 10, 2013

Sounds good to me :)

Until this is fixed, is there a monkeypatch that would undo the modifications by ActiveSupport? @jarmo indicated that removing the BigDecimal#encode_with method "makes BigDecimal work again", but it only fixes the serialization: the deserialization now delivers a String:

2.0.0p247 :001 > BigDecimal.new("1.5").to_yaml
 => "--- !ruby/object:BigDecimal 18:0.15E1\n...\n" 
2.0.0p247 :002 > YAML.load(BigDecimal.new("1.5").to_yaml)
 => "18:0.15E1" 
2.0.0p247 :003 > YAML.load(BigDecimal.new("1.5").to_yaml).class
 => String 

Nevermind, this is not due to ActiveSupport, but due to collectiveidea/delayed_job#588

jarmo commented Oct 31, 2013

Yeah, been there, done that - both of the issues are reported by me :)

@davidcelis davidcelis added a commit to davidcelis/rails that referenced this issue Feb 1, 2014

@davidcelis davidcelis Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/conversions'`. I would
agree. So, remove `active_support/core_ext/big_decimal` so that this
extension is no longer required along with `active_support/all`.

Signed-off-by: David Celis <me@davidcel.is>
9b3c703

@davidcelis davidcelis added a commit to davidcelis/rails that referenced this issue Feb 1, 2014

@davidcelis davidcelis Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/conversions'`. I would
agree. So, remove `active_support/core_ext/big_decimal` so that this
extension is no longer required along with `active_support/all`.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis <me@davidcel.is>
a2da0ee

@davidcelis davidcelis added a commit to davidcelis/rails that referenced this issue Feb 1, 2014

@davidcelis davidcelis Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/conversions'`. I would
agree. So, remove `active_support/core_ext/big_decimal` so that this
extension is no longer required along with `active_support/all`.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis <me@davidcel.is>
14cff0e

@davidcelis davidcelis added a commit to davidcelis/rails that referenced this issue Feb 1, 2014

@davidcelis davidcelis Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
# => "--- !ruby/object:BigDecimal 18:0.1337E2\n...\n"
YAML.load(yaml).class
# => BigDecimal

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
# => "--- 13.37\n...\n"
YAML.load(yaml).class
# => Float
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/conversions'`. I would
agree. So, remove `active_support/core_ext/big_decimal` so that this
extension is no longer required along with `active_support/all`.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis <me@davidcel.is>
66fe46c

@davidcelis davidcelis added a commit to davidcelis/rails that referenced this issue Feb 1, 2014

@davidcelis davidcelis Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
# => "--- !ruby/object:BigDecimal 18:0.1337E2\n...\n"
YAML.load(yaml).class
# => BigDecimal

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
# => "--- 13.37\n...\n"
YAML.load(yaml).class
# => Float
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/yaml_conversions'`.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis <me@davidcel.is>

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
# On branch remove-bigdecimal-serialization
# Changes to be committed:
#	modified:   activesupport/CHANGELOG.md
#	modified:   activesupport/lib/active_support/core_ext/big_decimal/conversions.rb
#	new file:   activesupport/lib/active_support/core_ext/big_decimal/yaml_conversions.rb
#	new file:   activesupport/test/core_ext/big_decimal/yaml_conversions_test.rb
#	modified:   activesupport/test/core_ext/bigdecimal_test.rb
#
718cbe8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment