Use space subqueries for fetchting service credential bindings

As a regular user the select query for service credential bindings is
filtered by a list of space guids (spaces the user has access to).
This can produce very long queries. Instead we can use subqueries to filter the spaces directly on the db.
The same approach is used in the list services endpoint, see cloudfoundry#2580.

app/controllers/v3/service_credential_bindings_controller.rb

@@ -22,7 +22,7 @@ def index
     invalid_param!(message.errors.full_messages) unless message.valid?
 
     results = list_fetcher.fetch(
-      space_guids: space_guids,
+      readable_spaces_query: spaces_query,
       message: message,
       eager_loaded_associations: Presenters::V3::ServiceCredentialBindingPresenter.associated_resources,
     )
@@ -328,14 +328,14 @@ def fetch_credentials_value(name)
   end
 
   def service_credential_binding
-    @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], space_guids: space_guids)
+    @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], readable_spaces_query: spaces_query)
   end
 
-  def space_guids
+  def spaces_query
     if permission_queryer.can_read_globally?
-      :all
+      nil
     else
-      permission_queryer.readable_space_guids
+      permission_queryer.readable_space_guids_query
     end
   end
 

app/fetchers/service_credential_binding_fetcher.rb

@@ -3,8 +3,8 @@
 module VCAP
   module CloudController
     class ServiceCredentialBindingFetcher
-      def fetch(guid, space_guids:)
-        list_fetcher.fetch(space_guids: space_guids).first(guid: guid)
+      def fetch(guid, readable_spaces_query: nil)
+        list_fetcher.fetch(readable_spaces_query: readable_spaces_query).first(guid: guid)
       end
 
       private

app/fetchers/service_credential_binding_list_fetcher.rb

@@ -18,12 +18,12 @@ class << self
           service_offering_guid
         }.freeze
 
-        def fetch(space_guids:, message: nil, eager_loaded_associations: [])
-          dataset = case space_guids
-                    when :all
+        def fetch(readable_spaces_query: nil, message: nil, eager_loaded_associations: [])
+          dataset = case readable_spaces_query
+                    when nil
                       ServiceCredentialBinding::View.dataset
                     else
-                      ServiceCredentialBinding::View.where { Sequel[:space_guid] =~ space_guids }
+                      ServiceCredentialBinding::View.where { Sequel[:space_id] =~ readable_spaces_query.select(:id) }
                     end
 
           dataset = dataset.eager(eager_loaded_associations)

app/models/services/service_credential_binding_view.rb

@@ -10,7 +10,7 @@ module Types
         Sequel.as(:service_keys__id, :id),
         Sequel.as(:service_keys__guid, :guid),
         Sequel.as(Types::SERVICE_KEY, :type),
-        Sequel.as(:spaces__guid, :space_guid),
+        Sequel.as(:spaces__id, :space_id),
         Sequel.as(:service_keys__created_at, :created_at),
         Sequel.as(:service_keys__updated_at, :updated_at),
         Sequel.as(:service_keys__name, :name),
@@ -44,7 +44,7 @@ module Types
         Sequel.as(:service_bindings__id, :id),
         Sequel.as(:service_bindings__guid, :guid),
         Sequel.as(Types::SERVICE_BINDING, :type),
-        Sequel.as(:spaces__guid, :space_guid),
+        Sequel.as(:spaces__id, :space_id),
         Sequel.as(:service_bindings__created_at, :created_at),
         Sequel.as(:service_bindings__updated_at, :updated_at),
         Sequel.as(:service_bindings__name, :name),

spec/unit/fetchers/service_credential_binding_fetcher_spec.rb

@@ -10,19 +10,20 @@ module CloudController
         let!(:existing_credential_binding) { ServiceBinding.make }
 
         it 'should return nothing' do
-          credential_binding = fetcher.fetch('does-not-exist', space_guids: :all)
+          credential_binding = fetcher.fetch('does-not-exist', readable_spaces_query: nil)
           expect(credential_binding).to be_nil
         end
       end
 
       describe 'service keys' do
         let(:space) { Space.make }
+        let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
         let(:service_instance) { ManagedServiceInstance.make(space: space) }
         let!(:service_key) { ServiceKey.make(service_instance: service_instance) }
 
         describe 'when in the space' do
           it 'can be found' do
-            credential_binding = fetcher.fetch(service_key.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceKey)
@@ -35,8 +36,9 @@ module CloudController
 
         describe 'when not in the space' do
           let!(:other_space) { Space.make }
+          let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
           it 'can not be found' do
-            credential_binding = fetcher.fetch(service_key.guid, space_guids: [other_space.guid])
+            credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).to be_nil
           end
@@ -46,11 +48,12 @@ module CloudController
       describe 'app bindings' do
         describe 'managed services' do
           let(:space) { Space.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let(:service_instance) { ManagedServiceInstance.make(space: space) }
           let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') }
 
           it 'can be found' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding)
@@ -64,8 +67,9 @@ module CloudController
 
           describe 'when not in the space' do
             let!(:other_space) { Space.make }
+            let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
             it 'can not be found' do
-              credential_binding = fetcher.fetch(app_binding.guid, space_guids: [other_space.guid])
+              credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
               expect(credential_binding).to be_nil
             end
@@ -74,11 +78,12 @@ module CloudController
 
         describe 'user provided services' do
           let(:space) { Space.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let(:service_instance) { UserProvidedServiceInstance.make(space: space) }
           let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') }
 
           it 'can be found' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding)
