brianmario / mysql2

A modern, simple and very fast Mysql library for Ruby - binding to libmysql
http://github.com/brianmario/mysql2
MIT License
2.24k stars 547 forks source link

AR adaptor: forces non-numeric attributes to be 0 #19

Closed yob closed 14 years ago

yob commented 14 years ago

I'm assuming you're planning for the mysql2 AR adaptor to be a drop in replacement for the standard mysql one? Let me know if that isn't the case and I'll stop filing these bugs.

In the standard mysql adaptor if I have a model with a decimal attribute and validates_numericality_of, trying to set that attribute to something silly like an array causes a validation error.

class Supplier < ActiveRecord::Base
  ...
  validates_numericality_of :margin, :greater_than_or_equal_to => 0, :less_than_or_equal_to => 100, :allow_blank => true
  ...
end

>> Supplier.new(:margin => []).valid?
=> false
>> Supplier.new(:margin => 0).valid?
=> true

With the mysql2 adaptor, using an array is accepted:

>> Supplier.new(:margin => []).valid?
=> true
>> Supplier.new(:margin => 0).valid?
=> true
brianmario commented 14 years ago

Yeah I'm definitely shooting for a drop-in replacement, or as close to it as I can get ;) No worries on reporting these issues, keep 'em comin!

I'll try and take a look at this one soon

brianmario commented 14 years ago

Ok try it now (http://github.com/brianmario/mysql2/commit/e01c7581e5a34fc25920b487d8a2ad102dd8b2d1)

yob commented 14 years ago

That commit causes any test in my app that tries to save an AR object to fail :)

All models that have a related object (Invoice belongs_to Company, a valid invoice instance fails to save because it thinks Invoice#company is nil).

brianmario commented 14 years ago

Any other info about why they aren't saving?

brianmario commented 14 years ago

Btw, I'm running my adapter against the rails test suite for the existing mysql gem and adapter to try and get those all passing

yob commented 14 years ago

OK, that was a really lame comment with no details. My aplogies :)

I have a company model that has the following validation:

class Company < ActiveRecord::Base
  ..
  validates_numericality_of :standard_float, :greater_than => 0, :allow_nil => true
  ..
end

The company model is created in nearly every test, so a failure in saving it triggers a sea of red across my test suite.

>> c = Company.new
=> #<Company id: nil, standard_float: nil>
>> c.save!
ActiveRecord::RecordInvalid: Validation failed: Standard float must be greater than 0

So it's calling c.standard_float invalid, even though my validation rule says nil is ok.

brianmario commented 14 years ago

Ok, hopefully this is the last iteration on this casting madness :P http://github.com/brianmario/mysql2/commit/6b285dcd85698e427d0498c6855f831572b6747a

brianmario commented 14 years ago

Whoops, pushed a typo in that commit, it's been fixed so try with the latest

yob commented 14 years ago

Down from 11 failures to 7! Running late for a dinner thing, so will send you detailed into later tonight.

yob commented 14 years ago

(and the failures look like a different issue)

yob commented 14 years ago
>> Supplier.last.update_attributes!(:payment_days => [])

NameError: undefined local variable or method value' for #<Supplier:0x7ff10c474a50> from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/attribute_methods.rb:260:inmethod_missing' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/attribute_methods.rb:211:in payment_days' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:466:insend' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:466 from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:465:in each' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:465 from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:182:incall' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:182:in evaluate_method' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:166:incall' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:90:in run' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:90:ineach' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:90:in send' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:90:inrun' from /var/lib/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:276:in run_callbacks' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:1098:invalid_without_callbacks?' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/callbacks.rb:315:in valid?' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/validations.rb:1087:insave_without_dirty!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/dirty.rb:87:in save_without_transactions!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/transactions.rb:200:insave!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/connection_adapters/abstract/database_statements.rb:136:in transaction' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/transactions.rb:182:intransaction' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/transactions.rb:200:in save!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/transactions.rb:208:inrollback_active_record_state!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/transactions.rb:200:in save!' from /var/lib/gems/1.8/gems/activerecord-2.3.5/lib/active_record/base.rb:2635:inupdate_attributes!' from (irb):4>>

yob commented 14 years ago

I should add that Supplier.payment_days is an int attribute defined like so:

`payment_days` int(11) NOT NULL DEFAULT '30',

with the following validation on it:

validates_numericality_of :payment_days, :greater_than_or_equal_to => 0
brianmario commented 14 years ago

Damn, had another typo... pushed - try one last time? (sorry if this is getting annoying :P)

brianmario commented 14 years ago

I thought my tests covered these cases, but apparently not... Will add more soon

yob commented 14 years ago

With 86b9ffe I drop from 7 failures to 4, but with 48db89 it jumps back up to 154.

The 4 failures are on methods where I'm doing a direct call to SomeModel.connection.select_all(....). All values returned are strings, so I'm manually casting the decimals to BigDecimals. That fails with mysql2, as they're already decimals. Arguably I'd say you could leave that as is, people shouldn't be dropping down to raw SQL too often.

Most of the 154 failures seems to be "String can't be coerced into Fixnum" and are triggered by aggregate functions on associations returning a string instead of a number:

>> CustomerOrderItem.last.approval_items.sum(:quantity)
=> "0"
brianmario commented 14 years ago

Was afraid of that, I reverted 48db89. Really sucks that I have to leave this casting stuff in because AR makes incorrect assumptions about the data all over in places I don't have control over :( Anyway, if things after this revert go back to "normal" and have only the 4 failing tests; I may push 0.1.6 - as for those tests, could just check if it's already a BigDecimal before attempting to cast and have them all pass?

yob commented 14 years ago

OK, b676a3 takes me back to 4 failures.

I've adapted my model code that is calling select_all() to prevent it passing a BigDecimal back into the BigDecimal constructor, so now my full suite is running green.

Thanks!

brianmario commented 14 years ago

awesome, thank you soooo much for all the testing!