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
- introduced 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

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)

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

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
 

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) }
 

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:)

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

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) }