From 63fe87c32b762bafbfe4c53dc36b36f4a77c7857 Mon Sep 17 00:00:00 2001 From: alpaca-tc Date: Thu, 18 Jan 2024 23:29:11 +0900 Subject: [PATCH 1/2] Do not dynamically load models specified in autoload. The basic way of Rails is that application code in the development environment is lazy loaded. For this reason, `class_name` is passed as a string rather than a class in the reflection definition, and the string is not constantized until it is evaluated. This is done to reduce performance issues and the complex locking load of constant-loading logic. Now activerecord-multitenant is loading the class specified in the argument to multi_tenant() without lazy loading. This conditional statement was added at https://github.com/citusdata/activerecord-multi-tenant/pull/6, but I have been fixed. Note that for projects where the tenant model does not exist, the existing behavior is supported by specifying `skip_reflection: true`. --- README.md | 1 + .../model_extensions.rb | 4 +- .../model_extensions_spec.rb | 39 +++++++++++++++++++ spec/schema.rb | 4 +- 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 34e97d8b..033a0927 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/lib/activerecord-multi-tenant/model_extensions.rb b/lib/activerecord-multi-tenant/model_extensions.rb index a2811130..9044910d 100644 --- a/lib/activerecord-multi-tenant/model_extensions.rb +++ b/lib/activerecord-multi-tenant/model_extensions.rb @@ -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 @@ -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? diff --git a/spec/activerecord-multi-tenant/model_extensions_spec.rb b/spec/activerecord-multi-tenant/model_extensions_spec.rb index a49af989..39860e34 100644 --- a/spec/activerecord-multi-tenant/model_extensions_spec.rb +++ b/spec/activerecord-multi-tenant/model_extensions_spec.rb @@ -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 diff --git a/spec/schema.rb b/spec/schema.rb index 517cf965..cc766481 100644 --- a/spec/schema.rb +++ b/spec/schema.rb @@ -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 From 55863dba4cb378196a4bca5b59eebc0bc8c6105a Mon Sep 17 00:00:00 2001 From: alpaca-tc Date: Thu, 18 Jan 2024 23:44:10 +0900 Subject: [PATCH 2/2] Remove unused method --- lib/activerecord-multi-tenant/multi_tenant.rb | 9 ---- .../multi_tenant_spec.rb | 54 ------------------- 2 files changed, 63 deletions(-) diff --git a/lib/activerecord-multi-tenant/multi_tenant.rb b/lib/activerecord-multi-tenant/multi_tenant.rb index e03008dc..45cf86f1 100644 --- a/lib/activerecord-multi-tenant/multi_tenant.rb +++ b/lib/activerecord-multi-tenant/multi_tenant.rb @@ -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 - end - def self.partition_key(tenant_name) "#{tenant_name.to_s.underscore}_id" end diff --git a/spec/activerecord-multi-tenant/multi_tenant_spec.rb b/spec/activerecord-multi-tenant/multi_tenant_spec.rb index f2f9fca9..dd54c9e3 100644 --- a/spec/activerecord-multi-tenant/multi_tenant_spec.rb +++ b/spec/activerecord-multi-tenant/multi_tenant_spec.rb @@ -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