This project is archived and is in readonly mode.

Unable to find by subclasses of String in activerecord

#6023

We have a subclass of String that we use for encapsulating some URL and domain normalization logic, in a very similar way to how rails does #html_safe. We recently upgraded from Rails 3.0.1 to Rails 3.0.3, and it appears some of the arel refactoring has affected the ability to find records by objects that are not one of a set of a few predefined classes.

Here is a simplified example of the problem. As you can see, you can create objects with "string" values that are subclasses of string, but you cannot actually issue a find to locate the object the same way:

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: 11, text: "ruby on rails", created_at: "2010-11-20 16:48:09", updated_at: "2010-11-20 16:48:09", crc32: -83612301>
irb(main):005:0> Keyword.find_by_text(new_text)
NoMethodError: undefined method `visit_Foo' for #<Arel::Visitors::MySQL:0x104d0f4d0>
    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'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:235:in `visit_Arel_Nodes_Equality'
    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'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:109:in `visit_Arel_Nodes_Grouping'
    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'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:89:in `visit_Arel_Nodes_SelectCore'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:89:in `map'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:89:in `visit_Arel_Nodes_SelectCore'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/mysql.rb:15:in `visit_Arel_Nodes_SelectCore'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:77:in `visit_Arel_Nodes_SelectStatement'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:77:in `map'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:77:in `visit_Arel_Nodes_SelectStatement'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/mysql.rb:10:in `visit_Arel_Nodes_SelectStatement'
    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'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/visitor.rb:5:in `accept'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:19:in `accept'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/abstract/connection_pool.rb:110:in `with_connection'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/visitors/to_sql.rb:17:in `accept'
    from /opt/local/lib/ruby/gems/1.8/gems/arel-2.0.4/lib/arel/tree_manager.rb:19:in `to_sql'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/relation.rb:64:in `to_a'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/relation/finder_methods.rb:333:in `find_first'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/relation/finder_methods.rb:122:in `first'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/relation/finder_methods.rb:234:in `send'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/relation/finder_methods.rb:234:in `find_by_attributes'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/base.rb:987:in `send'
    from /opt/local/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/base.rb:987:in `method_missing'

Digging through the Arel source some, it appears that it is calling obj.class.name to lookup the visitor. Obviously, there isn't a visit_Foo method.

Possible options to fix this:

Check if the passed value's class is one of the known set, and if not recurse up the inheritance tree until one of the known set is found
Simply fall back to visit_String if visit_ForeignClassName is called.

Thoughts?

Reported by Fluxx · November 21st, 2010 @ 01:02 AM

Activity

Andrés Mejía
+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)
```
November 21st, 2010 @ 03:17 PM
Fluxx
- 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.87 KB
November 21st, 2010 @ 07:07 PM

Fluxx

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
    [...]

November 29th, 2010 @ 05:27 PM

Aaron Patterson
- 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.
November 30th, 2010 @ 06:31 AM
Fluxx
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.
November 30th, 2010 @ 06:06 PM
Abel Muiño
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).

November 30th, 2010 @ 06:30 PM
Aaron Patterson
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">
>>
```
November 30th, 2010 @ 06:54 PM
Fluxx
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.

November 30th, 2010 @ 07:02 PM
Aaron Patterson
- 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!

November 30th, 2010 @ 07:13 PM
Ken Woodruff
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
```
November 30th, 2010 @ 07:17 PM
Santiago Pastorino
- Milestone set to 3.0.5
[bulk edit]

February 12th, 2011 @ 09:58 PM