Add Validation that Service Instance Name Doesn't Clash with Shared Instances

app/controllers/services/service_instances_controller.rb

@@ -34,14 +34,13 @@ class ServiceInstancesController < RestController::ModelController
     define_routes
 
     def self.translate_validation_exception(e, attributes)
-      space_and_name_errors = e.errors.on([:space_id, :name]).to_a
-      quota_errors = e.errors.on(:quota).to_a
-      service_plan_errors = e.errors.on(:service_plan).to_a
-      service_instance_errors = e.errors.on(:service_instance).to_a
+      quota_errors                 = e.errors.on(:quota).to_a
+      service_plan_errors          = e.errors.on(:service_plan).to_a
+      service_instance_errors      = e.errors.on(:service_instance).to_a
       service_instance_name_errors = e.errors.on(:name).to_a
       service_instance_tags_errors = e.errors.on(:tags).to_a
 
-      if space_and_name_errors.include?(:unique)
+      if service_instance_name_errors.include?(:unique)
         return CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name'])
       elsif quota_errors.include?(:service_instance_space_quota_exceeded)
         return CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceQuotaExceeded')

app/controllers/services/user_provided_service_instances_controller.rb

@@ -29,11 +29,11 @@ def inject_dependencies(dependencies)
     end
 
     def self.translate_validation_exception(e, attributes)
-      space_and_name_errors = e.errors.on([:space_id, :name])
+      name_errors = e.errors.on(:name)
       service_instance_errors = e.errors.on(:service_instance)
       service_instance_name_errors = e.errors.on(:name).to_a
 
-      if space_and_name_errors&.include?(:unique)
+      if name_errors&.include?(:unique)
         CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name'])
       elsif service_instance_errors&.include?(:space_mismatch)
         CloudController::Errors::ApiError.new_from_details('ServiceInstanceRouteBindingSpaceMismatch')

app/models/services/service_instance.rb

@@ -87,14 +87,27 @@ def managed_instance?
       !user_provided_instance?
     end
 
+    def name_clashes
+      proc do |_, instance|
+        next if instance.space_id.nil? || instance.name.nil?
+
+        clashes_with_shared_instance_names =
+          ServiceInstance.select_all(ServiceInstance.table_name).
+          join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: instance.space_guid).
+          where(name: instance.name)
+
+        clashes_with_instance_names =
+          ServiceInstance.select_all(ServiceInstance.table_name).
+          where(space_id: instance.space_id, name: instance.name)
+
+        clashes_with_shared_instance_names.union(clashes_with_instance_names)
+      end
+    end
+
     def validate
       validates_presence :name
       validates_presence :space
-      validates_unique [:space_id, :name], where: (proc do |_, obj, arr|
-                                                     vals = arr.map { |x| obj.send(x) }
-                                                     next if vals.any?(&:nil?)
-                                                     ServiceInstance.where(arr.zip(vals))
-                                                   end)
+      validates_unique :name, where: name_clashes
       validates_max_length 50, :name
       validates_max_length 10_000, :syslog_drain_url, allow_nil: true
     end

spec/support/matchers/sequel_validations.rb

@@ -48,7 +48,7 @@
       duplicate_object[attr] = source_obj[attr]
     end
     unless duplicate_object.valid?
-      errors_key = attributes.length > 1 ? attributes : attributes.first
+      errors_key = options[:error_key] || (attributes.length > 1 ? attributes : attributes.first)
       errors = duplicate_object.errors.on(errors_key)
       expected_error = options[:message] || :unique
       errors && errors.include?(expected_error)

spec/unit/controllers/services/service_instances_controller_spec.rb

@@ -885,6 +885,53 @@ def stub_delete_and_return(status, body)
             expect(last_response.status).to eq(400)
             expect(decoded_response['code']).to eq(60002)
           end
+
+          context 'when a service instance share exists between spaces' do
+            let(:source_space) { Space.make(organization: space.organization) }
+            before do
+              source_space.add_developer(developer)
+
+              service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
+              service_instance.add_shared_space(space)
+              expect(last_response.status).to eq(201)
+            end
+
+            it 'does not allow a managed service instance with same name as a shared service instance' do
+              create_managed_service_instance
+              expect(last_response.status).to eq(400)
+              expect(decoded_response['code']).to eq(60002)
+            end
+
+            it 'does not allow a user provided service instance with same name as a shared service instance' do
+              create_user_provided_service_instance
+              expect(last_response.status).to eq(400)
+              expect(decoded_response['code']).to eq(60002)
+            end
+
+            context 'when an unshared instance exists in the source space' do
+              before do
+                create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+
+              it 'allows an instance of the same name to be created in the shared to space' do
+                create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+            end
+
+            context 'when an unshared instance exists in the shared to space' do
+              before do
+                create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+
+              it 'allows an instance of the same name to be created in the source space' do
+                create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+            end
+          end
         end
 
         context 'when the service_plan does not exist' do
