v3: Improve performance of v3/routes

Refactor routes fetcher to use a dataset of readable guids instead
of an array. We also short circuit the admin case which speeds up worse
case scenario significantly.

[#174499866]

Co-authored-by: Seth Boyles <sboyles@pivotal.io>
Co-authored-by: Merric de Launey <mdelauney@pivotal.io>

Author: sethboyles

app/controllers/v3/domains_controller.rb

@@ -63,7 +63,7 @@ def check_routes
     message = RoutesListMessage.from_params(check_route_params)
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    dataset = RouteFetcher.fetch(message, Route.dataset.all.map(&:guid))
+    dataset = RouteFetcher.fetch(message, Route.dataset)
     matching_route = false
     if dataset.any?
       matching_route = true

app/controllers/v3/routes_controller.rb

@@ -23,7 +23,7 @@ def index
 
     dataset = RouteFetcher.fetch(
       message,
-      permission_queryer.readable_route_guids,
+      permission_queryer.readable_route_dataset,
       eager_loaded_associations: Presenters::V3::RoutePresenter.associated_resources
     )
 
@@ -198,7 +198,7 @@ def index_by_app
 
     dataset = RouteFetcher.fetch(
       message,
-      permission_queryer.readable_route_guids,
+      permission_queryer.readable_route_dataset,
       eager_loaded_associations: Presenters::V3::RoutePresenter.associated_resources
     )
 

app/fetchers/route_fetcher.rb

@@ -3,8 +3,8 @@
 module VCAP::CloudController
   class RouteFetcher < BaseListFetcher
     class << self
-      def fetch(message, readable_route_guids, eager_loaded_associations: [])
-        dataset = Route.where(guid: readable_route_guids).eager(eager_loaded_associations).qualify
+      def fetch(message, readable_route_dataset, eager_loaded_associations: [])
+        dataset = readable_route_dataset.eager(eager_loaded_associations).qualify
         filter(message, dataset)
       end
 

lib/cloud_controller/permissions.rb

@@ -145,7 +145,15 @@ def can_read_from_isolation_segment?(isolation_segment)
   end
 
   def readable_route_guids
-    VCAP::CloudController::Route.user_visible(@user, can_read_globally?).map(&:guid)
+    readable_route_dataset.map(&:guid)
+  end
+
+  def readable_route_dataset
+    if can_read_globally?
+      VCAP::CloudController::Route.dataset
+    else
+      VCAP::CloudController::Route.user_visible(@user, can_read_globally?)
+    end
   end
 
   def readable_secret_space_guids

lib/cloud_controller/permissions/queryer.rb

@@ -180,6 +180,10 @@ def can_read_from_isolation_segment?(isolation_segment)
     end
   end
 
+  def readable_route_dataset
+    db_permissions.readable_route_dataset
+  end
+
   def readable_route_guids
     science 'readable_route_guids' do |e|
       e.use { db_permissions.readable_route_guids }

spec/unit/fetchers/route_fetcher_spec.rb

@@ -25,7 +25,7 @@ module VCAP::CloudController
         let(:routes_filter) { {} }
 
         it 'eager loads the specified resources for the routes' do
-          results = RouteFetcher.fetch(message, [route1.guid, route2.guid], eager_loaded_associations: [:labels, :domain]).all
+          results = RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route2.guid]), eager_loaded_associations: [:labels, :domain]).all
 
           expect(results.first.associations.key?(:labels)).to be true
           expect(results.first.associations.key?(:domain)).to be true
@@ -38,7 +38,7 @@ module VCAP::CloudController
           let(:routes_filter) { { hosts: 'host2' } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route1.guid, route2.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route2.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route2.guid)
           end
