Remove rescue that is oculting exceptions on integer columns #7509

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

thiagopradi commented Sep 3, 2012

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?

Member

steveklabnik commented Sep 3, 2012

I love your use of 'occulting.'

Owner

rafaelfranca commented Sep 3, 2012

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?

Contributor

thiagopradi commented Sep 3, 2012

All tests are green on Ruby 1.9.3.

Contributor

thiagopradi commented Sep 3, 2012

@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

Owner

rafaelfranca commented Sep 3, 2012

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.

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.

Contributor

al2o3cr commented Sep 7, 2012

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?

Contributor

thiagopradi commented Sep 9, 2012

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

Owner

rafaelfranca commented Sep 9, 2012

Yes, Changelog entry would be great.

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

Contributor

thiagopradi commented Sep 9, 2012

Owner

rafaelfranca commented Sep 9, 2012

Please squash your commits

Contributor

thiagopradi commented Sep 9, 2012

@rafaefranca squashed. see #7582.

rafaelfranca added a commit that referenced this pull request Oct 30, 2012

Fix bug when Column is trying to type cast boolean values to integer.
This can occur if the user is using :integer columns to store boolean
values. Now we are handling the boolean values but it still raises if
the value can't type cast to integer and is not a boolean. See #7509.

Fixes #8067.

rafaelfranca added a commit that referenced this pull request Oct 30, 2012

Fix bug when Column is trying to type cast boolean values to integer.
This can occur if the user is using :integer columns to store boolean
values. Now we are handling the boolean values but it still raises if
the value can't type cast to integer and is not a boolean. See #7509.

Fixes #8067.

Conflicts:
	activerecord/CHANGELOG.md

amuino commented Dec 12, 2012

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?

Owner

rafaelfranca commented Dec 13, 2012

@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 commented Dec 13, 2012

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.

Owner

rafaelfranca commented Dec 13, 2012

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

amuino commented Dec 16, 2012

I'm quite confident it is a VARCHAR

mysql> describe markets;
+--------------+--------------+------+-----+---------+-------+
| Field        | Type         | Null | Key | Default | Extra |
+--------------+--------------+------+-----+---------+-------+
| id           | varchar(25)  | NO   | UNI | NULL    |       |
 ...
+--------------+--------------+------+-----+---------+-------+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment