Merge pull request #3875 from 3scale/tenant_id

tenant_id related fixes

app/models/account/buyer_methods.rb

@@ -25,7 +25,7 @@ module Account::BuyerMethods
 
     alias_method :application_contracts, :bought_cinstances
 
-    has_many :contracts, foreign_key: :user_account_id, dependent: :destroy
+    has_many :contracts, foreign_key: :user_account_id, dependent: :destroy, inverse_of: :user_account
 
     module UniqueAssociation
       # Oracle can't do DISTINCT when there are TEXT columns

app/models/cms/group.rb

@@ -4,7 +4,6 @@ class CMS::Group < ApplicationRecord
   # This is BuyerGroup
   self.table_name = :cms_groups
 
-  belongs_to :tenant
   belongs_to :provider, :class_name => "Account"
 
   validates :name, :provider, presence: true, length: { maximum: 255 }

app/models/cms/group_section.rb

@@ -4,5 +4,4 @@ class CMS::GroupSection < ApplicationRecord
   attr_accessible :section, :group
   belongs_to :group, :class_name => 'CMS::Group'
   belongs_to :section, :class_name => 'CMS::Section'
-  belongs_to :tenant
 end

app/models/contract.rb

@@ -35,7 +35,7 @@ class Contract < ApplicationRecord
   validates :user_key, length: { maximum: 256 }
 
   # TODO: rename to buyer_account and remove alias
-  belongs_to :user_account, class_name: 'Account', autosave: false
+  belongs_to :user_account, class_name: 'Account', autosave: false, inverse_of: :contracts
 
   alias buyer_account user_account
   alias buyer         user_account

app/models/features_plan.rb

@@ -1,6 +1,4 @@
 class FeaturesPlan < ApplicationRecord
-  self.primary_key = nil # this column does not exist, but rails needs to have something
-
   belongs_to :feature
   belongs_to :plan, :polymorphic => true
 

app/models/oidc_configuration.rb

@@ -55,7 +55,7 @@ def attributes
     end
   end
 
-  belongs_to :oidc_configurable, polymorphic: true
+  belongs_to :oidc_configurable, polymorphic: true, inverse_of: :oidc_configuration
   serialize :config, OIDCConfiguration::Config
 
   delegate(*OIDCConfiguration::Config::ATTRIBUTES, to: :config)

app/models/plan.rb

@@ -12,7 +12,7 @@ class PeriodRangeCalculationError < StandardError; end
   include SystemName
   include Logic::MetricVisibility::Plan
 
