Revise commits caching

Takes into account that this will also cache on the /commits
action too, improves the cache key, and removes the expires_in
call which is ignored by Rails.

app/helpers/commits_helper.rb

@@ -152,16 +152,20 @@ module CommitsHelper
   # partial. It takes some of the same parameters as used in the partial and will hash the
   # current pipeline status.
   #
-  # This is currently configured only for use on the merge requests commits tab, and will
-  # require expansion for use elsewhere.
-  def commit_partial_cache_key(commit, ref:, request:)
+  # This includes a keyed hash for values that can be nil, to prevent invalid cache entries
+  # being served if the order should change in future.
+  def commit_partial_cache_key(commit, ref:, merge_request:, request:)
     [
       commit,
       commit.author,
       ref,
-      hashed_pipeline_status(commit, ref),
-      { xhr: request.xhr? },
-      @path # referred to in #link_to_browse_code
+      {
+        merge_request: merge_request,
+        pipeline_status: hashed_pipeline_status(commit, ref),
+        xhr: request.xhr?,
+        controller: controller.controller_path,
+        path: @path # referred to in #link_to_browse_code
+      }
     ]
   end
 

app/views/projects/commits/_commit.html.haml

@@ -3,6 +3,11 @@
   - `assets/javascripts/diffs/components/commit_item.vue`
 
   EXCEPTION WARNING - see above `.vue` file for de-sync drift
+
+  WARNING: When introducing new content here, please consider what
+           changes may need to be made in the cache keys used to
+           wrap this view, found in
+           CommitsHelper#commit_partial_cache_key
 -#-----------------------------------------------------------------
 - view_details      = local_assigns.fetch(:view_details, false)
 - merge_request     = local_assigns.fetch(:merge_request, nil)
@@ -16,7 +21,6 @@
 - show_project_name = local_assigns.fetch(:show_project_name, false)
 
 %li{ class: ["commit flex-row", ("js-toggle-container" if collapsible)], id: "commit-#{commit.short_id}" }
-
   .avatar-cell.d-none.d-sm-block
     = author_avatar(commit, size: 40, has_tooltip: false)
 

app/views/projects/commits/_commits.html.haml

@@ -14,8 +14,8 @@
 
   %li.commits-row{ data: { day: day } }
     %ul.content-list.commit-list.flex-list
-      - if Feature.enabled?(:cached_mr_commits, project, default_enabled: :yaml)
-        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, request: request) }, expires_in: 1.day
+      - if Feature.enabled?(:cached_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
       - else
         = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
@@ -28,8 +28,8 @@
 
   %li.commits-row
     %ul.content-list.commit-list.flex-list
-      - if Feature.enabled?(:cached_mr_commits, project, default_enabled: :yaml)
-        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, request: request) }, expires_in: 1.day
+      - if Feature.enabled?(:cached_commits, project, default_enabled: :yaml)
+        = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }, cached: -> (commit) { commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
       - else
         = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request }
 

config/feature_flags/development/cached_mr_commits.yml → config/feature_flags/development/cached_commits.yml

 ---
-name: cached_mr_commits
+name: cached_commits
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61617
 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330968
 milestone: '13.12'

spec/helpers/commits_helper_spec.rb

@@ -291,12 +291,13 @@ RSpec.describe CommitsHelper do
   end
 
   describe "#commit_partial_cache_key" do
-    subject { helper.commit_partial_cache_key(commit, ref: ref, request: request) }
+    subject { helper.commit_partial_cache_key(commit, ref: ref, merge_request: merge_request, request: request) }
 
     let(:commit) { create(:commit).present(current_user: user) }
     let(:commit_status) { create(:commit_status) }
     let(:user) { create(:user) }
     let(:ref) { "master" }
+    let(:merge_request) { nil }
     let(:request) { double(xhr?: true) }
     let(:current_path) { "test" }
 
@@ -309,8 +310,17 @@ RSpec.describe CommitsHelper do
     it { is_expected.to include(commit) }
     it { is_expected.to include(commit.author) }
     it { is_expected.to include(ref) }
-    it { is_expected.to include({ xhr: true }) }
-    it { is_expected.to include(Digest::SHA1.hexdigest(commit_status.to_s)) }
-    it { is_expected.to include(current_path) }
+
+    it do
+      is_expected.to include(
+        {
+          merge_request: merge_request,
+          pipeline_status: Digest::SHA1.hexdigest(commit_status.to_s),
+          xhr: true,
+          controller: "commits",
+          path: current_path
+        }
+      )
+    end
   end
 end