@@ -92,8 +97,9 @@ module CloudController
 
           describe 'when not in the space' do
             let!(:other_space) { Space.make }
+            let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
             it 'can not be found' do
-              credential_binding = fetcher.fetch(app_binding.guid, space_guids: [other_space.guid])
+              credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
               expect(credential_binding).to be_nil
             end
@@ -102,14 +108,15 @@ module CloudController
 
         describe 'with last operation' do
           let(:service_instance) { ManagedServiceInstance.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [service_instance.space].map(&:id)) }
           let!(:app_binding) {
             binding = ServiceBinding.make(service_instance: service_instance)
             binding.save_with_new_operation({ state: 'succeeded', type: 'create', description: 'radical avocado' })
             binding
           }
 
           it 'fetches the last operation' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [service_instance.space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding.last_operation).to_not be_nil
 

spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb

@@ -12,7 +12,7 @@ module CloudController
 
       describe 'no bindings' do
         it 'returns an empty result' do
-          expect(fetcher.fetch(space_guids: :all, message: message).all).to eql([])
+          expect(fetcher.fetch(readable_spaces_query: nil, message: message).all).to eql([])
         end
       end
 
@@ -37,15 +37,15 @@ module CloudController
         let!(:app_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: instance, name: Sham.name, **app_binding_details) }
 
         it 'eager loads the specified resources' do
-          dataset = fetcher.fetch(space_guids: :all, message: message, eager_loaded_associations: [:labels_sti_eager_load])
+          dataset = fetcher.fetch(readable_spaces_query: nil, message: message, eager_loaded_associations: [:labels_sti_eager_load])
 
           expect(dataset.all.first.associations.key?(:labels)).to be true
           expect(dataset.all.first.associations.key?(:annotations)).to be false
         end
 
         context 'when getting everything' do
           it 'returns both key and app bindings' do
-            bindings = fetcher.fetch(space_guids: :all, message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
             to_hash = ->(b) {
               {
                 guid: b.guid,
@@ -63,11 +63,12 @@ module CloudController
         context 'when limiting to a space' do
           let(:other_space) { VCAP::CloudController::Space.make }
           let(:other_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: other_space) }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let!(:key_other_binding) { VCAP::CloudController::ServiceKey.make(service_instance: other_instance) }
           let!(:app_other_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: other_instance) }
 
           it 'returns only the bindings within that space' do
-            bindings = fetcher.fetch(space_guids: [space.guid], message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: readable_spaces_query, message: message).all
             binding_guids = bindings.map(&:guid)
 
             expect(binding_guids).to contain_exactly(key_binding.guid, app_binding.guid)
@@ -82,71 +83,71 @@ module CloudController
           context 'service instance name' do
             let(:params) { { 'service_instance_names' => instance.name } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'service instance guid' do
             let(:params) { { 'service_instance_guids' => instance.guid } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'app name' do
             let(:params) { { 'app_names' => "#{app_binding.app.name},'some-other-name'" } }
             it 'can filter by app name' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid)
             end
           end
 
           context 'app guid' do
             let(:params) { { 'app_guids' => "#{app_binding.app.guid}, 'some-other-guid'" } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid)
             end
           end
 
           context 'binding name' do
             let(:params) { { 'names' => "#{key_binding.name},#{app_binding.name}" } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'service plan names' do
             let(:params) { { 'service_plan_names' => instance.service_plan.name } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service plan guids' do
             let(:params) { { 'service_plan_guids' => instance.service_plan.guid } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service offering names' do
             let(:params) { { 'service_offering_names' => app_binding.service.name.to_s } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service offering guids' do
             let(:params) { { 'service_offering_guids' => app_binding.service.guid.to_s } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
@@ -166,7 +167,7 @@ module CloudController
 
             let(:params) { { 'label_selector' => 'fruit=strawberry,tier in (backend,worker)' } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_binding.guid)
             end
           end
@@ -176,7 +177,7 @@ module CloudController
               let(:params) { { 'type' => 'app' } }
 
               it 'returns the right result' do
-                bindings = fetcher.fetch(space_guids: :all, message: message).all
+                bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
                 expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, another_binding.guid)
               end
             end
@@ -185,14 +186,14 @@ module CloudController
               let(:params) { { type: 'key' } }
 
               it 'returns the right result' do
-                bindings = fetcher.fetch(space_guids: :all, message: message).all
+                bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
                 expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_key.guid)
               end
             end
           end
 
           it 'returns all if no filter is passed' do
-            bindings = fetcher.fetch(space_guids: :all, message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
             expect(bindings.count).to eq(4)
           end
 
@@ -201,7 +202,7 @@ module CloudController
               { service_instance_guids: ['fake-guid'], service_instance_names: ['fake-name'] }
             }
             it 'returns empty' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings).to be_empty
             end
           end
@@ -211,7 +212,7 @@ module CloudController
               { names: [key_binding.name, another_binding.name], service_instance_guids: [another_instance.guid] }
             }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(another_binding.guid)
             end
           end
@@ -230,7 +231,7 @@ module CloudController
             }
           )
 
-          credential_binding = fetcher.fetch(space_guids: :all, message: message).first
+          credential_binding = fetcher.fetch(readable_spaces_query: nil, message: message).first
           last_operation = credential_binding.last_operation
 
           expect(last_operation).to be_present