Browse files

Add a DataAccessor Registry to HitEnumerable

  • Loading branch information...
1 parent c290d35 commit 64d098561367f641844cb3e469811da957ab703b @brutuscat committed Mar 10, 2012
View
54 sunspot/lib/sunspot/adapters.rb
@@ -166,8 +166,13 @@ def instance_adapters #:nodoc:
# Sunspot::Adapters::DataAccessor.register(FileAccessor, File)
#
class DataAccessor
+ # Attributes that should be passed to other adapted subclasses
+ attr_accessor :inherited_attributes
+ attr_reader :clazz
+
def initialize(clazz) #:nodoc:
@clazz = clazz
+ @inherited_attributes = []
end
# Subclasses can override this class to provide more efficient bulk
@@ -261,5 +266,54 @@ def data_accessors #:nodoc:
end
end
end
+
+ class Registry
+ extend Forwardable
+
+ def initialize
+ @reg = {}
+ end
+ def_delegator :@reg, :keys, :registered
+ def_delegators :@reg, :include?
+
+ def retrieve(clazz)
+ key = clazz.name.to_sym
+ if !@reg.include?(key)
+ data_accessor = inject_inherited_attributes_for( Adapters::DataAccessor.create(clazz) )
+ @reg[key] ||= data_accessor
+ end
+ @reg[key]
+ end
+
+ # It will inject declared attributes to be inherited from ancestors
+ # only if they are not already present in the data_accessor for each class.
+ def inject_inherited_attributes_for(data_accessor)
+ return data_accessor if @reg.empty?
+
+ data_accessor.inherited_attributes.each do |attribute|
+ if try_attribute_for(attribute, data_accessor).nil?
+ inherited_value = nil
+ clazz = data_accessor.clazz
+ original_class_name = clazz.name
+ clazz.ancestors.each do |ancestor_class|
@subwindow
subwindow added a line comment Mar 13, 2012

This seems really bizarre to me. It appears to be duplicating Ruby's method lookup algorithm. Is there a good reason for it?

The code is really hard to read. Some white space and comments would be extremely helpful.

@brutuscat
Owner
brutuscat added a line comment Mar 13, 2012

Actually I took the code from the Adapter#for method https://github.com/brutuscat/sunspot/blob/64d098561367f641844cb3e469811da957ab703b/sunspot/lib/sunspot/adapters.rb#L118
So yes, it may look bizarre but it would be "understandable" for the repo owner ;)

To ensure that this would work for non-AR classes I just used that code. Feel free to change it :D

What I do need is to inspect the whole chain of ancestors to see if there is any adapter instantiated for it to inject the inherited attributes for the current, sub-class instance.
I know, may sound overkill but was the quickest "patch" it occurs to me.

@subwindow
subwindow added a line comment Mar 13, 2012

For whatever reason the code in Adapter#for is much more clear. The intent of the code here is not evident, like in try_attribute_for. It's not entirely clear that using send() here is the right thing to do. Won't send() automatically traverse up the ancestor list, making the clazz.ancestors.each loop redundant?

I may be wrong- it might be the right thing to do. However, it's really hard to figure out because I'm not sure what the code is supposed to do to begin with.

@brutuscat
Owner
brutuscat added a line comment Mar 13, 2012

The send yes, it should do that. But what I need is a value of an inherited_attribute for the super-class. Because in the current class it is nil actually. So calling send just there will return nil

While I could rename some methods I don't think it is actually hard to figure out what the code does..

  • It loops through the parent classes
  • Testing for values of those inheritable attributes
  • and stops when found a value
  • then injects the value in the current data_accessor instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ next if ancestor_class.name.nil? || ancestor_class.name.empty?
+ key = ancestor_class.name.to_sym
+ inherited_value = try_attribute_for(attribute, @reg[key]) if @reg[key]
+ break unless inherited_value.nil?
+ end
+ data_accessor.send("#{attribute.to_s}=", inherited_value) unless inherited_value.nil?
+ end
+ end
+ data_accessor
+ end
+
+ private
+
+ def try_attribute_for(attribute, data_accessor)
+ data_accessor.send(attribute)
+ rescue NoMethodError
+ nil
+ end
+ end
end
end
View
9 sunspot/lib/sunspot/search/hit_enumerable.rb
@@ -27,7 +27,7 @@ def populate_hits #:nodoc:
end
id_hit_hash.each_pair do |class_name, hits|
ids = hits.map { |id, hit| hit.primary_key }
- data_accessor = data_accessor_for(Util.full_const_get(class_name))
+ data_accessor = data_accessor_for(Util.full_const_get(class_name))
hits_for_class = id_hit_hash[class_name]
data_accessor.load_all(ids).each do |result|
hit = hits_for_class.delete(Adapters::InstanceAdapter.adapt(result).id.to_s)
@@ -61,11 +61,8 @@ def each_hit_with_result
# the block DSL.
#
def data_accessor_for(clazz) #:nodoc:
- # FIXME: This method does not belong here, but I was getting bogged
- # down trying to figure out where it should go. Punted for now.
- # - AL 27 Nov 2011
- (@data_accessors ||= {})[clazz.name.to_sym] ||=
- Adapters::DataAccessor.create(clazz)
+ @regitry ||= Sunspot::Adapters::Registry.new
+ @regitry.retrieve(clazz)
end
end
end
View
26 sunspot/spec/api/adapters_spec.rb
@@ -31,3 +31,29 @@
end.should raise_error(Sunspot::NoAdapterError)
end
end
+
+describe Sunspot::Adapters::Registry, focus:true do
+ let(:registry){ Sunspot::Adapters::Registry.new }
+ let(:abstractclass_accessor){ Sunspot::Adapters::DataAccessor::for(AbstractModel) }
+ let(:superclass_accessor){ Sunspot::Adapters::DataAccessor::for(Model) }
+ let(:mixin_accessor){ Sunspot::Adapters::DataAccessor::for(MixModel) }
+
+ it "registers and retrieves a data accessor for abstractclass" do
+ registry.retrieve(AbstractModel).should be_a(abstractclass_accessor)
+ end
+
+ it "registers and retrieves a data accessor for superclass" do
+ registry.retrieve(Model).should be_a(superclass_accessor)
+ end
+
+ it "registers and retrieves a data accessor for mixin" do
+ registry.retrieve(MixModel).should be_a(mixin_accessor)
+ end
+
+ it "injects inherited attributes" do
+ AbstractModelDataAccessor.any_instance.stub(:inherited_attributes).and_return([:to_be_injected])
+ in_registry_data_accessor = registry.retrieve(AbstractModel)
+ in_registry_data_accessor.stub(:to_be_injected){ "value" }
+ registry.retrieve(Model).to_be_injected.should == "value"
+ end
+end
View
1 sunspot/spec/mocks/adapters.rb
@@ -8,6 +8,7 @@ class AbstractModelInstanceAdapter < Sunspot::Adapters::InstanceAdapter
end
class AbstractModelDataAccessor < Sunspot::Adapters::DataAccessor
+ attr_accessor :to_be_injected
end
Sunspot::Adapters::InstanceAdapter.register(AbstractModelInstanceAdapter, AbstractModel)
View
5 sunspot_rails/lib/sunspot/rails/adapters.rb
@@ -22,6 +22,11 @@ class ActiveRecordDataAccessor < Sunspot::Adapters::DataAccessor
# options for the find
attr_accessor :include, :select
+ def initialize(clazz)
+ super(clazz)
+ @inherited_attributes = [:include, :select]
+ end
+
#
# Set the fields to select from the database. This will be passed
# to ActiveRecord.

0 comments on commit 64d0985

Please sign in to comment.