diff --git a/app/controllers/feature_flags_controller.rb b/app/controllers/feature_flags_controller.rb index 17588307b01..60886ab71eb 100644 --- a/app/controllers/feature_flags_controller.rb +++ b/app/controllers/feature_flags_controller.rb @@ -155,8 +155,15 @@ class FeatureFlagsController < ApplicationController route = polymorphic_url([:api_v1, @context, :features]) features = Feature.applicable_features(@context) features = Api.paginate(features, self, route) + + skip_cache = @context.grants_right?(@current_user, session, :manage_feature_flags) + @context.feature_flags.load if skip_cache + flags = features.map { |fd| - @context.lookup_feature_flag(fd.feature, Account.site_admin.grants_right?(@current_user, session, :read)) + @context.lookup_feature_flag(fd.feature, + override_hidden: Account.site_admin.grants_right?(@current_user, session, :read), + skip_cache: skip_cache + ) }.compact render json: flags.map { |flag| feature_with_flag_json(flag, @context, @current_user, session) } end @@ -202,7 +209,10 @@ class FeatureFlagsController < ApplicationController return render json: { message: "missing feature parameter" }, status: :bad_request unless params[:feature].present? feature = params[:feature] raise ActiveRecord::RecordNotFound unless Feature.definitions.has_key?(feature.to_s) - flag = @context.lookup_feature_flag(feature, Account.site_admin.grants_right?(@current_user, session, :read)) + flag = @context.lookup_feature_flag(feature, + override_hidden: Account.site_admin.grants_right?(@current_user, session, :read), + skip_cache: @context.grants_right?(@current_user, session, :manage_feature_flags) + ) raise ActiveRecord::RecordNotFound unless flag render json: feature_flag_json(flag, @context, @current_user, session) end @@ -234,8 +244,7 @@ class FeatureFlagsController < ApplicationController return render json: { message: "invalid feature" }, status: :bad_request unless feature_def && feature_def.applies_to_object(@context) # check whether the feature is locked - @context.feature_flag_cache.delete(@context.feature_flag_cache_key(params[:feature])) - current_flag = @context.lookup_feature_flag(params[:feature]) + current_flag = @context.lookup_feature_flag(params[:feature], skip_cache: true) if current_flag return render json: { message: "higher account disallows setting feature flag" }, status: :forbidden if current_flag.locked?(@context) prior_state = current_flag.state diff --git a/app/models/feature_flag.rb b/app/models/feature_flag.rb index 500d736daf2..4f372bfec15 100644 --- a/app/models/feature_flag.rb +++ b/app/models/feature_flag.rb @@ -34,7 +34,7 @@ class FeatureFlag < ActiveRecord::Base def unhides_feature? return false unless Feature.definitions[feature].hidden? return true if context.is_a?(Account) && context.site_admin? - parent_setting = Account.find(context.feature_flag_account_ids.last).lookup_feature_flag(feature, true) + parent_setting = Account.find(context.feature_flag_account_ids.last).lookup_feature_flag(feature, override_hidden: true) parent_setting.nil? || parent_setting.hidden? end diff --git a/lib/feature_flags.rb b/lib/feature_flags.rb index eb46a383e89..4028429ad1c 100644 --- a/lib/feature_flags.rb +++ b/lib/feature_flags.rb @@ -69,13 +69,20 @@ module FeatureFlags # return the feature flag for the given feature that is defined on this object, if any. # (helper method. use lookup_feature_flag to test policy.) - def feature_flag(feature) + def feature_flag(feature, skip_cache: false) return nil unless self.id - RequestCache.cache("feature_flag", self, feature) do - self.shard.activate do - result = feature_flag_cache.fetch(feature_flag_cache_key(feature)) do - # keep have the context association unloaded in case we can't marshal it - FeatureFlag.where(feature: feature.to_s).polymorphic_where(:context => self).first + + self.shard.activate do + if self.feature_flags.loaded? + self.feature_flags.detect{|ff| ff.feature == feature.to_s} + elsif skip_cache + self.feature_flags.where(feature: feature.to_s).first + else + result = RequestCache.cache("feature_flag", self, feature) do + feature_flag_cache.fetch(feature_flag_cache_key(feature)) do + # keep have the context association unloaded in case we can't marshal it + FeatureFlag.where(feature: feature.to_s).polymorphic_where(:context => self).first + end end result.context = self if result result @@ -101,7 +108,7 @@ module FeatureFlags # find the feature flag setting that applies to this object # it may be defined on the object or inherited - def lookup_feature_flag(feature, override_hidden = false) + def lookup_feature_flag(feature, override_hidden: false, skip_cache: false) feature = feature.to_s feature_def = Feature.definitions[feature] raise "no such feature - #{feature}" unless feature_def @@ -129,7 +136,7 @@ module FeatureFlags account end (accounts + [self]).each do |context| - flag = context.feature_flag(feature) + flag = context.feature_flag(feature, skip_cache: skip_cache) next unless flag retval = flag break unless flag.allowed? diff --git a/spec/apis/v1/feature_flags_api_spec.rb b/spec/apis/v1/feature_flags_api_spec.rb index 2626af5ce1a..483485bdf61 100644 --- a/spec/apis/v1/feature_flags_api_spec.rb +++ b/spec/apis/v1/feature_flags_api_spec.rb @@ -229,6 +229,21 @@ describe "Feature Flags API", type: :request do {}, {}, { expected_status: 404 }) end + it "should skip cache for admins" do + original = t_root_account.method(:lookup_feature_flag) + @checked = false + allow_any_instantiation_of(t_root_account).to receive(:lookup_feature_flag) do |feature, opts| + if feature.to_s == "root_account_feature" + @checked = true + expect(opts[:skip_cache]).to eq true + end + original.call(feature, *opts) + end + api_call_as_user(t_root_admin, :get, "/api/v1/accounts/#{t_root_account.id}/features/flags/root_account_feature", + { controller: 'feature_flags', action: 'show', format: 'json', account_id: t_root_account.to_param, feature: 'root_account_feature' }) + expect(@checked).to eq true # should actually check the expectation + end + it "should return the correct format" do json = api_call_as_user(t_teacher, :get, "/api/v1/users/#{t_teacher.id}/features/flags/user_feature", { controller: 'feature_flags', action: 'show', format: 'json', user_id: t_teacher.to_param, feature: 'user_feature' }) diff --git a/spec/lib/feature_flags_spec.rb b/spec/lib/feature_flags_spec.rb index e06818eef76..d8c0ec82f37 100644 --- a/spec/lib/feature_flags_spec.rb +++ b/spec/lib/feature_flags_spec.rb @@ -230,23 +230,23 @@ describe FeatureFlags do end it "should find hidden features if override_hidden is given" do - expect(t_site_admin.lookup_feature_flag('hidden_feature', true)).to be_default - expect(t_root_account.lookup_feature_flag('hidden_feature', true)).to be_default - expect(t_sub_account.lookup_feature_flag('hidden_feature', true)).to be_default - expect(t_course.lookup_feature_flag('hidden_feature', true)).to be_default - expect(t_user.lookup_feature_flag('hidden_user_feature', true)).to be_default + expect(t_site_admin.lookup_feature_flag('hidden_feature', override_hidden: true)).to be_default + expect(t_root_account.lookup_feature_flag('hidden_feature', override_hidden: true)).to be_default + expect(t_sub_account.lookup_feature_flag('hidden_feature', override_hidden: true)).to be_default + expect(t_course.lookup_feature_flag('hidden_feature', override_hidden: true)).to be_default + expect(t_user.lookup_feature_flag('hidden_user_feature', override_hidden: true)).to be_default end it "should not create the implicit-off root_opt_in flag" do - flag = t_root_account.lookup_feature_flag('hidden_root_opt_in_feature', true) + flag = t_root_account.lookup_feature_flag('hidden_root_opt_in_feature', override_hidden: true) expect(flag).to be_default expect(flag).to be_hidden end it "override_hidden should not trump root_opt_in" do - expect(t_root_account.lookup_feature_flag('hidden_root_opt_in_feature', true)).to be_default - expect(t_sub_account.lookup_feature_flag('hidden_root_opt_in_feature', true)).to be_nil - expect(t_course.lookup_feature_flag('hidden_root_opt_in_feature', true)).to be_nil + expect(t_root_account.lookup_feature_flag('hidden_root_opt_in_feature', override_hidden: true)).to be_default + expect(t_sub_account.lookup_feature_flag('hidden_root_opt_in_feature', override_hidden: true)).to be_nil + expect(t_course.lookup_feature_flag('hidden_root_opt_in_feature', override_hidden: true)).to be_nil end end @@ -367,8 +367,8 @@ describe FeatureFlags do enable_cache do t_root_account.feature_flag('course_feature2') expect(Rails.cache).to be_exist(t_root_account.feature_flag_cache_key('course_feature2')) - expect(t_root_account).to receive(:feature_flags).never - t_root_account.feature_flag('course_feature2') + expect(FeatureFlag).to receive(:where).never + t_root_account.reload.feature_flag('course_feature2') end end @@ -387,5 +387,16 @@ describe FeatureFlags do expect(Rails.cache).not_to be_exist(t_cache_key) end end + + it "should skip the cache if requested" do + enable_cache do + flag = t_root_account.feature_flag('course_feature') + expect(flag.state).to eq 'allowed' + allow(flag).to receive(:clear_cache).and_return(true) # pretend it was delayed + flag.update_attribute(:state, 'on') # update in db + expect(t_root_account.feature_flag('course_feature').state).to eq 'allowed' # still pulls from cache + expect(t_root_account.feature_flag('course_feature', skip_cache: true).state).to eq 'on' # skips it + end + end end end