Remove distinct for service plan list queries

Service plans (and service offerings) can have multiple sources depending on the query parameters, user permissions and service plan visibilities. To ensure uniqueness and avoid duplicates we used `distinct` in the SQL queries. However existing validators already ensure that there cannot be any duplicates. Thus: - `distinct` can be removed to improve SQL performance - introduce a new validation in `service_plan_visibility` ensures that a public plan cannot be associated with a `service_plan_visibility` - implement a test case to make sure the validation works as expected and throws proper error message
cloudfoundry · Feb 1, 2024 · 9f13183 · 9f13183
1 parent 8a547a6
commit 9f13183
Show file tree

Hide file tree

Showing 13 changed files with 38 additions and 22 deletions.
diff --git a/app/actions/v3/service_plan_visibility_update.rb b/app/actions/v3/service_plan_visibility_update.rb
@@ -15,16 +15,13 @@ def update(service_plan, message, append_organizations: false)
 
         service_plan.db.transaction do
           service_plan.lock!
-
           service_plan.public = public?(type)
-
+          service_plan.save
           if org?(type)
             update_service_plan_visibilities(service_plan, requested_org_guids, append_organizations)
           else
             service_plan.remove_all_service_plan_visibilities
           end
-
-          service_plan.save
         end
 
         service_plan.reload

diff --git a/app/fetchers/base_service_list_fetcher.rb b/app/fetchers/base_service_list_fetcher.rb
@@ -63,8 +63,7 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_
                     end.from_self(alias: klass.table_name)
                   end
 
-        # Select DISTINCT entries and eager load associations.
-        dataset.distinct.eager(eager_loaded_associations)
+        dataset.eager(eager_loaded_associations)
       end
 
       def readable_by_plan_org(dataset, readable_orgs_query)

diff --git a/app/models/services/service_plan_visibility.rb b/app/models/services/service_plan_visibility.rb
@@ -11,6 +11,7 @@ def validate
       validates_presence :organization
       validates_unique %i[organization_id service_plan_id]
       validate_plan_is_not_private
+      validate_plan_is_not_public
     end
 
     def self.visible_private_plan_ids_for_user(user)
@@ -28,5 +29,11 @@ def validate_plan_is_not_private
 
       errors.add(:service_plan, 'is from a private broker')
     end
+
+    def validate_plan_is_not_public
+      return unless service_plan&.public?
+
+      errors.add(:service_plan, 'is publicly available')
+    end
   end
 end
diff --git a/spec/api/documentation/events_api_spec.rb b/spec/api/documentation/events_api_spec.rb
@@ -134,7 +134,7 @@
 
     let(:test_broker) { VCAP::CloudController::ServiceBroker.make }
     let(:test_service) { VCAP::CloudController::Service.make(service_broker: test_broker) }
-    let(:test_plan) { VCAP::CloudController::ServicePlan.make(service: test_service) }
+    let(:test_plan) { VCAP::CloudController::ServicePlan.make(service: test_service, public: false) }
     let(:test_plan_visibility) do
       VCAP::CloudController::ServicePlanVisibility.make(organization_guid: test_organization.guid, service_plan_guid: test_plan.guid)
     end

diff --git a/spec/api/documentation/service_plan_visibilities_api_spec.rb b/spec/api/documentation/service_plan_visibilities_api_spec.rb
@@ -17,7 +17,7 @@
 
     example 'Creating a Service Plan Visibility' do
       org_guid = VCAP::CloudController::Organization.make.guid
-      service_plan_guid = VCAP::CloudController::ServicePlan.make.guid
+      service_plan_guid = VCAP::CloudController::ServicePlan.make(public: false).guid
       request_json = MultiJson.dump({ service_plan_guid: service_plan_guid, organization_guid: org_guid }, pretty: true)
 
       client.post '/v2/service_plan_visibilities', request_json, headers
@@ -32,7 +32,7 @@
     example 'Updating a Service Plan Visibility' do
       service_plan_visibility_guid = VCAP::CloudController::ServicePlanVisibility.make.guid
       org_guid = VCAP::CloudController::Organization.make.guid