@@ -4003,7 +4050,6 @@ def verify_forbidden(user)
       let(:errors) { instance_double(Sequel::Model::Errors) }
       let(:attributes) { {} }
 
-      let(:space_and_name_errors) { nil }
       let(:quota_errors) { nil }
       let(:service_plan_errors) { nil }
       let(:service_instance_name_errors) { nil }
@@ -4013,7 +4059,6 @@ def verify_forbidden(user)
 
       before do
         allow(e).to receive(:errors).and_return(errors)
-        allow(errors).to receive(:on).with([:space_id, :name]).and_return(space_and_name_errors)
         allow(errors).to receive(:on).with(:quota).and_return(quota_errors)
         allow(errors).to receive(:on).with(:service_plan).and_return(service_plan_errors)
         allow(errors).to receive(:on).with(:name).and_return(service_instance_name_errors)
@@ -4028,7 +4073,6 @@ def verify_forbidden(user)
       end
 
       context "when errors are included but aren't supported validation exceptions" do
-        let(:space_and_name_errors) { [:stuff] }
         let(:quota_errors) { [:stuff] }
         let(:service_plan_errors) { [:stuff] }
         let(:service_instance_name_errors) { [:stuff] }
@@ -4042,7 +4086,7 @@ def verify_forbidden(user)
 
       context 'when there is a service instance name taken error' do
         let(:attributes) { { 'name' => 'test name' } }
-        let(:space_and_name_errors) { [:unique] }
+        let(:service_instance_name_errors) { [:unique] }
 
         it 'returns a ServiceInstanceNameTaken error' do
           expect(VCAP::CloudController::ServiceInstancesController.translate_validation_exception(e, attributes).name).to eq('ServiceInstanceNameTaken')
@@ -4111,10 +4155,12 @@ def create_managed_service_instance(user_opts={})
       arbitrary_params = user_opts.delete(:parameters)
       accepts_incomplete = user_opts.delete(:accepts_incomplete) { |_| 'true' }
       tags = user_opts.delete(:tags)
+      service_instance_space = user_opts.delete(:space) || space
+      service_instance_name = user_opts.delete(:name) || 'foo'
 
       body = {
-        name: 'foo',
-        space_guid: space.guid,
+        name: service_instance_name,
+        space_guid: service_instance_space.guid,
         service_plan_guid: plan.guid,
       }
       body[:parameters] = arbitrary_params if arbitrary_params

spec/unit/models/services/managed_service_instance_spec.rb

@@ -32,7 +32,7 @@ module VCAP::CloudController
       it { is_expected.to validate_presence :name }
       it { is_expected.to validate_presence :service_plan }
       it { is_expected.to validate_presence :space }
-      it { is_expected.to validate_uniqueness [:space_id, :name] }
+      it { is_expected.to validate_uniqueness :space_id, :name, { error_key: :name } }
       it { is_expected.to strip_whitespace :name }
       let(:max_tags) { ['a' * 1024, 'b' * 1024] }
 

spec/unit/models/services/service_instance_spec.rb

@@ -85,7 +85,7 @@ module VCAP::CloudController
             expect {
               service_instance_foo.set(name: 'bar')
               service_instance_foo.save_changes
-            }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
           end
         end
       end
@@ -97,13 +97,13 @@ module VCAP::CloudController
           it 'raises an exception when creating another UserProvidedServiceInstance' do
             expect {
               UserProvidedServiceInstance.create(service_instance_attrs)
-            }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
           end
 
           it 'raises an exception when creating a ManagedServiceInstance' do
             expect {
               ManagedServiceInstance.create(service_instance_attrs)
-            }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
           end
         end
 
@@ -116,13 +116,37 @@ module VCAP::CloudController
           it 'raises an exception when creating another ManagedServiceInstance' do
             expect {
               ManagedServiceInstance.create(service_instance_attrs)
-            }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
           end
 
           it 'raises an exception when creating a UserProvidedServiceInstance' do
             expect {
               UserProvidedServiceInstance.create(service_instance_attrs)
-            }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
+          end
+        end
+
+        describe 'when a ManagedServiceInstance has been shared' do
+          let(:space) { Space.make }
+          let(:originating_space) { Space.make }
+          let(:service_instance) {
+            ManagedServiceInstance.make(name: 'shared-service', space: originating_space)
+          }
+
+          before do
+            service_instance.add_shared_space(space)
+          end
+
+          it 'raises an exception when creating another ManagedServiceInstance' do
+            expect {
+              ManagedServiceInstance.make(name: 'shared-service', space: space)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
+          end
+
+          it 'raises an exception when creating another UserProvidedServiceInstance' do
+            expect {
+              UserProvidedServiceInstance.make(name: 'shared-service', space: space)
+            }.to raise_error(Sequel::ValidationFailed, /name unique/)
           end
         end
       end