Sequel model instances respond_to :each

[scottburton11]

Sequel model instances respond to :each, and thus fail the is_object?(obj) test:

helpers.rb

# Returns true if obj is not enumerable
# is_object?(@user) => true
# is_object?([]) => false
# is_object?({}) => false
def is_object?(obj)
  obj && !data_object(obj).respond_to?(:each)
end

# Returns true if the obj is a collection of items
def is_collection?(obj)
  obj && data_object(obj).respond_to?(:each)
end

A typical stack trace looks like this:

NoMethodError - undefined method `map' for #<Product:0x007f9318d86f78>:
    /Users/scott/.rvm/gems/ruby-1.9.2-p290/gems/rabl-0.5.3/lib/rabl/engine.rb:43:in `to_hash'
    /Users/scott/.rvm/gems/ruby-1.9.2-p290/gems/rabl-0.5.3/lib/rabl/engine.rb:53:in `to_json'
    /Users/scott/.rvm/gems/ruby-1.9.2-p290/gems/rabl-0.5.3/lib/rabl/engine.rb:28:in `render'
    /Users/scott/.rvm/gems/ruby-1.9.2-p290/gems/rabl-0.5.3/lib/rabl/template.rb:15:in `evaluate'

Sequel collections unsurprisingly render without error.

[nesquena]

Hmm, I wish there was a better way to distinguish an object and a collection without breaking backward compatibility...if I make object always assume object and collection required for collections, I feel like that's a less than ideal breaking change.

[scottburton11]

I wonder if anyone even uses :each on Sequel models?

In any case, I wish I had a solution. I like the magic of object [:array, :of, :things], but I always use collection when I know I have a collection.

Do these shed some light on anything?

Array.new.public_methods(false).inject([]) { |m, i| m << i unless Hash.new.respond_to?(i); m }.sort

Hash.new.public_methods(false).inject([]) {|m,i| m << i unless Array.new.respond_to?(i); m }.sort

Not enough overlap with ActiveRecord::Base etc., but it might be a good place to start looking for a better way to distinguish the two.

Anyway, thanks for taking a look.

[databyte]

I don't use Sequel, can you see if it includes Enumerable?

>> [Array, Hash, String].map{|o| o.included_modules.include?(Enumerable)}
=> [true, true, false]

[kdstew]

I've just run into the same situation as scottburton11

To answer your question:

Sequel::Model.included_modules.include?(Enumerable)
=> false

Sequel::Dataset.included_modules.include?(Enumerable)
=> true

Also an array of model objects can be returned.

[databyte]

I guess Enumerable check would be too easy. Instead of seeing what all these have in common, maybe a better option is to register class names into an Array that we check against. It could be a configuration override such that the default is DATA_OBJECT_CLASSES = [Array, Hash, ActiveRecord::Relation] and then you can add to it yourself. (probably not the actual class but, you know, a string of the name)

(Sorry for the late responses, I was on holiday)

[scottburton11]

Sorry, I've been away from this for a while too.

I favor the registration route; rabl can be told what types of objects are collections and what types are singular objects, and fall back on the existing behavior when in doubt.

Maybe optional adapter gems are in order (rabl_sequel, rabl_mongoid) which do nothing but require rabl and configure for these ORM's. Similar pattern to what's used with rspec_rails, etc.

[databyte]

Just loaded up Sequel to see what it was all about. The easiest thing that I could do is just change the check from each to map.

>> Sequel::SQLite::Dataset.new(nil).methods.include? :map
=> true
>> Sequel::Model.new.methods.include? :map
=> false
>> Sequel::Model.new.methods.include? :each
=> true

A more robust method could be to whitelist classes into categorizing them as objects or collections such as:

    def is_object?(obj)
      obj && !(data_object(obj).respond_to?(:map) || Rabl.configuration.objects.include?(obj.class))
    end

    # Returns true if the obj is a collection of items
    def is_collection?(obj)
      obj && (data_object(obj).respond_to?(:map) || Rabl.configuration.collections.include?(obj.class))
    end

# where in config:

config.objects = [Sequel::Model, MyOtherStubbornClassThatIsReallyAnObject]
config.collections = [Sequel::SQLite::Dataset]

Personally I think it's easier just to change it to map and be pragmatic about the other method later on should the real need arise.

@nesquena - thoughts?

[nesquena]

I like being pragmatic. There aren't that many ORMs in the wild. If we can switch to checking for map and have the big 4 (datamapper, activerecord, sequel, mongoid) work then I am happy.

[databyte]

You just had to bring up other ORMs eh?

So I'm no expert at dm or mongoid but a quick look at the code tells me that they both support each but not map :-(

dm defining each but not map: https://github.com/datamapper/dm-core/blob/master/lib/dm-core/model.rb#L321

mongoid delegating each but not map: https://github.com/mongoid/mongoid/blob/master/lib/mongoid/finders.rb#L10

(and that was an entire 60 secs of searching so I may have missed another included class or module that does so - other frequent users of both should chime in)

[nesquena]

I think we might be OK. The question here is does an instance of an object respond to a method. The problem right now is that an instance of a sequel object responds to each. So this means that when given an object like @user in sequel it mistakes that for a collection. But sequel doesn't respond to map at the instance level and neither does a mongoid or dm or ar instance of a record either.

So as far as I can tell if a collection supports map then it means it is not an object. Trying this in the console with sequel and mongoid:

# Sequel
>> User.filter.respond_to?(:map)
=> true
>> user = User.first
>> user.respond_to?(:each)
=> true <--- problem
>> user.respond_to?(:map)
=> false

# Mongoid
>> User.criteria.respond_to?(:map)
=> true
>> user = User.first
>> user.respond_to?(:each)
=> false
>> user.respond_to?(:map)
=> false

Am I right here or am I missing something that makes this not work.

[databyte]

Nope, that looks good to me! And I did get it backwards on that last comment since it's not about what the collection responds to but the object. Which makes a lot more sense since you map over the collection within to_hash.

[databyte]

I feel bad though that no tests changed because of it. :cold_sweat:

We may need a TODO, wiki entry or something that lists a desire to build out a multi-ORM test fixture too.

[nesquena]

Great, thanks man. Yeah agreed I can see that being useful in the future.

[kdstew]

Thanks guys for getting that change in. Much appreciated!