soveran / ohm

Object-Hash Mapping for Redis
http://ohm.keyvalue.org
MIT License
1.4k stars 167 forks source link

Collections/references don't work in modules #209

Closed ProjectMoon closed 8 years ago

ProjectMoon commented 8 years ago

This code causes an uninitialized constant error when run: `const_get': uninitialized constant MyModule::Model1::Model2 (NameError). Everything works as intended if you remove the modules wrapping the models.

require 'ohm'

module MyModule
  class Model1 < Ohm::Model
    attribute :name
    index :name
    collection :model2_collection, :Model2
  end

  class Model2 < Ohm::Model
    reference :model1, :Model1 # has-a
    reference :model3, :Model3 # belongs-to
    attribute :thing
  end

  class Model3 < Ohm::Model
    collection :model2_collection, :Model2
  end
end

m1 = MyModule::Model1.create(name: 'MyModel')
m3 = MyModule::Model3.create
m2 = MyModule::Model2.create(thing: 'thingy', model1: m1, model3: m3)

m1.save
m2.save
m3.save

m3 = MyModule::Model3[1]
m1 = MyModule::Model1[1]

puts m1.model2_collection.size
puts m1.model2_collection.each { |m| puts m.thing }

puts m3.model2_collection.size
puts m3.model2_collection[0].nil?

I did find a reference and possible fix to this bug on Google groups: https://groups.google.com/forum/#!msg/ohm-ruby/dEDl9LavbVc/zE8wFunFalsJ

ProjectMoon commented 8 years ago

A possible solution to this in Utils.const might be to fall back to the enclosing module of the class by parsing the full class name and using Object.const_get, then try loading the class off that module, and then if that fails, throw an error.

soveran commented 8 years ago

The traditional solution for this is to qualify the class by including the module's name. For example, you would use reference :model1, 'MyModule::Model1' instead of just reference :model1, :Model1. Check this example in the tests.

ProjectMoon commented 8 years ago

I did actually try screwing around with something like that, although I was using symbols instead. I tried :"MyModule::Model1", which produced a similar error. Maybe the simplest solution to this is simply to update the documentation (both API and quickstart)?

I do think it would be good to have a bit of magic going on to find the module though. String values aren't able to be caught as easily as an uninitialized module. The interpreter will throw a much more sensible error if you try to reference a non-existing module rather than if Ohm tries to use a string it can't find.

soveran commented 8 years ago

I added an explanation about how to work with modules: https://github.com/soveran/ohm#modules

Closing this issue for the time being, but let me know if you find it insuficient.