datamapper / dm-core

DataMapper - Core
http://datamapper.org/
MIT License
754 stars 154 forks source link

Cast Problems in Ruby 2.0 #242

Closed dannluciano closed 8 years ago

dannluciano commented 11 years ago

DataMapper not convert string to int in Model.get("1"). This runned in ruby 1.9.3 but not in 2.0.0.

envygeeks commented 11 years ago

Please provide more debugging information: errors, better examples, anything else that might help sorting it out.

thorsten-de commented 11 years ago

Can confirm the very same problem: In Ruby 2.0.0 Model.get(id) workds only if id is a Number, not if it is a String. Example: MyModel.get 26 works fine, but MyModel.get "26" returns nil.

thorsten-de commented 11 years ago

The same problem can be seen when assigning strings to foreign keys. You can assign an Integer, and everything is fine. But if you assign a string, the foreign key is set to nil. E.G:

my_model_instance.parent_id = 13 #sets parent_id to 13 but my_model_instance.parent_id = '13' #sets parent_id to 'nil'

dannluciano commented 11 years ago

I was micro tests to try find the bug. I believe the bug is in typecast method (https://github.com/datamapper/dm-core/blob/master/lib/dm-core/property_set.rb#L112) or (https://github.com/datamapper/dm-core/blob/master/lib/dm-core/property.rb#L677)

dkubb commented 11 years ago

@dannluciano if you run the specs for DataMapper under Ruby 2.0, do you see any failing specs that can help pinpoint the issue? We've run DM somewhat recently against Ruby 2.0 https://travis-ci.org/datamapper/dm-core and not found any issues, which is why I find this curious.

dannluciano commented 11 years ago

I try run spec in dm-core and I get this errors.

'DataMapper::Property::Decimal#typecast does not typecast non-numeric value "00.0"' FAILED

expected #String:70284319365080 => "00.0" got #BigDecimal:70284307553040 => #<BigDecimal:7fd8ae5eb620,'0.0',9(18)>

Compared using equal?, which compares object identity, but expected and actual are not the same object. Use 'actual.should == expected' if you don't care about object identity in this example.

'DataMapper::Property::Float#typecast does not typecast non-numeric value "00.0"' FAILED

expected #String:70284317166840 => "00.0" got #Float:-9223372036854775806 => 0.0

Compared using equal?, which compares object identity, but expected and actual are not the same object. Use 'actual.should == expected' if you don't care about object identity in this example.

'DataMapper::Property::Integer#typecast does not typecast non-numeric value "00.0"' FAILED

expected #String:70284315974140 => "00.0" got #Fixnum:1 => 0

Compared using equal?, which compares object identity, but expected and actual are not the same object. Use 'actual.should == expected' if you don't care about object identity in this example.

12770 examples, 3 failures, 3496 pending

The complete log is there https://gist.github.com/dannluciano/5067665.

Thanks for your time.

dannluciano commented 11 years ago

Where the coercions is implemented in dm-core? By dm-core or https://github.com/solnic/virtus with https://github.com/solnic/coercible ?

samflores commented 11 years ago

I believe I tracked the issue. Ruby 2.0 changed the way it treats the visibility of protected methods, so calls to respond_to?(:typecast_to_primitive) are returning false in the DataMapper::Property#typecast method.

What surprises me is that the issue does not exists in the code hosted on Github. A second argument true is being passed to the mentioned respond_to? call, which instructs it to look for protected methods (https://github.com/datamapper/dm-core/blob/release-1.2/lib/dm-core/property.rb#L685), but the gem available in Rubygems does not have the argument. I guess that's why Travis didn't pointed anything.

envygeeks commented 11 years ago

Maybe ping @dkubb and @solnic and see if they think everything is copacetic and ready to release a new DM.

solnic commented 11 years ago

We could port dm1 to use coercible

samflores commented 11 years ago

I believe backporting coercible would be great, but I think releasing the already fixed 1.2.1 version (release-1.2 branch) as a gem would solve the problem until then.

solnic commented 11 years ago

@samflores if all projects are ported to use travis and all specs passed then we can push 1.2.1

@dkubb are all projects ported to travis already?

dkubb commented 11 years ago

@solnic no, a few people talked about assisting and we had a bit of initial activity towards it, but there are still a bunch of gems that need to be updated to work with travis.

envygeeks commented 11 years ago

@dkubb @solnic I thought we got all but one or two?

dkubb commented 11 years ago

@envygeeks no, about 80% of the dm-* gems listed under https://github.com/datamapper/ still haven't been touched.

dariocravero commented 11 years ago

Hi guys, any news on when you might be fixing/releasing this? Some users were scratching their heads when this happened on Padrino and then we found this issue :). Let me know if I can help in any way. Thanks!

envygeeks commented 11 years ago

I'm waiting for @dkubb to get back to me with a list of repos that need travis so I can get them added and then they can do the rest, it's already fixed in head we just have to get the rest of the components on public testing so everything can be confirmed.

dariocravero commented 11 years ago

@envygeeks amazing! thanks!! :D will keep an eye on this then

mbj commented 11 years ago

@envygeeks I think all repos under the datamapper organization, that have a release, need to be ported.

solnic commented 11 years ago

Here's a list of all gems that are part of the dm1 suite: https://gist.github.com/solnic/4999203

dkubb commented 11 years ago

@envygeeks sorry, I didn't realize you wanted me to get you a list. I see @solnic has already created a list of gems that are part of DM1 for you.

snusnu commented 11 years ago

@dkubb @solnic @mbj fyi, i've made @envygeeks an owner of the datamapper organization, so that he's not blocked during his travis spree

p commented 11 years ago
    # @api semipublic
    def typecast(value)
      if value.nil? || primitive?(value)
        value
      elsif respond_to?(:typecast_to_primitive)
        typecast_to_primitive(value)
      end
    end

If neither branch matches the return value is nil which surely cannot be right.

dkubb commented 11 years ago

@p this has been fixed in the release-1.2 branch by using respond_to?(:typecast_to_primitive, true).

However, you do bring up a good point. In a custom property, it is possible that neither of the first two branches will match, and nil will be returned which is not what was intended originally. I'm committing a change that will handle the custom property side of things by simply passing-through the value when it is unable to typecast.

mereghost commented 11 years ago

Any idea when a new release with that fix will be made?

ujifgc commented 11 years ago

'dm-core' '1.2.1' at rubygems is fixed.

tpitale commented 8 years ago

This issue appears to be resolved. Closing.