Remove rescue that is oculting exceptions on integer columns

[thiagopradi]

Hi Folks,

I found this weird behavior when I was programming during my day job. This is the scenario:

class User
  has_many :tasks
end

class Task
  belongs_to :user
end

so I was building a part of the application, and I made the following mistake, passing a object to the integer field:

t = Task.new
t.user_id = user
t.user_id
=> 1 

whenever this code runs, the user_id always became 1, not raising a error or anything. So I took a look in the rails source, and found that this behavior is fixed in rails master, but not in the 3.2 tree.

I think this could be considered a bug, so I did this patch. What you guys think?

[steveklabnik]

I love your use of 'occulting.'

[rafaelfranca]

I think is better to backport #6092 that has a better test coverage. But first I want to ask: All the tests are passing in all the ruby version?

[thiagopradi]

All tests are green on Ruby 1.9.3.

[thiagopradi]

@rafaelfranca I could backport that commit on #6092. also, if we want to keep compatibility between the code on master / 3.2 branch, we need to change and add tests to this line on master branch: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/column.rb#L117

[rafaelfranca]

I don't think we need to add tests to this line in master because it is not used in the Rails code anymore. It is only there to backward compatibility with others adapters. I think all the adapters are not using this code anymore.

Also we need to test with ruby 1.8.7 before merge.

[carlosantoniodasilva]

Hm yeah, I think it'd be good to backport that particular commit since it adds tests around the column itself as well, and adding your test as a separate commit would help ensuring the problem is fixed from the association point of view. Thanks bro.

[al2o3cr]

One thought - the previous code looks like it's designed to help out people who've misdeclared boolean fields as integer - true.to_i, for instance, raises NoMethodError. That explains the 0-or-1 conversion. Perhaps a heads-up in the changelog is warranted?

[thiagopradi]

@rafaelfranca all tests are passing on Ruby 1.8.7. Also, I backported the tests from #6092. I think it's ready to merge.

[rafaelfranca]

Yes, Changelog entry would be great.

@tchandy could you add it? This pull request is ready to merge

[thiagopradi]

@rafaelfranca done.

[rafaelfranca]

Please squash your commits

[thiagopradi]

@rafaefranca squashed. see #7582.

[amuino]

In Rails 3.2.9 I'm seeing a problem when trying to manage my own primary keys (works with 3.2.8):

• The migration has been run with id: false
• The model declares self.primary_key = 'id'
• But when trying to create/update a new record, to_i is called, resulting in errors or bad data (usually a 0, as expected from to_i)

Is there any workaround for that?

[rafaelfranca]

@amuino I didn't get you problem. to_i is called where?

If the primary key is a integer it is expected that it will call to_i.

Also, I don't think this is related to this pull request. They to_i call was there in 3.2.8 too.

[amuino]

We have an id which is a VARCHAR, but when trying to assign to it (mostly on test code), to_i is being called. I noticed the issue when passing a symbol. That does not raise any errors in 3.2.8, but does in 3.2.9.

I thought it might be related to these changes.

[rafaelfranca]

If to_i is called means that you column in the schema definition is integer.

[amuino]

I'm quite confident it is a VARCHAR

mysql> describe markets;
+--------------+--------------+------+-----+---------+-------+
| Field        | Type         | Null | Key | Default | Extra |
+--------------+--------------+------+-----+---------+-------+
| id           | varchar(25)  | NO   | UNI | NULL    |       |
 ...
+--------------+--------------+------+-----+---------+-------+