skip feature flag cache when showing API results to admins

test plan:
* in a production-like environment, feature flag changes should
 not appear to be undone when refreshing the page (while the
 cache is still in the process of invalidating)

closes #ADMIN-2939

Change-Id: I495820850f8facaa6965188140d979bd5227994d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/211436
Tested-by: Jenkins
QA-Review: Anju Reddy <areddy@instructure.com>
Product-Review: James Williams <jamesw@instructure.com>
Reviewed-by: Rex Fleischer <rfleischer@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>

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

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
 

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?

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' })

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