From 3e2762061fd123433df4fc20e3c419b107cd9a1b Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Mon, 16 Aug 2021 10:29:14 -0600 Subject: [PATCH] cache all feature flags on root accounts since they are the most common target of feature flag predicates Change-Id: I2c6be68f442b2eb66f8cd5ad57cafbc83f114ba1 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271502 Tested-by: Service Cloud Jenkins Reviewed-by: Rob Orton QA-Review: Cody Cutrer Product-Review: Cody Cutrer --- app/models/account.rb | 1 + app/models/feature_flag.rb | 27 +++++++++++--------- lib/feature_flags.rb | 11 +++++--- spec/models/enrollment_spec.rb | 3 +++ spec/models/quizzes/quiz_spec.rb | 6 ++--- spec/views/courses/settings.html.erb_spec.rb | 3 ++- 6 files changed, 32 insertions(+), 19 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 20b6e3530c0..eca766f1721 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1574,6 +1574,7 @@ class Account < ActiveRecord::Base # an opportunity for plugins to load some other stuff up before caching the account def precache + feature_flags.load end class ::Canvas::AccountCacheError < StandardError; end diff --git a/app/models/feature_flag.rb b/app/models/feature_flag.rb index ae971be5c56..2001d119f31 100644 --- a/app/models/feature_flag.rb +++ b/app/models/feature_flag.rb @@ -64,19 +64,22 @@ class FeatureFlag < ActiveRecord::Base end def clear_cache - if self.context - self.class.connection.after_transaction_commit { self.context.feature_flag_cache.delete(self.context.feature_flag_cache_key(feature)) } - self.context.touch if Feature.definitions[feature].try(:touch_context) - if self.context.is_a?(Account) - if self.context.site_admin? - Switchman::DatabaseServer.send_in_each_region(self.context, :clear_cache_key, {}, :feature_flags) - else - self.context.clear_cache_key(:feature_flags) - end - end + if context + self.class.connection.after_transaction_commit do + context.feature_flag_cache.delete(context.feature_flag_cache_key(feature)) + context.touch if Feature.definitions[feature].try(:touch_context) || context.try(:root_account?) - if ::Rails.env.development? && self.context.is_a?(Account) && Account.all_special_accounts.include?(self.context) - Account.clear_special_account_cache!(true) + if context.is_a?(Account) + if context.site_admin? + Switchman::DatabaseServer.send_in_each_region(context, :clear_cache_key, {}, :feature_flags) + else + context.clear_cache_key(:feature_flags) + end + end + + if !::Rails.env.production? && context.is_a?(Account) && Account.all_special_accounts.include?(context) + Account.clear_special_account_cache!(true) + end end end end diff --git a/lib/feature_flags.rb b/lib/feature_flags.rb index 9c7e8fa90f4..bd0bd2d04f3 100644 --- a/lib/feature_flags.rb +++ b/lib/feature_flags.rb @@ -45,12 +45,12 @@ module FeatureFlags def set_feature_flag!(feature, state) feature = feature.to_s - flag = self.feature_flags.where(feature: feature).first - flag ||= self.feature_flags.build(feature: feature) + flag = feature_flags.find_or_initialize_by(feature: feature) flag.state = state @feature_flag_cache ||= {} @feature_flag_cache[feature] = flag flag.save! + association(:feature_flags).reset end def allow_feature!(feature) @@ -141,6 +141,10 @@ module FeatureFlags # find the highest flag that doesn't allow override, # or the most specific flag otherwise accounts = feature_flag_account_ids.map do |id| + # optimizations for accounts we likely already have loaded (including their feature flags!) + next Account.site_admin if id == Account.site_admin.global_id + next Account.current_domain_root_account if id == Account.current_domain_root_account&.global_id + account = Account.new account.id = id account.shard = Shard.shard_for(id, self.shard) @@ -150,9 +154,10 @@ module FeatureFlags all_contexts = (accounts + [self]).uniq all_contexts -= [self] if inherited_only - all_contexts.each_with_index do |context, idx| + all_contexts.each do |context| flag = context.feature_flag(feature, skip_cache: context == self && skip_cache) next unless flag + retval = flag break unless flag.can_override? end diff --git a/spec/models/enrollment_spec.rb b/spec/models/enrollment_spec.rb index 96400c892b8..a2a2d6698c2 100644 --- a/spec/models/enrollment_spec.rb +++ b/spec/models/enrollment_spec.rb @@ -2977,6 +2977,7 @@ describe Enrollment do before :each do course_with_student @course.root_account.enable_feature!(:granular_permissions_manage_users) + @enrollment.reload end it 'is true for a user who has been granted :manage_students' do @@ -3041,6 +3042,7 @@ describe Enrollment do before :each do course_with_observer @course.root_account.enable_feature!(:granular_permissions_manage_users) + @enrollment.reload end it 'is true with :manage_students' do @@ -3074,6 +3076,7 @@ describe Enrollment do before :each do course_with_teacher @course.root_account.enable_feature!(:granular_permissions_manage_users) + @enrollment.reload end it 'is false with :manage_students' do diff --git a/spec/models/quizzes/quiz_spec.rb b/spec/models/quizzes/quiz_spec.rb index f4fa939dcf5..110d11ddf65 100644 --- a/spec/models/quizzes/quiz_spec.rb +++ b/spec/models/quizzes/quiz_spec.rb @@ -1147,7 +1147,7 @@ describe Quizzes::Quiz do end it "returns false when feature flag is off" do - @course.root_account.set_feature_flag! 'timer_without_autosubmission', 'off' + @quiz.context.root_account.set_feature_flag! 'timer_without_autosubmission', 'off' expect(@quiz.timer_autosubmit_disabled?).to be(false) @quiz.update({ disable_timer_autosubmission: true @@ -1156,12 +1156,12 @@ describe Quizzes::Quiz do end it "returns false when feature flag is on and disable_timer_autosubmission is false" do - @course.root_account.set_feature_flag! 'timer_without_autosubmission', 'on' + @quiz.context.root_account.set_feature_flag! 'timer_without_autosubmission', 'on' expect(@quiz.timer_autosubmit_disabled?).to be(false) end it "returns true when feature flag is on and disable_timer_autosubmission is true" do - @course.root_account.set_feature_flag! 'timer_without_autosubmission', 'on' + @quiz.context.root_account.set_feature_flag! 'timer_without_autosubmission', 'on' @quiz.update({ disable_timer_autosubmission: true }) diff --git a/spec/views/courses/settings.html.erb_spec.rb b/spec/views/courses/settings.html.erb_spec.rb index 58ad6378716..7eae3b596dd 100644 --- a/spec/views/courses/settings.html.erb_spec.rb +++ b/spec/views/courses/settings.html.erb_spec.rb @@ -145,6 +145,7 @@ describe "courses/settings.html.erb" do end before :each do + @course.root_account.reload view_context(@course, @teacher) end @@ -247,7 +248,7 @@ describe "courses/settings.html.erb" do end context "with the feature enabled" do - before { Account.default.enable_feature!(:course_templates) } + before { @course.root_account.enable_feature!(:course_templates) } it "is visible" do render