Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not dynamically load tenant model specified in autoload. #228

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ Once you are ready to enforce tenancy, make your tenant_id column NOT NULL and s
* **What if my tenant model is not defined in my application?**

The tenant model does not have to be defined. Use the gem as if the model was present. `MultiTenant.with` accepts either a tenant id or model instance.
However, you should pass `skip_reflection: true` when calling `multi_tenant()` to avoid generating a relation.

## Credits

Expand Down
4 changes: 2 additions & 2 deletions lib/activerecord-multi-tenant/model_extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def inherited(subclass)
partition_key = @partition_key

# Create an implicit belongs_to association only if tenant class exists
if MultiTenant.tenant_klass_defined?(tenant_name, options)
unless options[:skip_reflection]
belongs_to tenant_name, **options.slice(:class_name, :inverse_of, :optional)
.merge(foreign_key: options[:partition_key])
end
Expand Down Expand Up @@ -106,7 +106,7 @@ def inherited(subclass)
tenant_id
end

if MultiTenant.tenant_klass_defined?(tenant_name, options)
unless options[:skip_reflection]
define_method "#{tenant_name}=" do |model|
super(model)
if send("#{partition_key}_changed?") && persisted? && !send("#{partition_key}_was").nil?
Expand Down
9 changes: 0 additions & 9 deletions lib/activerecord-multi-tenant/multi_tenant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,6 @@ class Current < ::ActiveSupport::CurrentAttributes
attribute :tenant
end

def self.tenant_klass_defined?(tenant_name, options = {})
class_name = if options[:class_name].present?
options[:class_name]
else
tenant_name.to_s.classify
end
!!class_name.safe_constantize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class was dynamically loaded in this line.
Although top-level model can be checked with const_defined?(:X) or autoload?(:X), it is difficult for nested models such as A::B::Tenant, so I gave up the method of checking by the presence or absence of a class.

end

def self.partition_key(tenant_name)
"#{tenant_name.to_s.underscore}_id"
end
Expand Down
39 changes: 39 additions & 0 deletions spec/activerecord-multi-tenant/model_extensions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,45 @@
it { expect(@partition_key_not_model_task.non_model_id).to be 77 }
end

context 'Tenant model is defined in autoload' do
around do |example|
Object.autoload(:LazyTenant, "#{dir}/lazy_tenant")
example.run
ensure
Object.send(:remove_const, :LazyTenant)
end

let(:dir) do
Dir.mktmpdir
end

it "doesn't load constant" do
File.write("#{dir}/lazy_tenant.rb", <<~RUBY)
class LazyTenant < ActiveRecord::Base
end
RUBY

klass = Class.new(ActiveRecord::Base) do
self.table_name = Project.table_name

def self.name
'Dummy'
end
end

expect do
klass.multi_tenant(:lazy_tenant, partition_key: 'account_id')
end.to_not change {
[
defined?(LazyTenant),
autoload?(:LazyTenant)
]
}.from(['constant', "#{dir}/lazy_tenant"])

expect(klass.reflections).to have_key('lazy_tenant')
end
end

describe 'Tenant model with a nonstandard class name' do
let(:account_klass) do
Class.new(ActiveRecord::Base) do
Expand Down
54 changes: 0 additions & 54 deletions spec/activerecord-multi-tenant/multi_tenant_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,60 +65,6 @@
end
end

describe '.tenant_klass_defined?' do
context 'without options' do
before(:all) do
class SampleTenant < ActiveRecord::Base
multi_tenant :sample_tenant
end
end

it 'return true with valid tenant_name' do
expect(MultiTenant.tenant_klass_defined?(:sample_tenant)).to eq(true)
end

it 'return false with invalid_tenant_name' do
invalid_tenant_name = :tenant
expect(MultiTenant.tenant_klass_defined?(invalid_tenant_name)).to eq(false)
end
end

context 'with options' do
context 'and valid class_name' do
it 'return true' do
class SampleTenant < ActiveRecord::Base
multi_tenant :tenant
end

tenant_name = :tenant
options = {
class_name: 'SampleTenant'
}
expect(MultiTenant.tenant_klass_defined?(tenant_name, options)).to eq(true)
end

it 'return true when tenant class is nested' do
module SampleModule
class SampleNestedTenant < ActiveRecord::Base
multi_tenant :tenant
end
# rubocop:disable Layout/TrailingWhitespace
# Trailing whitespace is intentionally left here

class AnotherTenant < ActiveRecord::Base
end
# rubocop:enable Layout/TrailingWhitespace
end
tenant_name = :tenant
options = {
class_name: 'SampleModule::SampleNestedTenant'
}
expect(MultiTenant.tenant_klass_defined?(tenant_name, options)).to eq(true)
end
end
end
end

describe '.wrap_methods' do
context 'when method is already prepended' do
it 'is not an stack error' do
Expand Down
4 changes: 2 additions & 2 deletions spec/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,12 @@ class CustomPartitionKeyTask < ActiveRecord::Base
end

class PartitionKeyNotModelTask < ActiveRecord::Base
multi_tenant :non_model
multi_tenant :non_model, skip_reflection: true
end

class AbstractTask < ActiveRecord::Base
self.abstract_class = true
multi_tenant :non_model
multi_tenant :non_model, skip_reflection: true
end

class SubclassTask < AbstractTask
Expand Down