-      service_plan_guid = VCAP::CloudController::ServicePlan.make.guid
+      service_plan_guid = VCAP::CloudController::ServicePlan.make(public: false).guid
       request_json = MultiJson.dump({ service_plan_guid: service_plan_guid, organization_guid: org_guid }, pretty: true)
 
       client.put "/v2/service_plan_visibilities/#{service_plan_visibility_guid}", request_json, headers

diff --git a/spec/support/fakes/blueprints.rb b/spec/support/fakes/blueprints.rb
@@ -566,7 +566,7 @@ module VCAP::CloudController
   end
 
   ServicePlanVisibility.blueprint do
-    service_plan { ServicePlan.make }
+    service_plan { ServicePlan.make(public: false) }
     organization { Organization.make }
   end
 

diff --git a/spec/unit/access/service_plan_visibility_access_spec.rb b/spec/unit/access/service_plan_visibility_access_spec.rb
@@ -7,7 +7,7 @@ module VCAP::CloudController
     let(:user) { VCAP::CloudController::User.make }
     let(:service) { VCAP::CloudController::Service.make }
     let(:org) { VCAP::CloudController::Organization.make }
-    let(:service_plan) { VCAP::CloudController::ServicePlan.make(service:) }
+    let(:service_plan) { VCAP::CloudController::ServicePlan.make(service: service, public: false) }
 
     let(:object) { VCAP::CloudController::ServicePlanVisibility.make(organization: org, service_plan: service_plan) }
 

diff --git a/spec/unit/actions/organization_delete_spec.rb b/spec/unit/actions/organization_delete_spec.rb
@@ -25,7 +25,7 @@ module VCAP::CloudController
       let!(:private_domain_2) { PrivateDomain.make(owning_organization: org_2) }
       let!(:service_broker) { ServiceBroker.make }
       let!(:service) { Service.make(service_broker:) }
-      let!(:service_plan) { ServicePlan.make(service:) }
+      let!(:service_plan) { ServicePlan.make(service: service, public: false) }
       let!(:service_plan_visibility) do
         ServicePlanVisibility.make(organization_guid: org_1.guid, service_plan_guid: service_plan.guid)
       end

diff --git a/spec/unit/controllers/services/service_plan_visibilities_controller_spec.rb b/spec/unit/controllers/services/service_plan_visibilities_controller_spec.rb
@@ -60,7 +60,7 @@ def self.user_sees_empty_enumerate(user_role, member_a_ivar, member_b_ivar)
 
     describe 'POST /v2/service_plan_visibilities' do
       let!(:organization) { Organization.make }
-      let!(:service_plan) { ServicePlan.make }
+      let!(:service_plan) { ServicePlan.make(public: false) }
 
       it 'creates the service plan visibility' do
         params = { organization_guid: organization.guid, service_plan_guid: service_plan.guid }
@@ -97,7 +97,7 @@ def self.user_sees_empty_enumerate(user_role, member_a_ivar, member_b_ivar)
     describe 'PUT /v2/service_plan_visibilities/:guid' do
       let!(:organization) { Organization.make }
       let!(:new_organization) { Organization.make }
-      let!(:service_plan) { ServicePlan.make }
+      let!(:service_plan) { ServicePlan.make(public: false) }
       let!(:visibility) { ServicePlanVisibility.make(organization_guid: organization.guid, service_plan_guid: service_plan.guid) }
 
       it 'updates the service plan visibility' do
@@ -128,7 +128,7 @@ def self.user_sees_empty_enumerate(user_role, member_a_ivar, member_b_ivar)
 
     describe 'DELETE /v2/service_plan_visibilities/:guid' do
       let!(:organization) { Organization.make }
-      let!(:service_plan) { ServicePlan.make }
+      let!(:service_plan) { ServicePlan.make(public: false) }
       let!(:visibility) { ServicePlanVisibility.make(organization_guid: organization.guid, service_plan_guid: service_plan.guid) }
 
       it 'deletes the service plan visibility' do

