Permalink
Browse files

[ACTIVEMODEL] [¡BREAKING!] Wrap search results in Item class, not the…

… actual model class [Closes #11]

Due to the problem with indexing and retrieving ActiveRecord models with associations (such as Article has_many :comments),
do not wrap the results in the model class, but in the Tire::Configuration.wrapper class.

See the #10 issue for explanation.

This commit breaks the old behaviour and usage in Rails, since Results::Item is not (yet) ActiveModel compatible.
  • Loading branch information...
1 parent caef831 commit 05a1331e122221bdf55dff84e72824b28dc06440 @karmi committed Jul 13, 2011
View
@@ -33,9 +33,6 @@ def to_hash
module ClassMethods
def search(query=nil, options={}, &block)
- old_wrapper = Tire::Configuration.wrapper
- Tire::Configuration.wrapper self
-
sort = Array( options[:order] || options[:sort] )
options = {:type => document_type}.update(options)
@@ -56,8 +53,6 @@ def search(query=nil, options={}, &block)
block.arity < 1 ? s.instance_eval(&block) : block.call(s)
s.perform.results
end
- ensure
- Tire::Configuration.wrapper old_wrapper
end
# Wrapper for the ES index for this class
@@ -30,12 +30,8 @@ def results
# Update the document with meta information
['_score', '_type', '_index', '_version', 'sort', 'highlight'].each { |key| document.update( {key => h[key]} || {} ) }
- # for instantiating ActiveRecord with arbitrary attributes and setting @new_record etc.
- if @wrapper.respond_to?(:instantiate, true)
- @wrapper.send(:instantiate, document)
- else
- @wrapper.new(document)
- end
+ # Return an instance of the "wrapper" class
+ @wrapper.new(document)
end
end
end
View
@@ -26,6 +26,10 @@ def id
self[:id]
end
+ def persisted?
+ !!id
+ end
+
def inspect
s = []; self.each { |k,v| s << "#{k}: #{v.inspect}" }
%Q|<Item #{s.join(', ')}>|
@@ -52,7 +52,7 @@ def teardown
# The model should find only 1 document
assert_equal 1, results.count
- assert_instance_of SupermodelArticle, results.first
+ assert_instance_of Results::Item, results.first
assert_equal 'Test', results.first.title
assert_not_nil results.first._score
assert_equal id, results.first.id
@@ -50,9 +50,9 @@ def teardown
assert_equal 1, results.count
- assert_instance_of ActiveRecordArticle, results.first
+ assert_instance_of Results::Item, results.first
assert_not_nil results.first.id
- assert_equal id, results.first.id
+ assert_equal id.to_s, results.first.id.to_s
assert results.first.persisted?, "Record should be persisted"
assert_not_nil results.first._score
assert_equal 'Test', results.first.title
@@ -90,20 +90,18 @@ class SearchTest < Test::Unit::TestCase
ActiveModelArticle.elasticsearch_index.refresh
end
- should "wrap results in proper class with ID and score and not change the original wrapper" do
+ should "wrap results in instances of the wrapper class" do
response = { 'hits' => { 'hits' => [{'_id' => 1, '_score' => 0.8, '_source' => { 'title' => 'Article' }}] } }
Configuration.client.expects(:get).returns(mock_response(response.to_json))
collection = ActiveModelArticle.search 'foo'
assert_instance_of Results::Collection, collection
- assert_equal Results::Item, Tire::Configuration.wrapper
-
document = collection.first
- assert_instance_of ActiveModelArticle, document
- assert_not_nil document._score
- assert_equal 1, document.id
+ assert_instance_of Results::Item, document
+ assert_not_nil document._score
+ assert_equal 1, document.id
assert_equal 'Article', document.title
end

0 comments on commit 05a1331

Please sign in to comment.