whoahbot / dm-redis-adapter

A DataMapper adapter for redis
MIT License
143 stars 38 forks source link

Inheritance #26

Open sfeu opened 12 years ago

sfeu commented 12 years ago

Hey,

i played around with the redis adapter, because i am thinking about changing from a tuplespace (rinda) to redis. But i have trouble with inheritance for queries - like described here: http://datamapper.org/docs/misc.html

So far as i understood the adapter code - I think that the redis adapter does not consider the InclusionComparison correctly in the perform_query function.

Can anyone help me to fix this for the redis adapter?

Attached the spec that shows the problem - it actually only returns Woman (First test) and Mother (second test) but not all of them, which i espected to happed for the first test.

require 'spec_helper'

describe DataMapper::Adapters::RedisAdapter do
  before(:all) do
    @adapter = DataMapper.setup(:default, {
        :adapter  => "redis",
        :db => 15
    })
    @repository = DataMapper.repository(@adapter.name)
    @redis = Redis.new(:db => 15)
  end
  describe 'Inheritance' do

    before :all do

      class Person
        include DataMapper::Resource

        property :name, String
        property :job,  String,        :length => 1..255
        property :type, Discriminator
        property :id, Serial
      end

      class Male   < Person; end
      class Father < Male;   end
      class Son    < Male;   end

      class Woman    < Person; end
      class Mother   < Woman;  end
      class Daughter < Woman;  end

      DataMapper.finalize
    end

    it 'should select all women, mothers, and daughters based on Woman query' do
      w = Woman.create(:name => "woman")
      m = Mother.create(:name => "mother")
      d = Daughter.create(:name => "daughter")

      r = Woman.all
      r.to_set.should == [w,m,d].to_set
      r.size.should == [w,m,d].size
    end

    it 'should select all women, mothers, and daughters based on Person query' do
      w = Woman.create(:name => "woman")
      m = Mother.create(:name => "mother")
      d = Daughter.create(:name => "daughter")
      p = Person.all
      p.to_set.should == [w,m,d].to_set
      p.size.should == [w,m,d].size
    end

    it 'should select all mothers' do
      w = Woman.create(:name => "woman")
      m = Mother.create(:name => "mother")
      d = Daughter.create(:name => "daughter")

      mo = Mother.all
      mo.to_set.should == [m].to_set
      mo.size.should == [m].size
    end

  end
  after(:each) do
    @redis.flushdb
  end
end
sfeu commented 12 years ago

Hey,

have started to work on this issue. A first implementation and an improved spec can be found in this branch.

Its already working in most situations, but there is a problem which i think is related to the dm-core that prevents correct results if one queries for Person.all (see second test on spec above).

https://github.com/sfeu/dm-redis-adapter/tree/inheritance_fixes

whoahbot commented 12 years ago

Hello!

Catching up on issues this weekend. I'll have a look at your branch.

sfeu commented 12 years ago

Hey,

would be great if you could take a look at it. Not sure if i did it correctly - but at least the specs continue to work ;-)

I asked for help regarding the remaining issue, but did not get an answer so far...

http://groups.google.com/group/datamapper/browse_thread/thread/3365c68cb0a3d170

whoahbot commented 12 years ago

Hey sfeu,

I had a look at this and I wasn't able to get much further than you were. At this point it looks as though it's doing a find with no conditions, and there isn't a simple way to express that all subclasses of the base type should be included. I'll keep working on it after I finish and merge the refactoring branch.

I'll keep an eye on the mailing list as well.

Thanks!

whoahbot

sfeu commented 12 years ago

Hey whoahbot,

i was able to fix this problem by collecting all descendants myself for this special case. It works but unfortunately i have broken one of your tests ( "should allow has n :through"), because for the inheritance i need unique serials over all models:

i am not sure how to proceed...

When i played around with it and tried to change the broken test to run with the in_memory adapter instead of the redis one it fails there as well...