diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 2b124aefd367d9ab064a863f5ae1fcb7d8fe9fd0..279fd4c457e0cd926d4a4e1d3ea893ca9603b56c 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -48,24 +48,24 @@ def diffs_batch allow_tree_conflicts: display_merge_conflicts_in_diff? } - if diff_options_hash[:paths].blank? - # NOTE: Any variables that would affect the resulting json needs to be added to the cache_context to avoid stale cache issues. - cache_context = [ - current_user&.cache_key, - unfoldable_positions.map(&:to_h), - diff_view, - params[:w], - params[:expanded], - params[:page], - params[:per_page], - options[:merge_ref_head_diff], - options[:allow_tree_conflicts] - ] - - if Feature.enabled?(:etag_merge_request_diff_batches, @merge_request.project) - return unless stale?(etag: [cache_context, diffs]) - end + # NOTE: Any variables that would affect the resulting json needs to be added to the cache_context to avoid stale cache issues. + cache_context = [ + current_user&.cache_key, + unfoldable_positions.map(&:to_h), + diff_view, + params[:w], + params[:expanded], + params[:page], + params[:per_page], + options[:merge_ref_head_diff], + options[:allow_tree_conflicts] + ] + + if Feature.enabled?(:etag_merge_request_diff_batches, @merge_request.project) + return unless stale?(etag: [cache_context + diff_options_hash.fetch(:paths, []), diffs]) + end + if diff_options_hash[:paths].blank? render_cached( diffs, with: PaginatedDiffSerializer.new(current_user: current_user), diff --git a/spec/requests/projects/merge_requests/diffs_spec.rb b/spec/requests/projects/merge_requests/diffs_spec.rb index 4e6a7be415e1c24d8b7e27ac1a8cc4b10dead717..937b0f1d7131f7ec3935ecba46ccd7ab3301e72e 100644 --- a/spec/requests/projects/merge_requests/diffs_spec.rb +++ b/spec/requests/projects/merge_requests/diffs_spec.rb @@ -252,14 +252,48 @@ def go(headers: {}, **extra_params) end context 'when the paths is given' do - subject { go(page: 0, per_page: 5, paths: %w[README CHANGELOG]) } + subject { go(headers: headers, page: 0, per_page: 5, paths: %w[README CHANGELOG]) } - it 'does not use cache' do - expect(Rails.cache).not_to receive(:fetch).with(/cache:gitlab:PaginatedDiffSerializer/).and_call_original + before do + go(page: 0, per_page: 5, paths: %w[README CHANGELOG]) + end - subject + context 'when using ETag caching' do + let(:headers) { { 'If-None-Match' => response.etag } } + + context 'when etag_merge_request_diff_batches is true' do + it 'does not serialize diffs' do + expect(PaginatedDiffSerializer).not_to receive(:new) - expect(response).to have_gitlab_http_status(:success) + subject + + expect(response).to have_gitlab_http_status(:not_modified) + end + end + + context 'when etag_merge_request_diff_batches is false' do + before do + stub_feature_flags(etag_merge_request_diff_batches: false) + end + + it 'does not use cache' do + expect(Rails.cache).not_to receive(:fetch).with(/cache:gitlab:PaginatedDiffSerializer/).and_call_original + + subject + + expect(response).to have_gitlab_http_status(:success) + end + end + end + + context 'when not using ETag caching' do + it 'does not use cache' do + expect(Rails.cache).not_to receive(:fetch).with(/cache:gitlab:PaginatedDiffSerializer/).and_call_original + + subject + + expect(response).to have_gitlab_http_status(:success) + end end end end