Merge branch 'compare-caching-fix' into 'master'

Add latest commit hash to compare cache key

See merge request gitlab-org/gitlab!72388

lib/api/repositories.rb

@@ -42,6 +42,26 @@ module API
 
           not_found! 'Blob' unless @blob
         end
+
+        def fetch_target_project(current_user, user_project, params)
+          return user_project unless params[:from_project_id].present?
+
+          MergeRequestTargetProjectFinder
+            .new(current_user: current_user, source_project: user_project, project_feature: :repository)
+            .execute(include_routes: true).find_by_id(params[:from_project_id])
+        end
+
+        def compare_cache_key(current_user, user_project, target_project, params)
+          [
+            user_project,
+            target_project,
+            current_user,
+            :repository_compare,
+            target_project.repository.commit(params[:from]),
+            user_project.repository.commit(params[:to]),
+            params
+          ]
+        end
       end
 
       desc 'Get a project repository tree' do
@@ -126,20 +146,15 @@ module API
       end
       get ':id/repository/compare' do
         ff_enabled = Feature.enabled?(:api_caching_rate_limit_repository_compare, user_project, default_enabled: :yaml)
+        target_project = fetch_target_project(current_user, user_project, params)
 
-        cache_action_if(ff_enabled, [user_project, :repository_compare, current_user, declared_params], expires_in: 1.minute) do
-          if params[:from_project_id].present?
-            target_project = MergeRequestTargetProjectFinder
-              .new(current_user: current_user, source_project: user_project, project_feature: :repository)
-              .execute(include_routes: true).find_by_id(params[:from_project_id])
+        if target_project.blank?
+          render_api_error!("Target project id:#{params[:from_project_id]} is not a fork of project id:#{params[:id]}", 400)
+        end
 
-            if target_project.blank?
-              render_api_error!("Target project id:#{params[:from_project_id]} is not a fork of project id:#{params[:id]}", 400)
-            end
-          else
-            target_project = user_project
-          end
+        cache_key = compare_cache_key(current_user, user_project, target_project, declared_params)
 
+        cache_action_if(ff_enabled, cache_key, expires_in: 1.minute) do
           compare = CompareService.new(user_project, params[:to]).execute(target_project, params[:from], straight: params[:straight])
 
           if compare

spec/requests/api/repositories_spec.rb

@@ -495,6 +495,43 @@ RSpec.describe API::Repositories do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      it "returns a newly created commit", :use_clean_rails_redis_caching do
+        # Parse the commits ourselves because json_response is cached
+        def commit_messages(response)
+          Gitlab::Json.parse(response.body)["commits"].map do |commit|
+            commit["message"]
+          end
+        end
+
+        # First trigger the rate limit cache
+        get api(route, current_user), params: { from: 'master', to: 'feature' }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(commit_messages(response)).not_to include("Cool new commit")
+
+        # Then create a new commit via the API
+        post api("/projects/#{project.id}/repository/commits", user), params: {
+          branch: "feature",
+          commit_message: "Cool new commit",
+          actions: [
+            {
+              action: "create",
+              file_path: "foo/bar/baz.txt",
+              content: "puts 8"
+            }
+          ]
+        }
+
+        expect(response).to have_gitlab_http_status(:created)
+
+        # Now perform the same query as before, but the cache should have expired
+        # and our new commit should exist
+        get api(route, current_user), params: { from: 'master', to: 'feature' }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(commit_messages(response)).to include("Cool new commit")
+      end
     end
 
     context 'when unauthenticated', 'and project is public' do