-  self.background_deletion = [:cinstances, :contracts, :plan_metrics, :pricing_rules,
+  self.background_deletion = [:contracts, :plan_metrics, :pricing_rules,
                               :usage_limits, [:customizations, { action: :destroy, class_name: 'Plan' }]]
 
   has_system_name :uniqueness_scope => [ :type, :issuer_id, :issuer_type ]
@@ -73,8 +73,6 @@ class PeriodRangeCalculationError < StandardError; end
   # But calling `#lock!` will call `#reload` so some instance variables are reset
   before_destroy :avoid_destruction, prepend: true
 
-  has_many :cinstances, :dependent => :destroy
-
   has_many :contracts, dependent: :destroy
 
   # TODO: move this to application plan, but beware

lib/tasks/multitenant/tenants.rake

@@ -68,6 +68,15 @@ namespace :multitenant do
       update_tenant_ids(proc { |object| object.account.tenant_id }, proc { account }, proc { tenant_id == nil }, args.to_hash.merge({ table_name: 'Alert' }))
     end
 
+    desc 'validate tenant_id integrity'
+    task :integrity => :environment do
+      require "three_scale/tenant_id_integrity_checker"
+
+      inconsistent = ThreeScale::TenantIDIntegrityChecker.new.check
+
+      Rails.logger.error "Inconsistent tenant_ids for:\n#{inconsistent.map {_1.join(" ")}.join("\n")}"
+    end
+
     def update_tenant_ids(tenant_id_block, association_block, condition, **args)
       query = args[:table_name].constantize.joining(&association_block).where.has(&condition)
       puts "------ Updating #{args[:table_name]} ------"

lib/three_scale/tenant_id_integrity_checker.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true
+
+module ThreeScale
+  class TenantIDIntegrityChecker
+    attr_reader :tenant_id
+
+    def initialize(attribute = :tenant_id)
+      @attribute = attribute
+    end
+
+    def check
+      processed = []
+
+      models_with_tenant_id.inject([]) do |inconsistent_found, model|
+        Rails.logger.info "Tenant integrity of #{model}"
+        inconsistent_found.concat associated_inconsistent_pairs(model, processed: processed)
+      end
+    end
+
+    private
+
+    def associated_inconsistent_pairs(model, processed: [])
+      model.reflect_on_all_associations.inject([]) do |inconsistent_found, association|
+        next inconsistent_found if can_skip_asociation?(association, processed: processed)
+
+        processed << association
+
+        inconsistent_found.concat inconsistent_pairs_for(model, association)
+      end
+    end
+
+    def inconsistent_pairs_for(model, association)
+      table = model.arel_table.name
+      table_alias = last_table_alias_from_sql(model.joins(association.name).to_sql)
+      found = model.joins(association.name).where.not("#{table_alias}.tenant_id = #{table}.tenant_id")
+      found = found.merge(Account.where(provider: false).or(Account.where(provider: nil))) if model == Account && association.name == :provider_account
+      found = pluck_pks(found, association: association, table: table, assoc_table: table_alias)
+      found.map { ["#{model}#{_1}", association.name, "#{association.klass}#{_2}"] }
+    end
+
+    def pluck_pks(joined_relation, association:, assoc_table:, table:)
+      model_pk = pk_fields association.active_record
+      assoc_pk = pk_fields association.klass
+      res = joined_relation.reorder('').pluck(*model_pk.map{"#{table}.#{_1}"}, *assoc_pk.map{"#{assoc_table}.#{_1}"})
+      res.map { [_1.slice(0, model_pk.size), _1.slice(model_pk.size..-1)] }
+    end
+
+    def pk_fields(model)
+      # in oracle-enhanced model.connection.schema_cache.primary_keys returns nil for composite so can't use the cache
+      model.primary_key ? Array(model.primary_key) : model.connection.primary_keys(model.table_name)
+    end
+
+    def can_skip_asociation?(association, processed: [])
+      # we can ignore these as they can't be automatically excluded but are redundant for the check anyway
+      ignored = {
+        Service => %i[all_metrics], # all metrics of service and APIs used by service so is redundant
+        Account => %i[provider_accounts], # only master has this and it is normal that all will mismatch
+      }
+      model = association.active_record
+
+      return true if ignored[model]&.include?(association.name)
+
+      # we live in a perfect world where all associations have an inverse so we can skip polymorphic ones
+      return true if association.polymorphic?
+
+      # arity can be one when association has a scope defined with a proc taking current object as argument
+      # We can't handle such associations but we can ignore them if the inverse one we can handle
+      if association.scope&.arity&.public_send(:>, 0)
+        return true unless association.inverse_of.polymorphic? || association.inverse_of.scope&.arity&.public_send(:>, 0)
+        raise "we can't handle #{association.name} of #{model}"
+      end
+
+      return true unless association.klass.attribute_names.include?("tenant_id")
+
+      # skip indirect associations where the "through association" has tenant_id, because we will check that
+      #   indirect association through the "through association" later (or we did already)
+      return true if association.through_reflection&.try(:klass)&.attribute_names&.include?("tenant_id")
+
+      processed.any? {_1 == association || _1 == association.inverse_of }
+    end
+
+    def last_table_alias_from_sql(sql)
+      matcher = /.*INNER JOIN [`'"]([\S]+)[`'"] (?:[`'"]([\S]+)[`'"] )?ON/i.match(sql)
+      matcher[2] || matcher[1]
+    end
+
+    def models_with_tenant_id
+      Rails.autoloaders.main.eager_load_dir("#{Rails.root}/app/models")
+      all_models = ApplicationRecord.descendants.select(&:arel_table).reject(&:abstract_class?)
+      all_models.select! { _1.attribute_names.include? "tenant_id" }
+    end
+  end
+end

test/unit/plan_test.rb

@@ -12,7 +12,7 @@ class PlanTest < ActiveSupport::TestCase
   should validate_numericality_of :cost_per_month
 
   should belong_to :issuer
-  should have_many(:cinstances).dependent(:destroy)
+  should have_many(:contracts).dependent(:destroy)
   should have_many(:customizations).dependent(:destroy)
 
   should belong_to :original

