STI aware Identity Map
Reported by Troy K | June 6th, 2008 @ 02:57 PM
Scoping is already documented as a todo in repository.rb for get, but I wanted to document a valid use case and also add that the inheritance should also be consulted when adding records to the identity map.
class Contact
include DataMapper::Resource
property :id, Integer, :serial => true
property :type, Discriminator
property :name, String, :nullable => false
end
class Person < Contact; end;
class Company < Contact; end;
>> p = Person.create(:name=>'troy')
=> #<Person id=1 ...
>> Company[1]
=> #<Person id=1 ... ## ouch... asked for a Person, got a company
>> Company.first(:id=>1)
=> nil ## cool, resource.first/all have scoping, get does not
As for the identity map, it would be cool if Contact[1] or Person[1] both pointed to the same obj. I took a quick look at submitting a patch, but the changes needed to scope get look pretty deep. I then was going to alias get to first and move on, but did the following monkey_patch instead (I'm posting it here incase someone else wants to use it until get scoping is done). I'm up for doing a patch with some direction from you guys if you'd like the help.
module DataMapper
class Repository
#monkey for STI until dm supports STI scoping by key natively
def get(model, key)
unless resource = identity_maps[model][key]
resource = adapter.read(self, model, key)
if resource and resource.class.respond_to?(:inheritance_class_names)
resource.class.ancestors.each do |klass|
identity_map(klass)[resource.key] = resource if klass.ancestors.include?(DataMapper::Resource) and klass != DataMapper::Resource
end
resource = nil unless resource.class.ancestors.include?(model)
end
end
resource
end
end
end
Comments and changes to this ticket
-
Dan Kubb (dkubb) June 23rd, 2008 @ 07:28 PM
- Title changed from “Repository.get breaks STI” to “STI aware Identity Map”
- State changed from “new” to “open”
Repository#get has been removed, but the issue of Identity Map needing to be STI aware remains.
I was thinking if there was a Model#base method that would return the top-most parent class (and nil if STI is not being used), then that model's Identity Map could be used instead of the current child model's IM.
-
Sam Smoot July 1st, 2008 @ 10:26 AM
- Tag set to “bug, dm-core”
This used to work. Regression in DM9. So yeah, we used to have an IdentityMap#root_model_name or something method. Dan's idea for a Model#base method is better tho'.
I think it should just return self if it's not using STI tho'. That way the IM can just use something.base regardless. No extra conditional. Cleaner, and faster (probably mostly insignificantly, but perhaps not on small models, like m:n models).
-
Bernerd Schaefer July 30th, 2008 @ 11:36 AM
- State changed from “open” to “resolved”
Not sure what's up here, but I pushed a spec that shows this is working correctly, already (dbf2149).
I did add Model#base_model a while back, so perhaps those changes fixed this problem, too. Regardless, it appears to work, so I'll make this resolved.
-
Dan Kubb (dkubb) July 30th, 2008 @ 02:54 PM
(NOTE: the following only applies if we go forward with our plans to lookup stuff in the IM using base_model, which as I understand it is not yet completed)
I think there still might be a problem with this. Think about a situation where there's one parent and two children that are siblings.
Things work fine when a parent or child is saved and then looked up in the IM.
However, there would be a problem if one sibling (sibling1) saved and then the other sibling (sibling2) used the key from sibling1 to do the lookup. In this case sibling1 would be returned, which is probably wrong. In that situation you'd want only existing parent and sibling2 instances to be returned.
I'm wondering if the idea of using base_model only for the IM lookups is flawed. I'm thinking perhaps that there should be one IM for each model, and that inside the callback that is provided to Hash.new it should lookup in the parent model's IM if nothing is found. This should happen all the way up the chain to the top, and if nothing is found then return nil.
Please Sign in or create a free account to add a new ticket.
With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Dan Kubb (dkubb)