Fix/representer cache key (#4385)

Moves the calculation for whether the WP schema representer has to be rerendered to the representer itself in the form of the cache_key method. This reduces coupling as the cache_key calculation is based on information the representer already has and the controller now no longer needs to know that information.

It additionally uses the same calculation for the E-Tag to reduce duplication.

In order to allow plugins to add to that cache_key, the acts_as_op_engine module is extended.

lib/api/v3/work_packages/schema/work_package_schema_representer.rb

@@ -92,6 +92,16 @@ module API
             super(schema, context)
           end
 
+          def cache_key
+            custom_fields = represented.project.all_work_package_custom_fields
+
+            custom_fields_key = ActiveSupport::Cache.expand_cache_key custom_fields
+
+            ["api/v3/work_packages/schema/#{represented.project.id}-#{represented.type.id}",
+             represented.type.updated_at,
+             Digest::SHA2.hexdigest(custom_fields_key)]
+          end
+
           link :self do
             { href: @self_link } if @self_link
           end

lib/api/v3/work_packages/schema/work_package_schemas_api.rb

@@ -46,17 +46,13 @@ module API
               def raise404
                 raise ::API::Errors::NotFound.new
               end
-
-              def cache_key(project_id, type_id)
-                "api/v3/work_packages/schema/#{project_id}-#{type_id}"
-              end
             end
 
-            # The schema identifier is an artificial identifier that is composed of a work packages
-            # project and its type (separated by a dash)
+            # The schema identifier is an artificial identifier that is composed of a work package's
+            # project and its type (separated by a dash).
             # This allows to have a separate schema URL for each kind of different work packages
             # but with better caching capabilities than simply using the work package id as
-            # identifier for the schema
+            # identifier for the schema.
             namespace ':project-:type' do
               before do
                 begin
@@ -69,20 +65,19 @@ module API
                 authorize(:view_work_packages, context: @project) do
                   raise404
                 end
-
-                # Compare with ETag composed of project and customizations
-                # to avoid evaluating the server request
-                @custom_fields = @project.all_work_package_custom_fields
-                with_etag! "#{@project.id}/#{@custom_fields.count}/#{@custom_fields.to_param}"
               end
 
               get do
-                cache([cache_key(@project.id, @type.id), @type.updated_at, @custom_fields]) do
                 schema = TypedWorkPackageSchema.new(project: @project, type: @type)
                 self_link = api_v3_paths.work_package_schema(@project.id, @type.id)
-                  WorkPackageSchemaRepresenter.create(schema,
+                represented_schema = WorkPackageSchemaRepresenter.create(schema,
                                                                          self_link: self_link,
                                                                          current_user: nil)
+
+                with_etag! represented_schema.cache_key
+
+                cache(represented_schema.cache_key) do
+                  represented_schema
                 end
               end
             end

lib/open_project/plugins/acts_as_op_engine.rb

@@ -225,6 +225,37 @@ module OpenProject::Plugins
           end
         end
       end
+
+      # Register a block to return results when an api representer's cache key is asked for.
+      #
+      # This is important for cache invalidation e.g. when another schema needs
+      # to be returned depending on whether a module is active or not.
+      #
+      # path:          The fully namespaced representer name, excluding 'API' at the
+      #                beginning and 'Representer' at the end.
+      # keys:          The block to be executed when the cache key is queried for. The block's
+      #                results will be appended to the original cache key if a cache key is already
+      #                defined. If no cache key was defined before, the block's result makes up
+      #                the whole cache key.
+      def add_api_representer_cache_key(*path,
+                                        &keys)
+        mod = Module.new
+        mod.send :define_method, :cache_key do
+          if defined?(super)
+            existing = super()
+
+            existing + instance_eval(&keys)
+          else
+            instance_eval(&keys)
+          end
+        end
+
+        config.to_prepare do
+          representer_namespace = path.map { |arg| arg.to_s.camelize }.join('::')
+          representer_class     = "::API::#{representer_namespace}Representer".constantize
+          representer_class.prepend mod
+        end
+      end
     end
   end
 end

spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb

@@ -580,4 +580,46 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       end
     end
   end
+
+  describe '#cache_key' do
+    def joined_cache_key
+      representer.cache_key.join('/')
+    end
+
+    before do
+      allow(work_package.project)
+        .to receive(:all_work_package_custom_fields)
+        .and_return []
+
+      original_cache_key
+    end
+
+    let(:original_cache_key) { joined_cache_key }
+
+    it 'changes when the project changes' do
+      work_package.project = FactoryGirl.build_stubbed(:project)
+
+      expect(joined_cache_key).to_not eql(original_cache_key)
+    end
+
+    it 'changes when the type updates' do
+      work_package.type.updated_at += 1.hour
+
+      expect(joined_cache_key).to_not eql(original_cache_key)
+    end
+
+    it 'changes when the type changes' do
+      work_package.type = FactoryGirl.build_stubbed(:type)
+
+      expect(joined_cache_key).to_not eql(original_cache_key)
+    end
+
+    it 'changes when the custom_fields changes' do
+      allow(work_package.project)
+        .to receive(:all_work_package_custom_fields)
+        .and_return [FactoryGirl.build_stubbed(:custom_field)]
+
+      expect(joined_cache_key).to_not eql(original_cache_key)
+    end
+  end
 end

spec/requests/api/v3/work_packages/work_packages_schemas_resource_spec.rb

@@ -40,6 +40,18 @@ describe API::V3::WorkPackages::Schema::WorkPackageSchemasAPI, type: :request do
   describe 'GET /api/v3/work_packages/schemas/:id' do
     let(:schema_path) { api_v3_paths.work_package_schema project.id, type.id }
 
+    def cache_key
+      # Compare with ETag composed of project and customizations
+      # to avoid evaluating the server request
+      custom_fields = project.all_work_package_custom_fields
+
+      custom_fields_key = ActiveSupport::Cache.expand_cache_key custom_fields
+
+      ["api/v3/work_packages/schema/#{project.id}-#{type.id}",
+       type.updated_at,
+       Digest::SHA2.hexdigest(custom_fields_key)]
+    end
+
     context 'logged in' do
       before do
         allow(User).to receive(:current).and_return(current_user)
@@ -54,6 +66,20 @@ describe API::V3::WorkPackages::Schema::WorkPackageSchemasAPI, type: :request do
         it 'should set a weak ETag' do
           expect(last_response.headers['ETag']).to match(/W\/\"\w+\"/)
         end
+
+        it 'caches the response' do
+          schema_class = API::V3::WorkPackages::Schema::TypedWorkPackageSchema
+          representer_class = API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter
+
+          schema = schema_class.new(project: project,
+                                    type: type)
+          self_link = api_v3_paths.work_package_schema(project.id, type.id)
+          represented_schema = representer_class.create(schema,
+                                                        self_link: self_link,
+                                                        current_user: nil)
+
+          expect(Rails.cache.fetch(represented_schema.cache_key)).to_not be_nil
+        end
       end
 
       context 'id is too long' do
@@ -81,55 +107,6 @@ describe API::V3::WorkPackages::Schema::WorkPackageSchemasAPI, type: :request do
         expect(last_response.status).to eql(404)
       end
     end
-
-    describe 'schema caching' do
-      # Reproduce the schema cache key.
-      # This is somewhat deeper knowledge, but I can't reliably access
-      # the embedded helper
-      def schema_cache_key
-        [
-          "api/v3/work_packages/schema/#{project.id}-#{type.id}/#{type.updated_at}",
-          project.all_work_package_custom_fields
-        ]
-      end
-
-      let(:cache) { ActiveSupport::Cache::MemoryStore.new }
-      before do
-        allow(Rails).to receive(:cache).and_return(cache)
-        allow(User).to receive(:current).and_return(current_user)
-      end
-
-      it 'should only create the representer once' do
-        expect(::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter)
-          .to receive(:create).once
-          .and_call_original
-
-        expect(Rails.cache.read(schema_cache_key)).to be_nil
-
-        # First request causes schema to be cached
-        get schema_path
-        expect(Rails.cache.read(schema_cache_key)).not_to be_nil
-
-        get schema_path
-        expect(last_response.status).to eql(200)
-      end
-
-      it 'refreshes the cache when the type changes' do
-        expect(::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter)
-          .to receive(:create).twice
-          .and_call_original
-
-        get schema_path
-        expect(Rails.cache.read(schema_cache_key)).not_to be_nil
-
-        expect {
-          type.update_attribute(:updated_at, 1.day.from_now)
-        }.to change { schema_cache_key }
-
-        get schema_path
-        expect(Rails.cache.read(schema_cache_key)).not_to be_nil
-      end
-    end
   end
 
   describe 'GET /api/v3/work_packages/schemas/sums' do