Merge branch 'zj-raise-etag-route-regex-miss' into 'master'

app/models/environment.rb

@@ -209,7 +209,8 @@ class Environment < ActiveRecord::Base
   def etag_cache_key
     Gitlab::Routing.url_helpers.namespace_project_environments_path(
       project.namespace,
-      project)
+      project,
+      format: :json)
   end
 
   private

changelogs/unreleased/zj-raise-etag-route-regex-miss.yml

+---
+title: Fix etag route not being a match for environments
+merge_request:
+author:

lib/gitlab/etag_caching/middleware.rb

@@ -7,7 +7,7 @@ module Gitlab
 
       def call(env)
         request = Rack::Request.new(env)
-        route = Gitlab::EtagCaching::Router.match(request)
+        route = Gitlab::EtagCaching::Router.match(request.path_info)
         return @app.call(env) unless route
 
         track_event(:etag_caching_middleware_used, route)

lib/gitlab/etag_caching/router.rb

@@ -53,8 +53,8 @@ module Gitlab
         )
       ].freeze
 
-      def self.match(request)
-        ROUTES.find { |route| route.regexp.match(request.path_info) }
+      def self.match(path)
+        ROUTES.find { |route| route.regexp.match(path) }
       end
     end
   end

lib/gitlab/etag_caching/store.rb

@@ -25,6 +25,8 @@ module Gitlab
       end
 
       def redis_key(key)
+        raise 'Invalid key' if !Rails.env.production? && !Gitlab::EtagCaching::Router.match(key)
+
         "#{REDIS_NAMESPACE}#{key}"
       end
     end

spec/lib/gitlab/etag_caching/middleware_spec.rb

@@ -15,13 +15,13 @@ describe Gitlab::EtagCaching::Middleware do
     end
 
     it 'does not add ETag header' do
-      _, headers, _ = middleware.call(build_env(path, if_none_match))
+      _, headers, _ = middleware.call(build_request(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))
+      status, _, _ = middleware.call(build_request(path, if_none_match))
 
       expect(status).to eq app_status_code
     end
@@ -39,7 +39,7 @@ describe Gitlab::EtagCaching::Middleware do
       expect_any_instance_of(Gitlab::EtagCaching::Store)
         .to receive(:touch).and_return('123')
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
 
     context 'when If-None-Match header was specified' do
@@ -51,7 +51,7 @@ describe Gitlab::EtagCaching::Middleware do
         expect(Gitlab::Metrics).to receive(:add_event)
           .with(:etag_caching_key_not_found, endpoint: 'issue_notes')
 
-        middleware.call(build_env(path, if_none_match))
+        middleware.call(build_request(path, if_none_match))
       end
     end
   end
@@ -65,7 +65,7 @@ describe Gitlab::EtagCaching::Middleware do
     end
 
     it 'returns this value as header' do
-      _, headers, _ = middleware.call(build_env(path, if_none_match))
+      _, headers, _ = middleware.call(build_request(path, if_none_match))
 
       expect(headers['ETag']).to eq 'W/"123"'
     end
@@ -82,17 +82,17 @@ describe Gitlab::EtagCaching::Middleware do
     it 'does not call app' do
       expect(app).not_to receive(:call)
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
 
     it 'returns status code 304' do
-      status, _, _ = middleware.call(build_env(path, if_none_match))
+      status, _, _ = middleware.call(build_request(path, if_none_match))
 
       expect(status).to eq 304
     end
 
     it 'returns empty body' do
-      _, _, body = middleware.call(build_env(path, if_none_match))
+      _, _, body = middleware.call(build_request(path, if_none_match))
 
       expect(body).to be_empty
     end
@@ -103,7 +103,7 @@ describe Gitlab::EtagCaching::Middleware do
       expect(Gitlab::Metrics).to receive(:add_event)
         .with(:etag_caching_cache_hit, endpoint: 'issue_notes')
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
 
     context 'when polling is disabled' do
@@ -113,7 +113,7 @@ describe Gitlab::EtagCaching::Middleware do
       end
 
       it 'returns status code 429' do
-        status, _, _ = middleware.call(build_env(path, if_none_match))
+        status, _, _ = middleware.call(build_request(path, if_none_match))
 
         expect(status).to eq 429
       end
@@ -131,7 +131,7 @@ describe Gitlab::EtagCaching::Middleware do
     it 'calls app' do
       expect(app).to receive(:call).and_return([app_status_code, {}, ['body']])
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
 
     it 'tracks "etag_caching_resource_changed" event' do
@@ -142,7 +142,7 @@ describe Gitlab::EtagCaching::Middleware do
       expect(Gitlab::Metrics).to receive(:add_event)
         .with(:etag_caching_resource_changed, endpoint: 'issue_notes')
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
   end
 
@@ -160,7 +160,7 @@ describe Gitlab::EtagCaching::Middleware do
       expect(Gitlab::Metrics).to receive(:add_event)
         .with(:etag_caching_header_missing, endpoint: 'issue_notes')
 
-      middleware.call(build_env(path, if_none_match))
+      middleware.call(build_request(path, if_none_match))
     end
   end
 
@@ -192,10 +192,7 @@ describe Gitlab::EtagCaching::Middleware do
       .to receive(:get).and_return(value)
   end
 
-  def build_env(path, if_none_match)
-    {
-      'PATH_INFO' => path,
-      'HTTP_IF_NONE_MATCH' => if_none_match
-    }
+  def build_request(path, if_none_match)
+    { 'PATH_INFO' => path, 'HTTP_IF_NONE_MATCH' => if_none_match }
   end
 end

spec/lib/gitlab/etag_caching/router_spec.rb

@@ -2,115 +2,91 @@ require 'spec_helper'
 
 describe Gitlab::EtagCaching::Router do
   it 'matches issue notes endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/and-subgroup/here-comes-the-project/noteable/issue/1/notes'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'issue_notes'
   end
 
   it 'matches issue title endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/issues/123/realtime_changes'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'issue_title'
   end
 
   it 'matches project pipelines endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/pipelines.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'project_pipelines'
   end
 
   it 'matches commit pipelines endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/commit/aa8260d253a53f73f6c26c734c72fdd600f6e6d4/pipelines.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'commit_pipelines'
   end
 
   it 'matches new merge request pipelines endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/merge_requests/new.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'new_merge_request_pipelines'
   end
 
   it 'matches merge request pipelines endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/merge_requests/234/pipelines.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'merge_request_pipelines'
   end
 
   it 'matches build endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/builds/234.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'project_build'
   end
 
   it 'does not match blob with confusing name' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/blob/master/pipelines.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_blank
   end
 
   it 'matches the environments path' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/environments.json'
     )
 
-    result = described_class.match(request)
     expect(result).to be_present
-
     expect(result.name).to eq 'environments'
   end
 
   it 'matches pipeline#show endpoint' do
-    request = build_request(
+    result = described_class.match(
       '/my-group/my-project/pipelines/2.json'
     )
 
-    result = described_class.match(request)
-
     expect(result).to be_present
     expect(result.name).to eq 'project_pipeline'
   end
-
-  def build_request(path)
-    double(path_info: path)
-  end
 end