test/unit/tasks/multitenant/tenants_integrity_test.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+module Tasks
+  module Multitenant
+    class TenantsIntegrityTest < ActiveSupport::TestCase
+      test "reports tenant lack of integrity with belongs_to associations" do
+        provider = FactoryBot.create(:simple_provider)
+        wrong_buyers = FactoryBot.create_list(:simple_buyer, 2, provider_account: provider, tenant_id: provider.id + 1)
+        FactoryBot.create(:simple_buyer, provider_account: provider, tenant_id: provider.id)
+
+        expected_lines = ["Inconsistent tenant_ids for:"]
+        expected_lines.concat(wrong_buyers.map { |buyer| "Account[#{buyer.id}] provider_account Account[#{provider.id}]" })
+
+        Rails.logger.expects(:error).with { |msg| expected_lines.all? { msg.include?(_1) } }
+
+        execute_rake_task "multitenant/tenants.rake", "multitenant:tenants:integrity"
+      end
+
+      test "reports tenant lack of integrity with has_many associations" do
+        plan = FactoryBot.create(:application_plan)
+        wrong_cinstances = FactoryBot.create_list(:cinstance, 2, plan: plan, tenant_id: 0)
+        FactoryBot.create(:cinstance, plan: plan)
+
+        expected_lines = ["Inconsistent tenant_ids for:"]
+        wrong_cinstances.each do |cinstance|
+          expected_lines << "Account[#{cinstance.user_account.id}] bought_cinstances Cinstance[#{cinstance.id}]"
+          expected_lines << "Account[#{cinstance.user_account.id}] contracts Contract[#{cinstance.id}]"
+          expected_lines << "Contract[#{cinstance.id}] plan Plan[#{cinstance.plan.id}]"
+          expected_lines << "Cinstance[#{cinstance.id}] plan ApplicationPlan[#{cinstance.plan.id}]"
+          expected_lines << "Cinstance[#{cinstance.id}] service Service[#{cinstance.service.id}]"
+        end
+
+        Rails.logger.expects(:error).with { |msg| expected_lines.all? { msg.include?(_1) } }
+
+        execute_rake_task "multitenant/tenants.rake", "multitenant:tenants:integrity"
+      end
+
+      test "reports tenant lack of integrity with complex primary keys" do
+        plan = FactoryBot.create(:application_plan)
+        feature = FactoryBot.create(:feature, featurable: plan.issuer, tenant_id: 0)
+        plan.features << feature
+
+        expected_lines = ["Inconsistent tenant_ids for:"]
+        expected_lines << "Plan[#{plan.id}] features_plans FeaturesPlan[#{plan.id}, #{feature.id}]"
+        expected_lines << "Service[#{plan.issuer.id}] features Feature[#{feature.id}]"
+
+        Rails.logger.expects(:error).with { |msg| expected_lines.all? { msg.include?(_1) } }
+
+        execute_rake_task "multitenant/tenants.rake", "multitenant:tenants:integrity"
+      end
+    end
+  end
+end

test/workers/set_tenant_id_worker_test.rb

@@ -32,18 +32,20 @@ class BatchEnqueueWorkerTest < ActiveSupport::TestCase
     end
 
     test "account relations are also fixed for buyer accounts" do
-      buyer = FactoryBot.create(:buyer_account)
-      provider = buyer.provider_account
+      plan = FactoryBot.create(:simple_application_plan)
+      provider = plan.provider_account
+      buyer = FactoryBot.create(:simple_buyer, provider_account: provider)
+      cinstance = FactoryBot.create(:simple_cinstance, user_account: buyer, plan: plan)
 
       assert_equal provider.id, buyer.reload.tenant_id
 
-      alert_provider = FactoryBot.create(:limit_alert, account: provider)
-      alert_buyer = FactoryBot.create(:limit_alert, account: buyer)
+      alert_provider = FactoryBot.create(:limit_alert, account: provider, cinstance: cinstance)
+      alert_buyer = FactoryBot.create(:limit_alert, account: buyer, cinstance: cinstance)
 
       [alert_provider, alert_buyer].each do |alert|
         assert_equal provider.id, alert.reload.tenant_id
-        alert.tenant_id = nil
-        assert_nil alert.tenant_id
+        alert.update_column(:tenant_id, nil)
+        assert_nil alert.reload.tenant_id
       end
 
       perform_enqueued_jobs(only: [SetTenantIdWorker, SetTenantIdWorker::ModelTenantIdWorker]) do
@@ -54,6 +56,28 @@ class BatchEnqueueWorkerTest < ActiveSupport::TestCase
       assert_equal provider.id, alert_buyer.reload.tenant_id
     end
 
+    test "big" do
+      plan = FactoryBot.create(:simple_application_plan)
+      provider = plan.provider_account
+      buyers = FactoryBot.create_list(:simple_buyer, 20, provider_account: provider)
+      buyers.each do |buyer|
+        cinstance = FactoryBot.create(:simple_cinstance, user_account: buyer, plan: plan)
+        FactoryBot.create(:limit_alert, account: provider, cinstance: cinstance)
+        FactoryBot.create_list(:limit_alert, 300, account: buyer, cinstance: cinstance)
+      end
+
+      Alert.update_all(tenant_id: nil)
+
+      assert_nil Alert.take.tenant_id
+      assert_equal 22, Account.all.count
+
+      perform_enqueued_jobs(only: [SetTenantIdWorker, SetTenantIdWorker::ModelTenantIdWorker]) do
+        SetTenantIdWorker::BatchEnqueueWorker.new.perform("alerts")
+      end
+
+      assert_empty Alert.where(tenant_id: nil)
+    end
+
     test "raises on empty params" do
       assert_raises do
         SetTenantIdWorker::BatchEnqueueWorker.new.perform