@@ -48,7 +48,7 @@ module VCAP::CloudController
           let(:routes_filter) { { hosts: 'unknown-host' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route1.guid, route2.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route2.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -59,7 +59,7 @@ module VCAP::CloudController
           let(:routes_filter) { { paths: '/path1' } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route1.guid, route2.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route2.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route1.guid)
           end
@@ -69,7 +69,7 @@ module VCAP::CloudController
           let(:routes_filter) { { paths: 'unknown-path' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route1.guid, route2.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route2.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -80,7 +80,7 @@ module VCAP::CloudController
           let(:routes_filter) { { space_guids: space1.guid } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route2.guid)
           end
@@ -90,7 +90,7 @@ module VCAP::CloudController
           let(:routes_filter) { { space_guids: '???' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -101,7 +101,7 @@ module VCAP::CloudController
           let(:routes_filter) { { organization_guids: space1.organization.guid } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route2.guid)
           end
@@ -111,7 +111,7 @@ module VCAP::CloudController
           let(:routes_filter) { { organization_guids: '???' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -122,7 +122,7 @@ module VCAP::CloudController
           let(:routes_filter) { { domain_guids: domain2.guid } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route3.guid)
           end
@@ -132,7 +132,7 @@ module VCAP::CloudController
           let(:routes_filter) { { domain_guids: '???' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -157,7 +157,7 @@ module VCAP::CloudController
           let(:routes_filter) { { ports: '8888' } }
 
           it 'only returns the matching route' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid, route_with_ports.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid, route_with_ports.guid])).all
             expect(results.length).to eq(1)
             expect(results[0].guid).to eq(route_with_ports.guid)
           end
@@ -167,7 +167,7 @@ module VCAP::CloudController
           let(:routes_filter) { { ports: '123' } }
 
           it 'returns no routes' do
-            results = RouteFetcher.fetch(message, [route2.guid, route3.guid]).all
+            results = RouteFetcher.fetch(message, Route.where(guid: [route2.guid, route3.guid])).all
             expect(results.length).to eq(0)
           end
         end
@@ -186,10 +186,10 @@ module VCAP::CloudController
           VCAP::CloudController::RouteLabelModel.make(resource_guid: route3.guid, key_name: 'dog', value: 'chihuahua')
         end
 
-        let(:results) { RouteFetcher.fetch(message, [route1.guid, route3.guid]).all }
+        let(:results) { RouteFetcher.fetch(message, Route.where(guid: [route1.guid, route3.guid])).all }
 
         context 'only the label_selector is present' do
-          let(:results) { RouteFetcher.fetch(message, [route1.guid]).all }
+          let(:results) { RouteFetcher.fetch(message, Route.where(guid: [route1.guid])).all }
 
           let(:message) {
             RoutesListMessage.from_params({ 'label_selector' => 'dog in (chihuahua,scooby-doo)' })
@@ -220,7 +220,7 @@ module VCAP::CloudController
         let(:routes_filter) { { app_guids: [app_model.guid, app_model2.guid] } }
 
         it 'only returns routes that are mapped to the app' do
-          results = RouteFetcher.fetch(message, Route.all.map(&:guid)).all
+          results = RouteFetcher.fetch(message, Route.dataset).all
           expect(results).to contain_exactly(route1, route2)
         end
       end
@@ -232,7 +232,7 @@ module VCAP::CloudController
         let(:routes_filter) { { app_guids: [app_model.guid] } }
 
         it 'only returns routes that are mapped to the app' do
-          results = RouteFetcher.fetch(message, Route.all.map(&:guid)).all
+          results = RouteFetcher.fetch(message, Route.dataset).all
           expect(results).to contain_exactly(route1, route2)
         end
       end

spec/unit/lib/cloud_controller/permissions_spec.rb

@@ -805,6 +805,55 @@ module VCAP::CloudController
       end
     end
 
+    describe '#readable_route_dataset' do
+      it 'returns all the routes for admins' do
+        user = set_current_user_as_admin
+        subject = Permissions.new(user)
+
+        org1 = Organization.make
+        space1 = Space.make(organization: org1)
+        route1 = Route.make(space: space1)
+        route2 = Route.make(space: space1)
+        org2 = Organization.make
+        space2 = Space.make(organization: org2)
+        route3 = Route.make(space: space2)
+
+        dataset = subject.readable_route_dataset
+
+        expect(dataset.first(guid: route1.guid)).to be_present
+        expect(dataset.first(guid: route2.guid)).to be_present
+        expect(dataset.first(guid: route3.guid)).to be_present
+      end
+
+      it 'returns routes where the user has an appropriate org membership' do
+        manager_org = Organization.make
+        manager_space = Space.make(organization: manager_org)
+        manager_route = Route.make(space: manager_space)
+        manager_org.add_manager(user)
+
+        auditor_org = Organization.make
+        auditor_space = Space.make(organization: auditor_org)
+        auditor_route = Route.make(space: auditor_space)
+        auditor_org.add_auditor(user)
+
+        billing_manager_org = Organization.make
+        billing_manager_space = Space.make(organization: billing_manager_org)
+        billing_manager_route = Route.make(space: billing_manager_space)
+        billing_manager_org.add_billing_manager(user)
+
+        member_org = Organization.make
+        member_space = Space.make(organization: member_org)
+        member_route = Route.make(space: member_space)
+        member_org.add_user(user)
+
+        dataset = permissions.readable_route_dataset
+
+        expect(dataset.first(guid: manager_route.guid)).to be_present
+        expect(dataset.first(guid: auditor_route.guid)).to be_present
+        expect(dataset.first(guid: billing_manager_route.guid)).to be_nil
+        expect(dataset.first(guid: member_route.guid)).to be_nil
+      end
+    end
     describe '#readable_route_guids' do
       it 'returns all the route guids for admins' do
         user = set_current_user_as_admin