Only set an ETag for the notes endpoint after all notes have been sent

The ETag caching system we use assumes that all notes are sent in a
single response, and makes that response the cached one. When a note
is subsequently updated, the ETag is invalidated and the note thrown
away.

This is incompatible with pagination, which is currently behind a flag
(appropriately, called `paginated_notes`. I *think* it also means that
the clients end up parsing the cached response every poll-interval,
which is less than ideal.

One approach is to remove the ETag from this endpoint altogether, but
this has some costs in terms of additional server load.

In this commit, I introduce a mechanism to make the ETag conditional,
and skip adding the ETag if there are any notes in the response. This
means that:

* Pagination will work correctly
* The browser will only ever cache the empty response

This means there will be one additional HTTP request before the ETag
is applied, but I think it's overall positive in its own right, and
allowing paginated notes to work is a big plus too.

app/controllers/concerns/notes_actions.rb

@@ -31,6 +31,10 @@ module NotesActions
     # We know there's more data, so tell the frontend to poll again after 1ms
     set_polling_interval_header(interval: 1) if meta[:more]
 
+    # Only present an ETag for the empty response to ensure pagination works
+    # as expected
+    ::Gitlab::EtagCaching::Middleware.skip!(response) if notes.present?
+
     render json: meta.merge(notes: notes)
   end
 
@@ -115,7 +119,7 @@ module NotesActions
   end
 
   def gather_some_notes
-    paginator = Gitlab::UpdatedNotesPaginator.new(
+    paginator = ::Gitlab::UpdatedNotesPaginator.new(
       notes_finder.execute.inc_relations_for_view,
       last_fetched_at: last_fetched_at
     )

changelogs/unreleased/209784-notes-etag-only-for-empty-response.yml

+---
+title: Only set an ETag for the notes endpoint after all notes have been sent
+merge_request: 46810
+author:
+type: performance

lib/gitlab/etag_caching/middleware.rb

@@ -3,6 +3,14 @@
 module Gitlab
   module EtagCaching
     class Middleware
+      SKIP_HEADER_KEY = 'X-Gitlab-Skip-Etag'
+
+      class << self
+        def skip!(response)
+          response.set_header(SKIP_HEADER_KEY, '1')
+        end
+      end
+
       def initialize(app)
         @app = app
       end
@@ -22,9 +30,7 @@ module Gitlab
         else
           track_cache_miss(if_none_match, cached_value_present, route)
 
-          status, headers, body = @app.call(env)
-          headers['ETag'] = etag
-          [status, headers, body]
+          maybe_apply_etag(etag, *@app.call(env))
         end
       end
 
@@ -43,6 +49,13 @@ module Gitlab
         [weak_etag_format(current_value), cached_value_present]
       end
 
+      def maybe_apply_etag(etag, status, headers, body)
+        headers['ETag'] = etag unless
+          Gitlab::Utils.to_boolean(headers.delete(SKIP_HEADER_KEY))
+
+        [status, headers, body]
+      end
+
       def weak_etag_format(value)
         %Q{W/"#{value}"}
       end

spec/controllers/projects/notes_controller_spec.rb

@@ -113,6 +113,8 @@ RSpec.describe Projects::NotesController do
         end
 
         it 'returns the first page of notes' do
+          expect(Gitlab::EtagCaching::Middleware).to receive(:skip!)
+
           get :index, params: request_params
 
           expect(json_response['notes'].count).to eq(page_1.count)
@@ -122,6 +124,8 @@ RSpec.describe Projects::NotesController do
         end
 
         it 'returns the second page of notes' do
+          expect(Gitlab::EtagCaching::Middleware).to receive(:skip!)
+
           request.headers['X-Last-Fetched-At'] = page_1_boundary
 
           get :index, params: request_params
@@ -133,6 +137,8 @@ RSpec.describe Projects::NotesController do
         end
 
         it 'returns the final page of notes' do
+          expect(Gitlab::EtagCaching::Middleware).to receive(:skip!)
+
           request.headers['X-Last-Fetched-At'] = page_2_boundary
 
           get :index, params: request_params
@@ -142,6 +148,19 @@ RSpec.describe Projects::NotesController do
           expect(json_response['last_fetched_at']).to eq(microseconds(Time.zone.now))
           expect(response.headers['Poll-Interval'].to_i).to be > 1
         end
+
+        it 'returns an empty page of notes' do
+          expect(Gitlab::EtagCaching::Middleware).not_to receive(:skip!)
+
+          request.headers['X-Last-Fetched-At'] = microseconds(Time.zone.now)
+
+          get :index, params: request_params
+
+          expect(json_response['notes']).to be_empty
+          expect(json_response['more']).to be_falsy
+          expect(json_response['last_fetched_at']).to eq(microseconds(Time.zone.now))
+          expect(response.headers['Poll-Interval'].to_i).to be > 1
+        end
       end
 
       context 'feature flag disabled' do

spec/lib/gitlab/etag_caching/middleware_spec.rb

@@ -10,6 +10,17 @@ RSpec.describe Gitlab::EtagCaching::Middleware, :clean_gitlab_redis_shared_state
   let(:enabled_path) { '/gitlab-org/gitlab-foss/noteable/issue/1/notes' }
   let(:endpoint) { 'issue_notes' }
 
+  describe '.skip!' do
+    it 'sets the skip header on the response' do
+      rsp = ActionDispatch::Response.new
+      rsp.set_header('Anything', 'Else')
+
+      described_class.skip!(rsp)
+
+      expect(rsp.headers.to_h).to eq(described_class::SKIP_HEADER_KEY => '1', 'Anything' => 'Else')
+    end
+  end
+
   context 'when ETag caching is not enabled for current route' do
     let(:path) { '/gitlab-org/gitlab-foss/tree/master/noteable/issue/1/notes' }
 
@@ -77,6 +88,28 @@ RSpec.describe Gitlab::EtagCaching::Middleware, :clean_gitlab_redis_shared_state
     end
   end
 
+  context 'when the matching route requests that the ETag is skipped' do
+    let(:path) { enabled_path }
+    let(:app) do
+      proc do |_env|
+        response = ActionDispatch::Response.new
+
+        described_class.skip!(response)
+
+        [200, response.headers.to_h, '']
+      end
+    end
+
+    it 'returns the correct headers' do
+      expect(app).to receive(:call).and_call_original
+
+      _, headers, _ = middleware.call(build_request(path, if_none_match))
+
+      expect(headers).not_to have_key('ETag')
+      expect(headers).not_to have_key(described_class::SKIP_HEADER_KEY)
+    end
+  end
+
   shared_examples 'sends a process_action.action_controller notification' do |status_code|
     let(:expected_items) do
       {

spec/requests/projects/noteable_notes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Project noteable notes' do
+  describe '#index' do
+    let_it_be(:merge_request) { create(:merge_request) }
+
+    let(:etag_store) { Gitlab::EtagCaching::Store.new }
+    let(:notes_path) { project_noteable_notes_path(project, target_type: merge_request.class.name.underscore, target_id: merge_request.id) }
+    let(:project) { merge_request.project }
+    let(:user) { project.owner }
+
+    let(:response_etag) { response.headers['ETag'] }
+    let(:stored_etag) { "W/\"#{etag_store.get(notes_path)}\"" }
+
+    before do
+      login_as(user)
+    end
+
+    it 'does not set a Gitlab::EtagCaching ETag if there is a note' do
+      create(:note_on_merge_request, noteable: merge_request, project: merge_request.project)
+
+      get notes_path
+
+      expect(response).to have_gitlab_http_status(:ok)
+
+      # Rack::ETag will set an etag based on the body digest, but that doesn't
+      # interfere with notes pagination
+      expect(response_etag).not_to eq(stored_etag)
+    end
+
+    it 'sets a Gitlab::EtagCaching ETag if there is no note' do
+      get notes_path
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response_etag).to eq(stored_etag)
+    end
+  end
+end