pat / thinking-sphinx

Sphinx/Manticore plugin for ActiveRecord/Rails
http://freelancing-gods.com/thinking-sphinx
MIT License
1.63k stars 468 forks source link

Thinking sphinx 3.0.5 generates wrong configuration file with 2 database setup #597

Closed icem closed 10 years ago

icem commented 11 years ago

I'm using sphinx with Rails 3.2.14 and I have indeces from 2 databases. Model User uses secondary database connection through ActiveRecord::Base#establish_connection. Previous thinking_sphinx version 3.0.3 generated configuration correctly, using secondary db. But after upgrading to 3.0.5 I see that all indeces in sphinx config are using the same default db. So when doing indexing step I get

ERROR: index 'user_core': sql_query_range: : range-query failed: Table 's2_development.users' doesn't exist (DSN=mysql://root:***@localhost:3306/s2_development)
icem commented 11 years ago

I found that commit introduced the issue https://github.com/pat/thinking-sphinx/commit/117fad9b425d6514e2810303cce25a139bc60d60 But how it's supposed to work with 2 databases?

pat commented 11 years ago

Can you share your user index definition here?

icem commented 11 years ago

Nothing special actually.

#app/indices/user_index.rb
ThinkingSphinx::Index.define :user, :with => :active_record do #TODO delayed deltas or smth like that
  indexes UserNaming::FULL_NAME_SQL, :as => :full_name, :sortable => true
  indexes email
  indexes curriculum_vitae_contents
  indexes sector.term_name, :as => :sector_name, :sortable => true
  indexes current_location.city.name, :as => :current_city, :sortable => true
  indexes [ current_employer.title, current_employer.name ], :as => :current_employer_summary, :sortable => true
  has active
  has created_at
end

#app/models/user.rb
class User < ActiveRecord::Base
  establish_connection "xx_#{Rails.env}"
pat commented 11 years ago

Try the following:

ThinkingSphinx::Index.define :user, :with => :active_record do
  ::ActiveRecord::Base.establish_connection "xx_#{Rails.env}"

  # ... normal index definition

  ::ActiveRecord::Base.establish_connection Rails.env
end
icem commented 11 years ago

That works, of course, but it's a hack. I don't want to establish connection in every index file which uses different db. (Actually my User model inherited from abstract class with #establish_connection) I just can't get the purpose of https://github.com/pat/thinking-sphinx/commit/117fad9b425d6514e2810303cce25a139bc60d60 change. Normally I would need same db connection in AR model and Sphinx index.

pat commented 11 years ago

From memory, I think the use-case was a multi-tenancy app (with each tenant using a different database) and so index definitions ended up being like so:

ThinkingSphinx::Index.define :user ... do
  ::Tenant.all.each do |tenant|
    ::ActiveRecord::Base.establish_connection tenant.db_credentials
    define_source "user_#{tenant.name}" do
      # ...
    end
  end
end

Hence, the database connection that was captured was reflected in the overall connection, not for the model in question. I'm open to suggestions to have both behaviours work.

icem commented 11 years ago

Unfortunately, Rails wasn't designed to build multitenancy apps out of the box. I would rewrite you example like that:

ThinkingSphinx::Index.define :user ... do
  ::Tenant.all.each do |tenant|
    ::User.establish_connection tenant.db_credentials
    define_source "user_#{tenant.name}" do
      # ...
    end
  end
end

Multitenancy apps anyway will have both per-client and shared databases (for ex. database of clients), so it's preferred to switch connection per-model rather than globally. (Just my thoughts, I don't have that experience) If you want to keep current code, it's ok, but it would be helpful to see that in docs.

karbyshevds commented 10 years ago

Pat, this new abilities is good, but not for all cases. If i have application where models stored in different databases, in previous versions i could simply inherit from abstract model to transparently grant access to models from rails and from idexes bunt now, according to example i'll be forced to declare connections again & it's very sad. What if do so:

@database_settings = model.connection.instance_variable_get(:@config) === ::ActiveRecord::Base.connection.instance_variable_get(:@config) ? ::ActiveRecord::Base.connection.instance_variable_get(:@config) : model.connection.instance_variable_get(:@config)

Thus if model inherited from other with different connector you will use it and will use default connection in all other cases. What are you thinking?

P.S.: sorry for bad english

pat commented 10 years ago

Okay, I've reverted commit 117fad9b425d6514e2810303cce25a139bc60d60, and added set_database within index definitions, which accepts either an environment name (symbol or string), or a set of database credentials as a hash (same as what you'd find under any given environment in config/database.yml). This means you likely won't need to use establish_connection at any point within your index definitions.

icem commented 10 years ago

Tried from develop branch, works as expected. Thanks @pat :+1: