Merge branch 'etag-notes-polling' into 'master'

Use ETag to improve performance of issue notes polling

Closes #27582

See merge request !9036

5 files changed, 217 insertions, 5 deletions

diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index dc597202050..d80780b1d90 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -200,4 +200,31 @@ describe Projects::NotesController do
       end
     end
   end
+
+  describe 'GET index' do
+    let(:last_fetched_at) { '1487756246' }
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        target_type: 'issue',
+        target_id: issue.id
+      }
+    end
+
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    it 'passes last_fetched_at from headers to NotesFinder' do
+      request.headers['X-Last-Fetched-At'] = last_fetched_at
+
+      expect(NotesFinder).to receive(:new)
+        .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
+        .and_call_original
+
+      get :index, request_params
+    end
+  end
 end
diff --git a/spec/initializers/metrics_spec.rb b/spec/initializers/8_metrics_spec.rb
index bb595162370..570754621f3 100644
--- a/spec/initializers/metrics_spec.rb
+++ b/spec/initializers/8_metrics_spec.rb
@@ -1,5 +1,5 @@
 require 'spec_helper'
-require_relative '../../config/initializers/metrics'
+require_relative '../../config/initializers/8_metrics'
 
 describe 'instrument_classes', lib: true do
   let(:config) { double(:config) }
diff --git a/spec/lib/gitlab/etag_caching/middleware_spec.rb b/spec/lib/gitlab/etag_caching/middleware_spec.rb
new file mode 100644
index 00000000000..8b5bfc4dbb0
--- /dev/null
+++ b/spec/lib/gitlab/etag_caching/middleware_spec.rb
@@ -0,0 +1,163 @@
+require 'spec_helper'
+
+describe Gitlab::EtagCaching::Middleware do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:app_status_code) { 200 }
+  let(:if_none_match) { nil }
+  let(:enabled_path) { '/gitlab-org/gitlab-ce/noteable/issue/1/notes' }
+
+  context 'when ETag caching is not enabled for current route' do
+    let(:path) { '/gitlab-org/gitlab-ce/tree/master/noteable/issue/1/notes' }
+
+    before do
+      mock_app_response
+    end
+
+    it 'does not add ETag header' do
+      _, headers, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(headers['ETag']).to be_nil
+    end
+
+    it 'passes status code from app' do
+      status, _, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(status).to eq app_status_code
+    end
+  end
+
+  context 'when there is no ETag in store for given resource' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_app_response
+      mock_value_in_store(nil)
+    end
+
+    it 'generates ETag' do
+      expect_any_instance_of(Gitlab::EtagCaching::Store)
+        .to receive(:touch).and_return('123')
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    context 'when If-None-Match header was specified' do
+      let(:if_none_match) { 'W/"abc"' }
+
+      it 'tracks "etag_caching_key_not_found" event' do
+        expect(Gitlab::Metrics).to receive(:add_event)
+          .with(:etag_caching_middleware_used)
+        expect(Gitlab::Metrics).to receive(:add_event)
+          .with(:etag_caching_key_not_found)
+
+        middleware.call(build_env(path, if_none_match))
+      end
+    end
+  end
+
+  context 'when there is ETag in store for given resource' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_app_response
+      mock_value_in_store('123')
+    end
+
+    it 'returns this value as header' do
+      _, headers, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(headers['ETag']).to eq 'W/"123"'
+    end
+  end
+
+  context 'when If-None-Match header matches ETag in store' do
+    let(:path) { enabled_path }
+    let(:if_none_match) { 'W/"123"' }
+
+    before do
+      mock_value_in_store('123')
+    end
+
+    it 'does not call app' do
+      expect(app).not_to receive(:call)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    it 'returns status code 304' do
+      status, _, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(status).to eq 304
+    end
+
+    it 'tracks "etag_caching_cache_hit" event' do
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_cache_hit)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  context 'when If-None-Match header does not match ETag in store' do
+    let(:path) { enabled_path }
+    let(:if_none_match) { 'W/"abc"' }
+
+    before do
+      mock_value_in_store('123')
+    end
+
+    it 'calls app' do
+      expect(app).to receive(:call).and_return([app_status_code, {}, ['body']])
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    it 'tracks "etag_caching_resource_changed" event' do
+      mock_app_response
+
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_resource_changed)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  context 'when If-None-Match header is not specified' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_value_in_store('123')
+      mock_app_response
+    end
+
+    it 'tracks "etag_caching_header_missing" event' do
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_header_missing)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  def mock_app_response
+    allow(app).to receive(:call).and_return([app_status_code, {}, ['body']])
+  end
+
+  def mock_value_in_store(value)
+    allow_any_instance_of(Gitlab::EtagCaching::Store)
+      .to receive(:get).and_return(value)
+  end
+
+  def build_env(path, if_none_match)
+    {
+      'PATH_INFO' => path,
+      'HTTP_IF_NONE_MATCH' => if_none_match
+    }
+  end
+end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 1cde9e04951..33536487c41 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -387,4 +387,16 @@ describe Note, models: true do
       end
     end
   end
+
+  describe 'expiring ETag cache' do
+    let(:note) { build(:note_on_issue) }
+
+    it "expires cache for note's issue when note is saved" do
+      expect_any_instance_of(Gitlab::EtagCaching::Store)
+        .to receive(:touch)
+        .with("/#{note.project.namespace.to_param}/#{note.project.to_param}/noteable/issue/#{note.noteable.id}/notes")
+
+      note.save!
+    end
+  end
 end
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index a5bc62ef6c2..d31f1bdfb7c 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -431,12 +431,22 @@ describe 'project routing' do
     end
   end
 
-  #         project_notes GET    /:project_id/notes(.:format)         notes#index
-  #                       POST   /:project_id/notes(.:format)         notes#create
-  #          project_note DELETE /:project_id/notes/:id(.:format)     notes#destroy
+  # project_noteable_notes GET    /:project_id/noteable/:target_type/:target_id/notes notes#index
+  #                        POST   /:project_id/notes(.:format)                        notes#create
+  #           project_note DELETE /:project_id/notes/:id(.:format)                    notes#destroy
   describe Projects::NotesController, 'routing' do
+    it 'to #index' do
+      expect(get('/gitlab/gitlabhq/noteable/issue/1/notes')).to route_to(
+        'projects/notes#index',
+        namespace_id: 'gitlab',
+        project_id: 'gitlabhq',
+        target_type: 'issue',
+        target_id: '1'
+      )
+    end
+
     it_behaves_like 'RESTful project resources' do
-      let(:actions)    { [:index, :create, :destroy] }
+      let(:actions)    { [:create, :destroy] }
       let(:controller) { 'notes' }
     end
   end