diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 22d23373de2..c2bf7e8b687 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/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') diff --git a/app/controllers/services/user_provided_service_instances_controller.rb b/app/controllers/services/user_provided_service_instances_controller.rb index 3ac2ecb257e..4a114b4cb98 100644 --- a/app/controllers/services/user_provided_service_instances_controller.rb +++ b/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') diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 02159e1d975..ddf2420d28a 100644 --- a/app/models/services/service_instance.rb +++ b/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 diff --git a/spec/support/matchers/sequel_validations.rb b/spec/support/matchers/sequel_validations.rb index 7e40682eb99..c4f397a2e07 100644 --- a/spec/support/matchers/sequel_validations.rb +++ b/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) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 93f3a78af3e..f2cdee0e36c 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/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 diff --git a/spec/unit/models/services/managed_service_instance_spec.rb b/spec/unit/models/services/managed_service_instance_spec.rb index 56d9ea23ce9..8a2dbc54a51 100644 --- a/spec/unit/models/services/managed_service_instance_spec.rb +++ b/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] } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 52bb0ff5662..5729832b48d 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/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