Skip to content
This repository has been archived by the owner on Jun 30, 2018. It is now read-only.

Commit

Permalink
[#218] Fixed the incorrect serialization of document_type in Index#…
Browse files Browse the repository at this point in the history
…bulk_store

Previously, when storing individual namespaced records, it correctly stored them as "my_namespace/my_model",
but bulk_store was saving _type as "my_namespace%2Fmy_model".

This resulted in two different document types where there should only be one, incorrect mapping, incorrect searches.

Closes #290, closes #291, fixes #218.
  • Loading branch information
sarmiena authored and karmi committed Mar 27, 2012
1 parent c483607 commit 4a92865
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 32 deletions.
10 changes: 7 additions & 3 deletions lib/tire/index.rb
Expand Up @@ -66,8 +66,8 @@ def store(*args)

def bulk_store(documents, options={})
payload = documents.map do |document|
type = get_type_from_document(document, :escape => false) # Do not URL-escape the _type
id = get_id_from_document(document)
type = get_type_from_document(document)

STDERR.puts "[ERROR] Document #{document.inspect} does not have ID" unless id

Expand Down Expand Up @@ -265,7 +265,9 @@ def logged(endpoint='/', curl='')
end
end

def get_type_from_document(document)
def get_type_from_document(document, options={})
options = {:escape => true}.merge(options)

old_verbose, $VERBOSE = $VERBOSE, nil # Silence Object#type deprecation warnings
type = case
when document.respond_to?(:document_type)
Expand All @@ -278,7 +280,9 @@ def get_type_from_document(document)
document.type
end
$VERBOSE = old_verbose
Utils.escape( type || 'document' )

type ||= 'document'
options[:escape] ? Utils.escape(type) : type
end

def get_id_from_document(document)
Expand Down
32 changes: 3 additions & 29 deletions test/integration/active_record_searchable_test.rb
Expand Up @@ -7,42 +7,18 @@ class ActiveRecordSearchableIntegrationTest < Test::Unit::TestCase

def setup
super
ActiveRecord::Base.establish_connection( :adapter => 'sqlite3', :database => ":memory:" )

ActiveRecord::Migration.verbose = false
ActiveRecord::Schema.define(:version => 1) do
create_table :active_record_articles do |t|
t.string :title
t.datetime :created_at, :default => 'NOW()'
end
create_table :active_record_comments do |t|
t.string :author
t.text :body
t.references :article
t.timestamps
end
create_table :active_record_stats do |t|
t.integer :pageviews
t.string :period
t.references :article
end
create_table :active_record_class_with_tire_methods do |t|
t.string :title
end
create_table :active_record_class_with_dynamic_index_names do |t|
t.string :title
end
end
Tire::ActiveRecordSchemaBuilder.build_schema!
end

context "ActiveRecord integration" do

setup do
setup do
ActiveRecordArticle.destroy_all
Tire.index('active_record_articles').delete

load File.expand_path('../../models/active_record_models.rb', __FILE__)
end

teardown do
ActiveRecordArticle.destroy_all
Tire.index('active_record_articles').delete
Expand Down Expand Up @@ -409,8 +385,6 @@ module ::Rails; end

context "with namespaced models" do
setup do
ActiveRecord::Schema.define { create_table(:active_record_namespace_my_models) { |t| t.string :title, :timestamp } }

ActiveRecordNamespace::MyModel.create :title => 'Test'
ActiveRecordNamespace::MyModel.tire.index.refresh
end
Expand Down
37 changes: 37 additions & 0 deletions test/models/active_record_models.rb
Expand Up @@ -120,3 +120,40 @@ class ActiveRecordNamespace::MyModel < ActiveRecord::Base
include Tire::Model::Search
include Tire::Model::Callbacks
end

module Tire
class ActiveRecordSchemaBuilder
def self.build_schema!
ActiveRecord::Base.establish_connection( :adapter => 'sqlite3', :database => ":memory:" )

ActiveRecord::Migration.verbose = false
ActiveRecord::Schema.define(:version => 1) do
create_table :active_record_articles do |t|
t.string :title
t.datetime :created_at, :default => 'NOW()'
end
create_table :active_record_comments do |t|
t.string :author
t.text :body
t.references :article
t.timestamps
end
create_table :active_record_stats do |t|
t.integer :pageviews
t.string :period
t.references :article
end
create_table :active_record_class_with_tire_methods do |t|
t.string :title
end
create_table :active_record_class_with_dynamic_index_names do |t|
t.string :title
end

create_table :active_record_namespace_my_models do |t|
t.string :title, :timestamp
end
end
end
end
end
24 changes: 24 additions & 0 deletions test/unit/index_test.rb
Expand Up @@ -400,6 +400,30 @@ class MyDocument;end; document = MyDocument.new

end

context "namespaced instances" do
setup do
load File.expand_path('../../models/active_record_models.rb', __FILE__)
Tire::ActiveRecordSchemaBuilder.build_schema!
end

should "serialize namespaced ActiveRecord objects" do
Configuration.client.expects(:post).with do |url, json|
url == "#{Configuration.url}/_bulk" &&
json =~ /"_index":"active_record_namespace_my_models"/ &&
json =~ /"_type":"active_record_namespace\/my_model"/ &&
json =~ /"_id":"1"/ &&
json =~ /"_id":"2"/ &&
json =~ /"title":"One"/ &&
json =~ /"title":"Two"/
end.returns(mock_response('{}', 200))

one = ActiveRecordNamespace::MyModel.new 'title' => 'One'; one.id = '1'
two = ActiveRecordNamespace::MyModel.new 'title' => 'Two'; two.id = '2'

ActiveRecordNamespace::MyModel.index.bulk_store [ one, two ]
end
end

should "try again when an exception occurs" do
Configuration.client.expects(:post).returns(mock_response('Server error', 503)).at_least(2)

Expand Down

0 comments on commit 4a92865

Please sign in to comment.