diff --git a/spec/unit/fetchers/service_plan_visibility_fetcher_spec.rb b/spec/unit/fetchers/service_plan_visibility_fetcher_spec.rb
@@ -18,14 +18,14 @@ module VCAP::CloudController
     let!(:org2) { Organization.make }
 
     let!(:plan_1) do
-      plan = ServicePlan.make
+      plan = ServicePlan.make(public: false)
       ServicePlanVisibility.make(service_plan: plan, organization: org1)
       ServicePlanVisibility.make(service_plan: plan, organization: org2)
       plan
     end
 
     let!(:plan_2) do
-      plan = ServicePlan.make
+      plan = ServicePlan.make(public: false)
       ServicePlanVisibility.make(service_plan: plan, organization: org2)
       plan
     end

diff --git a/spec/unit/models/services/service_plan_spec.rb b/spec/unit/models/services/service_plan_spec.rb
@@ -7,7 +7,7 @@ module VCAP::CloudController
     describe 'Associations' do
       it { is_expected.to have_associated :service }
       it { is_expected.to have_associated :service_instances, class: ManagedServiceInstance }
-      it { is_expected.to have_associated :service_plan_visibilities }
+      it { is_expected.to have_associated :service_plan_visibilities, { test_instance: ServicePlan.make(public: false) } }
       it { is_expected.to have_associated :labels, class: ServicePlanLabelModel }
       it { is_expected.to have_associated :annotations, class: ServicePlanAnnotationModel }
     end
@@ -153,7 +153,7 @@ module VCAP::CloudController
     end
 
     describe '#destroy' do
-      let(:service_plan) { ServicePlan.make }
+      let(:service_plan) { ServicePlan.make(public: false) }
 
       it 'destroys associated dependencies' do
         service_plan_visibility = ServicePlanVisibility.make(service_plan:)

diff --git a/spec/unit/models/services/service_plan_visibility_spec.rb b/spec/unit/models/services/service_plan_visibility_spec.rb
@@ -20,13 +20,26 @@ module VCAP::CloudController
           space = Space.make(organization:)
           private_broker = ServiceBroker.make(space:)
           service = Service.make service_broker: private_broker, active: true
-          plan = ServicePlan.make(service:)
+          plan = ServicePlan.make(service: service, public: false)
 
           expect do
             ServicePlanVisibility.create service_plan: plan, organization: organization
           end.to raise_error Sequel::ValidationFailed, 'service_plan is from a private broker'
         end
       end
+
+      context 'when the service plan visibility is for a public plan' do
+        it 'returns a validation error' do
+          organization = Organization.make
+          private_broker = ServiceBroker.make
+          service = Service.make service_broker: private_broker, active: true
+          plan = ServicePlan.make(service: service, public: true)
+
+          expect do
+            ServicePlanVisibility.create service_plan: plan, organization: organization
+          end.to raise_error Sequel::ValidationFailed, 'service_plan is publicly available'
+        end
+      end
     end
 
     describe 'Serialization' do

diff --git a/spec/unit/models/services/service_spec.rb b/spec/unit/models/services/service_spec.rb
@@ -211,12 +211,12 @@ def records(user)
 
     describe '#purge' do
       let!(:event_repository) { double(Repositories::ServiceUsageEventRepository) }
-      let!(:service_plan) { ServicePlan.make(service:) }
+      let!(:service_plan) { ServicePlan.make(service: service, public: false) }
       let!(:service_plan_visibility) { ServicePlanVisibility.make(service_plan:) }
       let!(:service_instance) { ManagedServiceInstance.make(service_plan:) }
       let!(:service_binding) { ServiceBinding.make(service_instance:) }
 
-      let!(:service_plan_2) { ServicePlan.make(service:) }
+      let!(:service_plan_2) { ServicePlan.make(service: service, public: false) }
       let!(:service_plan_visibility_2) { ServicePlanVisibility.make(service_plan: service_plan_2) }
       let!(:service_instance_2) { ManagedServiceInstance.make(service_plan: service_plan_2) }
       let!(:service_binding_2) { ServiceBinding.make(service_instance: service_instance_2) }