Rails Ruby on Rails → 3.0.5

Comments and changes to this ticket

• You flagged this item as spam.
  

  Andrés Mejía November 21st, 2010 @ 03:17 PM

  +1 on this. How was this handled on Rails 3.0.1? Why didn't it fail before?

  For a quick fix, you can try:
  

   Keyword.find_by_text(new_text.to_s)
  

• You flagged this item as spam.
  

  Fluxx November 21st, 2010 @ 07:07 PM

  • Tag set to “patch”

  So I dug in to the Arel and AR source a little bit and came up with a patch (attached) that seems to preserve this behavior. Feedback would be great. I ran the mysql and mysql2 tests with the change and everything passes.

  I am seeing the "test_no_automatic_reconnection_after_timeout" fail consistently even on master, which is weird. After I got the tests running last night (with a MySQL upgrade) they all passed, but this morning the no_automatic_reconnection test isn't passing at all. Not sure what's going on there, but the patch shouldn't be affecting that behavior.

  • to_s_on_finders.diff 1.9 KB

• You flagged this item as spam.
  

  Fluxx November 29th, 2010 @ 05:27 PM

  I should also add that it appears that .where with array conditions works, while .find with the same conditions causes the exception:

  Loading development environment (Rails 3.0.3)
  irb(main):001:0> class Foo < String; end
  => nil
  irb(main):002:0> new_text = Foo.new("ruby on rails")
  => "ruby on rails"
  irb(main):003:0> Keyword
  => Keyword(id: integer, text: string, created_at: datetime, updated_at: datetime, crc32: integer)
  irb(main):004:0> Keyword.create(:text => new_text)
  => #<Keyword id: 7, text: "ruby on rails", created_at: "2010-11-29 09:22:27", updated_at: "2010-11-29 09:22:27", crc32: -83612301>
  irb(main):005:0* Keyword.where("text = ?", new_text)
  => [#<Keyword id: 7, text: "ruby on rails", created_at: "2010-11-29 09:22:27", updated_at: "2010-11-29 09:22:27", crc32: -83612301>]
  irb(main):006:0> Keyword.find("text = ?", new_text)
  NoMethodError: undefined method `visit_Foo' for #<Arel::Visitors::MySQL:0x104f7deb0>
      from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/visitor.rb:15:in `send'
      from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/visitor.rb:15:in `visit
      [...]
  

• 

  Aaron Patterson November 30th, 2010 @ 06:31 AM

  • Milestone cleared.
  • Assigned user set to “Aaron Patterson”
  • Importance changed from “” to “Low”

  The patch seems fine, though, I have to ask whether Rails should really implement this? The reason is because ActiveRecord cannot successfully round-trip your custom string.

  Let me show an example:

  Loading development environment (Rails 3.0.3)
  >> class BenString < String; end
  => nil
  >> ben = BenString.new('ben')
  => "ben"
  >> tag = Tags.create(:name => ben)
  => #<Tags id: 2, name: "ben", created_at: "2010-11-30 06:26:54", updated_at: "2010-11-30 06:26:54">
  >> tag.reload
  => #<Tags id: 2, name: "ben", created_at: "2010-11-30 06:26:54", updated_at: "2010-11-30 06:26:54">
  >> ben.class == tag.name.class
  => false
  >>
  

  The BenString class information is lost during the round trip.

  I'm happy to add this patch for the 3.0.x series along with a deprecation warning.

• You flagged this item as spam.
  

  Fluxx November 30th, 2010 @ 06:06 PM

  True, though it seems that activerecord just #inspects everything on the way in, so it always looses any custom classes. It just seems weird to me to allow creation of objects with anything that responds to #inspect but then blow up when using that same object in a finder.

  My main question I guess is what the expected behavior when you try to do a #find/#where using a class that isn't part of the "main core classes." Right now you get a very cryptic NoMethodError: undefined methodvisit_Foo' for #<Arel::Visitors::MySQL:0x104f85430>error.

  It seems fine to have Arel only support a specific set of classes (ones it has a visitor for), that's not its job. Does activerecord then need to do some kind of enforcement/transformation on the data? Some possible options are:

  • Call #inspect on any object used in a finder
  • Check if the object's class is one of the known set, and if not recurse up the inheritance tree until one of the known set is found
  • Reject the unknown object and raise an exception.

  I would defer to others with more experience on what the right answer is, I was just more bringing it up as an issue and inconsistent behavior.

• You flagged this item as spam.
  

  Abel Muiño November 30th, 2010 @ 06:30 PM

  Also seeing this. In my case, it was with an ActiveSupport::SafeBuffer on mysql.
  While looking up the issue on google, found a similar issue on postgresql reported on https://github.com/ProtectedMethod/restful_acl/issues/issue/9

  The provided patch won't work in this case since ActiveSupport::SafeBuffer implements to_s by returning self.

  I was thinking on adding a method_missing implementation on the ToSql visitor delegating to visit_String if the parameter.is_a?(String)

  I'm not so sure that's the best approach, specially after reading the latter comments on this report (i.e. keep arel light and have active record do the checks/conversions).

• 

  Aaron Patterson November 30th, 2010 @ 06:54 PM

  Yes, I agree this is inconsistent behavior (and that sucks!) But the problem is, how does it know to convert to a string vs an integer. From your example:

  Keyword.find("text = ?", new_text)
  

  Since we're using arbitrary SQL in this example, how can active record determine that it should cast to a string vs an integer? It can't detect that "text" is a string column vs a numeric vs a date (etc) without parsing the SQL and querying the database. inspect can't be arbitrarily called for the same reasons.

  The reason the insert statement worked is because currently, AR is just doing string concatenation for insert statements. As we move AR to rely more on ARel for building SQL statements, this feature will break on inserts too.

  I would rather we have a solid coercion protocol in AR and reject objects that can't be coerced.

  In the mean time, here is a work around that will always be supported and will work with Rails 3.0.3. You can teach ARel about any custom types you want:

  >> class BenString < String; end
  => nil
  >> Tags.find_by_name BenString.new('ben') rescue 'doh'
  => "doh"
  >> Arel::Visitors::Visitor::DISPATCH[BenString] = :visit_String
  => :visit_String
  >> Tags.find_by_name BenString.new('ben') rescue 'doh'
  => #<Tags id: 1, name: "ben", created_at: "2010-11-30 06:26:10", updated_at: "2010-11-30 06:26:10">
  >>
  

• You flagged this item as spam.
  

  Fluxx November 30th, 2010 @ 07:02 PM

  Oh well that's an awesome workaround, Aaron. Thanks! We'll use that for now.

  I'm with you also on a solid coercion protocol as well. Being stricter about this stuff is the best idea long term.

• 

  Aaron Patterson November 30th, 2010 @ 07:13 PM

  • State changed from “new” to “resolved”

  @Fluxx excellent! :-)

  I've pushed a new version of ARel that should fix the problem for you. It will spit out deprecation notices if you have -w set, but I won't actually deprecate this until we get solid coercion code in AR.

  Try ARel version 2.0.5.

  Hope that helps!

• You flagged this item as spam.
  

  Ken Woodruff November 30th, 2010 @ 07:17 PM

  Assuming that passed types have particular class names ('String') rather than just particular behaviors (respond_to? :to_s) breaks a fundamental O-O paradigm.

  how does it know to convert to a string vs an integer

  The target type is readily available from the model's columns.

  irb(main):029:0> Keyword.columns_hash['text'].type
  => :string
  

• 

  Santiago Pastorino February 12th, 2011 @ 09:58 PM

  • Milestone set to “3.0.5”

  [bulk edit]