improve feature flag caching

 * properly cache nil per context
 * cache the full lookup in-proc only

test plan:
 * quick regression test of feature flags

Change-Id: I0d2435777bcc82d01301af96df81f179366b2fdc
Reviewed-on: https://gerrit.instructure.com/28118
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Matt Fairbourn <mfairbourn@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

Author: ccutrer

lib/feature_flags.rb

@@ -65,9 +65,11 @@ def feature_flag_cache_key(feature)
   # (helper method.  use lookup_feature_flag to test policy.)
   def feature_flag(feature)
     self.shard.activate do
-      Rails.cache.fetch(feature_flag_cache_key(feature)) do
-        self.feature_flags.where(feature: feature.to_s).first
+      result = Rails.cache.fetch(feature_flag_cache_key(feature)) do
+        self.feature_flags.where(feature: feature.to_s).first || :nil
       end
+      result = nil if result == :nil
+      result
     end
   end
 
@@ -99,6 +101,9 @@ def lookup_feature_flag(feature, override_hidden = false)
     # inherit the feature definition as a default unless it's a hidden feature
     retval = feature_def unless feature_def.hidden? && !is_site_admin && !(is_root_account && override_hidden)
 
+    @feature_flag_cache ||= {}
+    return @feature_flag_cache[feature] if @feature_flag_cache.key?(feature)
+
     # find the highest flag that doesn't allow override,
     # or the most specific flag otherwise
     account_ids = feature_flag_account_ids
@@ -119,11 +124,11 @@ def lookup_feature_flag(feature, override_hidden = false)
         retval = self.feature_flags.build feature: feature, state: 'off'
       else
         # the feature doesn't exist beneath the root account until the root account opts in
-        return nil
+        return @feature_flag_cache[feature] = nil
       end
     end
 
-    retval
+    @feature_flag_cache[feature] = retval
   end
 
 end

spec/lib/feature_flags_spec.rb

@@ -76,7 +76,9 @@
         t_root_account.lookup_feature_flag('course_feature').context.should eql t_root_account
         t_course.feature_enabled?('course_feature').should be_true
         t_site_admin.feature_flags.create! feature: 'course_feature', state: 'off'
+        t_root_account.instance_variable_set(:@feature_flag_cache, nil)
         t_root_account.lookup_feature_flag('course_feature').context.should eql t_site_admin
+        t_course.instance_variable_set(:@feature_flag_cache, nil)
         t_course.feature_enabled?('course_feature').should be_false
       end
     end
@@ -99,6 +101,14 @@
         t_sub_account.feature_enabled?('course_feature').should be_false
         t_course.feature_enabled?('course_feature').should be_false
       end
+
+      it "should cache the lookup" do
+        t_sub_account.feature_flags.create! feature: 'course_feature', state: 'on'
+        t_root_account.feature_flags.create! feature: 'course_feature', state: 'off'
+        t_sub_account.lookup_feature_flag('course_feature').context.should eql t_root_account
+        Account.any_instance.expects(:feature_flag).never
+        t_sub_account.lookup_feature_flag('course_feature').context.should eql t_root_account
+      end
     end
 
     context "course flags" do
@@ -114,6 +124,7 @@
       it "should apply settings at the site admin level" do
         t_user.lookup_feature_flag('user_feature').should be_default
         t_site_admin.feature_flags.create! feature: 'user_feature', state: 'off'
+        t_user.instance_variable_set(:@feature_flag_cache, nil)
         t_user.lookup_feature_flag('user_feature').context.should eql t_site_admin
         t_user.feature_enabled?('user_feature').should be_false
       end
@@ -134,6 +145,12 @@
           t_sub_account.lookup_feature_flag('root_opt_in_feature').should be_nil
           t_course.lookup_feature_flag('root_opt_in_feature').should be_nil
         end
+
+        it "should cache the nil of the feature beneath the root account" do
+          t_course.lookup_feature_flag('root_opt_in_feature').should be_nil
+          Account.any_instance.expects(:feature_flag).never
+          t_course.lookup_feature_flag('root_opt_in_feature').should be_nil
+        end
       end
 
       context "with site admin feature flag" do
@@ -269,6 +286,15 @@
       end
     end
 
+    it "should cache a nil result" do
+      enable_cache do
+        t_root_account.feature_flag('course_feature2')
+        Rails.cache.should be_exist(t_root_account.feature_flag_cache_key('course_feature2'))
+        t_root_account.expects(:feature_flags).never
+        t_root_account.feature_flag('course_feature2')
+      end
+    end
+
     it "should invalidate the cache when a feature flag is changed" do
       enable_cache do
         t_root_account.feature_flag('course_feature')

spec/models/quiz_spec.rb

@@ -1232,8 +1232,8 @@
   context "draft_state" do
 
     it "updates the assignment's workflow state" do
+      @course.root_account.enable_feature!(:draft_state)
       @quiz = @course.quizzes.create!(title: 'Test Quiz')
-      @quiz.context.root_account.enable_feature!(:draft_state)
       @quiz.publish!
       @quiz.unpublish!
       @quiz.assignment.should_not be_published