Revert to passing the path when matching key to the router

This was edited to the request, but this won't work if the data is not
available at the time of setting the key for the first time.

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

@@ -0,0 +1,4 @@
+---
+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

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