From 62c7c5290bda10879602a4d7a09527ed6c993a0d Mon Sep 17 00:00:00 2001
From: Denise Yu <dyu@pivotal.io>
Date: Tue, 31 Oct 2017 11:24:21 +0000
Subject: [PATCH 1/6] Validate uniqueness of service instance name to include
 shared service instances

[#152314627]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
---
 .../services/service_instances_controller.rb  |  9 +++--
 ...r_provided_service_instances_controller.rb |  4 +--
 app/models/services/service_instance.rb       | 23 ++++++++++---
 spec/support/matchers/sequel_validations.rb   |  2 +-
 .../service_instances_controller_spec.rb      | 34 ++++++++++++++++---
 .../services/managed_service_instance_spec.rb |  2 +-
 .../models/services/service_instance_spec.rb  | 34 ++++++++++++++++---
 7 files changed, 84 insertions(+), 24 deletions(-)

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..83c1d946ca7 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, 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..8dab830faf9 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -885,6 +885,32 @@ def stub_delete_and_return(status, body)
             expect(last_response.status).to eq(400)
             expect(decoded_response['code']).to eq(60002)
           end
+
+          it 'does not allow a managed service instance with same name as a shared service instance' do
+            source_space = Space.make(organization: space.organization)
+            source_space.add_developer(developer)
+            service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
+            expect(last_response.status).to eq(201)
+
+            service_instance.add_shared_space(space)
+
+            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
+            source_space = Space.make(organization: space.organization)
+            source_space.add_developer(developer)
+            service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
+            expect(last_response.status).to eq(201)
+
+            service_instance.add_shared_space(space)
+
+            create_user_provided_service_instance
+            expect(last_response.status).to eq(400)
+            expect(decoded_response['code']).to eq(60002)
+          end
         end
 
         context 'when the service_plan does not exist' do
@@ -4003,7 +4029,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 +4038,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 +4052,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 +4065,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 +4134,11 @@ 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
 
       body = {
         name: 'foo',
-        space_guid: space.guid,
+        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

From 2ee2066a4967910d8a4d15324abc6fd4e08afbc8 Mon Sep 17 00:00:00 2001
From: Sam Gunaratne <sgunaratne@pivotal.io>
Date: Tue, 31 Oct 2017 14:51:40 +0000
Subject: [PATCH 2/6] Fix name collisions between shared spaces

* This fixes an issue that any share between two spaces results in a
shared name pool between spaces. Name collisions should only occur if
the service instance has been shared into the space.

[#152314627]
---
 app/models/services/service_instance.rb       |  2 +-
 .../service_instances_controller_spec.rb      | 62 +++++++++++++------
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb
index 83c1d946ca7..ddf2420d28a 100644
--- a/app/models/services/service_instance.rb
+++ b/app/models/services/service_instance.rb
@@ -93,7 +93,7 @@ def name_clashes
 
         clashes_with_shared_instance_names =
           ServiceInstance.select_all(ServiceInstance.table_name).
-          join(:service_instance_shares, target_space_guid: instance.space_guid).
+          join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: instance.space_guid).
           where(name: instance.name)
 
         clashes_with_instance_names =
diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb
index 8dab830faf9..f2cdee0e36c 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -886,30 +886,51 @@ def stub_delete_and_return(status, body)
             expect(decoded_response['code']).to eq(60002)
           end
 
-          it 'does not allow a managed service instance with same name as a shared service instance' do
-            source_space = Space.make(organization: space.organization)
-            source_space.add_developer(developer)
-            service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
-            expect(last_response.status).to eq(201)
+          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.add_shared_space(space)
+              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
 
-            create_managed_service_instance
-            expect(last_response.status).to eq(400)
-            expect(decoded_response['code']).to eq(60002)
-          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
-            source_space = Space.make(organization: space.organization)
-            source_space.add_developer(developer)
-            service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
-            expect(last_response.status).to eq(201)
+            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
 
-            service_instance.add_shared_space(space)
+            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
 
-            create_user_provided_service_instance
-            expect(last_response.status).to eq(400)
-            expect(decoded_response['code']).to eq(60002)
+              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
 
@@ -4135,9 +4156,10 @@ def create_managed_service_instance(user_opts={})
       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',
+        name: service_instance_name,
         space_guid: service_instance_space.guid,
         service_plan_guid: plan.guid,
       }

From eacdb9b2a070307b2aed7c625cd4307c235e4674 Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Wed, 1 Nov 2017 15:05:53 +0000
Subject: [PATCH 3/6] Delete service instance should fail when service is
 shared

[#152470931]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
---
 .../services/service_instances_controller.rb  | 20 ++++++++---
 spec/request/v2/service_instances_spec.rb     | 27 ++++++++++++++
 .../fakes/fake_service_broker_v2_client.rb    | 10 ++++++
 .../service_instances_controller_spec.rb      | 36 +++++++++++++++++++
 vendor/errors/v2.yml                          |  5 +++
 5 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb
index c2bf7e8b687..8c229b3bb68 100644
--- a/app/controllers/services/service_instances_controller.rb
+++ b/app/controllers/services/service_instances_controller.rb
@@ -159,11 +159,15 @@ def delete(guid)
       end
 
       validate_access(:delete, service_instance)
-      has_assocations = has_routes?(service_instance) ||
-        has_bindings?(service_instance) ||
-        has_keys?(service_instance)
 
-      association_not_empty! if has_assocations && !recursive_delete?
+      unless recursive_delete?
+        has_associations = has_routes?(service_instance) ||
+          has_bindings?(service_instance) ||
+          has_keys?(service_instance)
+
+        association_not_empty! if has_associations
+        service_is_shared!     if has_shares?(service_instance)
+      end
 
       deprovisioner = ServiceInstanceDeprovisioner.new(@services_event_repository, self, logger)
       delete_job = deprovisioner.deprovision_service_instance(service_instance, accepts_incomplete, async)
@@ -458,6 +462,10 @@ def association_not_empty!
       raise CloudController::Errors::ApiError.new_from_details('AssociationNotEmpty', associations, :service_instances)
     end
 
+    def service_is_shared!
+      raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared')
+    end
+
     def space_change_not_allowed!
       raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceChangeNotAllowed')
     end
@@ -490,6 +498,10 @@ def has_keys?(service_instance)
       !service_instance.service_keys.empty?
     end
 
+    def has_shares?(service_instance)
+      !service_instance.shared_spaces.empty?
+    end
+
     def space_change_requested?(requested_space_guid, current_space)
       requested_space_guid && requested_space_guid != current_space.guid
     end
diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb
index 2446c5b6414..a3c5f62b727 100644
--- a/spec/request/v2/service_instances_spec.rb
+++ b/spec/request/v2/service_instances_spec.rb
@@ -244,4 +244,31 @@
       )
     end
   end
+
+  describe 'DELETE /v2/service_instance/:guid' do
+    let(:originating_space) { VCAP::CloudController::Space.make }
+    let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: originating_space) }
+
+    context 'when the service instance has been shared' do
+      before do
+        allow(VCAP::Services::ServiceBrokers::V2::Client).to receive(:new) do |*args, **kwargs, &block|
+          FakeServiceBrokerV2Client.new(*args, **kwargs, &block)
+        end
+
+        set_current_user_as_admin
+        service_instance.add_shared_space(space)
+      end
+
+      it 'fails with an appropriate response' do
+        delete "v2/service_instances/#{service_instance.guid}", nil, admin_headers
+
+        expect(last_response.status).to eq(400)
+
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted'
+        expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared'
+        expect(parsed_response['code']).to eq 10014
+      end
+    end
+  end
 end
diff --git a/spec/support/fakes/fake_service_broker_v2_client.rb b/spec/support/fakes/fake_service_broker_v2_client.rb
index 45ccacbdf04..2e23581aa54 100644
--- a/spec/support/fakes/fake_service_broker_v2_client.rb
+++ b/spec/support/fakes/fake_service_broker_v2_client.rb
@@ -46,6 +46,16 @@ def provision(_instance, arbitrary_parameters: {}, accepts_incomplete: false)
     }
   end
 
+  def deprovision(_instance, arbitrary_parameters: {}, accepts_incomplete: false)
+    {
+      last_operation: {
+        type:        'delete',
+        description: '',
+        state:       'succeeded'
+      }
+    }
+  end
+
   def bind(_binding, _arbitrary_parameters)
     {
       credentials: credentials,
diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb
index f2cdee0e36c..8b585992af1 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -2362,6 +2362,42 @@ def stub_delete_and_return(status, body)
           end
         end
 
+        context 'when the service instance has been shared' do
+          let(:originating_space) { Space.make }
+          let!(:service_instance) { ManagedServiceInstance.make(space: originating_space) }
+
+          before do
+            service_instance.add_shared_space(space)
+          end
+
+          it 'does not delete the associated shares' do
+            delete "/v2/service_instances/#{service_instance.guid}"
+
+            expect(ServiceInstance.find(guid: service_instance.guid)).to be
+            expect(ServiceInstance.find(guid: service_instance.guid).shared_spaces.length).to eq(1)
+          end
+
+          it 'should give the user an error' do
+            delete "/v2/service_instances/#{service_instance.guid}"
+
+            expect(last_response).to have_status_code 400
+            expect(last_response.body).to include 'ServiceIsShared'
+            expect(last_response.body).to include
+            'Service instances must be unshared before they can be deleted'
+          end
+
+          context 'and recursive=true' do
+            it 'deletes the associated shares' do
+              expect {
+                delete "/v2/service_instances/#{service_instance.guid}?recursive=true"
+              }.to change(ServiceInstance.join(:service_instance_shares, service_instance_guid: :service_instances__guid), :count).by(-1)
+
+              expect(last_response.status).to eq(204)
+              expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil
+            end
+          end
+        end
+
         context 'with ?accepts_incomplete=true' do
           before do
             stub_deprovision(service_instance, body: body, status: status, accepts_incomplete: true)
diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml
index 70d20c87ee8..5af32bb057a 100644
--- a/vendor/errors/v2.yml
+++ b/vendor/errors/v2.yml
@@ -88,6 +88,11 @@
   http_code: 429
   message: "Rate Limit Exceeded"
 
+10014:
+  name: ServiceIsShared
+  http_code: 400
+  message: "Service instances must be unshared before they can be deleted"
+
 20001:
   name: UserInvalid
   http_code: 400

From 32b6bead380a300081b1365c5c3972a5a7ee2e87 Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Wed, 1 Nov 2017 17:05:47 +0000
Subject: [PATCH 4/6] Better warning when there are bindings on shared service
 instances

[#151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
---
 .../services/service_instances_controller.rb  |  7 ++---
 spec/request/v2/service_instances_spec.rb     |  4 ++-
 .../service_instances_controller_spec.rb      | 26 +++++++++++++++++--
 vendor/errors/v2.yml                          |  2 +-
 4 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb
index 8c229b3bb68..a7e0206e7b0 100644
--- a/app/controllers/services/service_instances_controller.rb
+++ b/app/controllers/services/service_instances_controller.rb
@@ -161,12 +161,13 @@ def delete(guid)
       validate_access(:delete, service_instance)
 
       unless recursive_delete?
+        service_is_shared!(service_instance.name) if has_shares?(service_instance)
+
         has_associations = has_routes?(service_instance) ||
           has_bindings?(service_instance) ||
           has_keys?(service_instance)
 
         association_not_empty! if has_associations
-        service_is_shared!     if has_shares?(service_instance)
       end
 
       deprovisioner = ServiceInstanceDeprovisioner.new(@services_event_repository, self, logger)
@@ -462,8 +463,8 @@ def association_not_empty!
       raise CloudController::Errors::ApiError.new_from_details('AssociationNotEmpty', associations, :service_instances)
     end
 
-    def service_is_shared!
-      raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared')
+    def service_is_shared!(name)
+      raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared', name)
     end
 
     def space_change_not_allowed!
diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb
index a3c5f62b727..bf3a47b5745 100644
--- a/spec/request/v2/service_instances_spec.rb
+++ b/spec/request/v2/service_instances_spec.rb
@@ -265,7 +265,9 @@
         expect(last_response.status).to eq(400)
 
         parsed_response = MultiJson.load(last_response.body)
-        expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted'
+        expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \
+          "Unsharing #{service_instance.name} will automatically delete any bindings " \
+          'that have been made to applications in other spaces'
         expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared'
         expect(parsed_response['code']).to eq 10014
       end
diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb
index 8b585992af1..e70145d8171 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -2382,8 +2382,30 @@ def stub_delete_and_return(status, body)
 
             expect(last_response).to have_status_code 400
             expect(last_response.body).to include 'ServiceIsShared'
-            expect(last_response.body).to include
-            'Service instances must be unshared before they can be deleted'
+            expect(last_response.body).to include(
+              'Service instances must be unshared before they can be deleted. ' \
+              "Unsharing #{service_instance.name} will automatically delete any bindings " \
+              'that have been made to applications in other spaces')
+          end
+
+          context 'and there are bindings to the shared instance' do
+            before do
+              ServiceBinding.make(
+                app: AppModel.make(space: space),
+                service_instance: service_instance
+              )
+            end
+
+            it 'should give the user an error' do
+              delete "/v2/service_instances/#{service_instance.guid}"
+
+              expect(last_response).to have_status_code 400
+              expect(last_response.body).to include 'ServiceIsShared'
+              expect(last_response.body).to include(
+                'Service instances must be unshared before they can be deleted. ' \
+                "Unsharing #{service_instance.name} will automatically delete any bindings " \
+                'that have been made to applications in other spaces')
+            end
           end
 
           context 'and recursive=true' do
diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml
index 5af32bb057a..30c1ee40c26 100644
--- a/vendor/errors/v2.yml
+++ b/vendor/errors/v2.yml
@@ -91,7 +91,7 @@
 10014:
   name: ServiceIsShared
   http_code: 400
-  message: "Service instances must be unshared before they can be deleted"
+  message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces"
 
 20001:
   name: UserInvalid

From 748f059052fe6dc0c95f924f7f2dee5ebb1b66cd Mon Sep 17 00:00:00 2001
From: Sam Gunaratne <sgunaratne@pivotal.io>
Date: Thu, 2 Nov 2017 11:10:14 +0000
Subject: [PATCH 5/6] Improve delete instance when sharing error code

* Change error code
* Grammar

[#151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
---
 spec/request/v2/service_instances_spec.rb              |  4 ++--
 .../services/service_instances_controller_spec.rb      |  4 ++--
 vendor/errors/v2.yml                                   | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb
index bf3a47b5745..9823829c38c 100644
--- a/spec/request/v2/service_instances_spec.rb
+++ b/spec/request/v2/service_instances_spec.rb
@@ -267,9 +267,9 @@
         parsed_response = MultiJson.load(last_response.body)
         expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \
           "Unsharing #{service_instance.name} will automatically delete any bindings " \
-          'that have been made to applications in other spaces'
+          'that have been made to applications in other spaces.'
         expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared'
-        expect(parsed_response['code']).to eq 10014
+        expect(parsed_response['code']).to eq 390002
       end
     end
   end
diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb
index e70145d8171..cb98fcadc02 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -2385,7 +2385,7 @@ def stub_delete_and_return(status, body)
             expect(last_response.body).to include(
               'Service instances must be unshared before they can be deleted. ' \
               "Unsharing #{service_instance.name} will automatically delete any bindings " \
-              'that have been made to applications in other spaces')
+              'that have been made to applications in other spaces.')
           end
 
           context 'and there are bindings to the shared instance' do
@@ -2404,7 +2404,7 @@ def stub_delete_and_return(status, body)
               expect(last_response.body).to include(
                 'Service instances must be unshared before they can be deleted. ' \
                 "Unsharing #{service_instance.name} will automatically delete any bindings " \
-                'that have been made to applications in other spaces')
+                'that have been made to applications in other spaces.')
             end
           end
 
diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml
index 30c1ee40c26..b07dc388e50 100644
--- a/vendor/errors/v2.yml
+++ b/vendor/errors/v2.yml
@@ -88,11 +88,6 @@
   http_code: 429
   message: "Rate Limit Exceeded"
 
-10014:
-  name: ServiceIsShared
-  http_code: 400
-  message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces"
-
 20001:
   name: UserInvalid
   http_code: 400
@@ -1113,3 +1108,8 @@
   name: ServiceInstanceUnshareFailed
   http_code: 502
   message: "Unshare of service instance failed because one or more bindings could not be deleted.\n\n%s"
+
+390002:
+  name: ServiceIsShared
+  http_code: 400
+  message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces."

From 16747314cef3d8df8e0ba9daa45f2f73ad7dfa7c Mon Sep 17 00:00:00 2001
From: Sam Gunaratne <sgunaratne@pivotal.io>
Date: Thu, 2 Nov 2017 13:52:55 +0000
Subject: [PATCH 6/6] Give a more meaningful name for the shared service
 deletion error code

[Finishes #151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
---
 app/controllers/services/service_instances_controller.rb      | 2 +-
 spec/request/v2/service_instances_spec.rb                     | 2 +-
 .../controllers/services/service_instances_controller_spec.rb | 4 ++--
 vendor/errors/v2.yml                                          | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb
index a7e0206e7b0..5e3d903560c 100644
--- a/app/controllers/services/service_instances_controller.rb
+++ b/app/controllers/services/service_instances_controller.rb
@@ -464,7 +464,7 @@ def association_not_empty!
     end
 
     def service_is_shared!(name)
-      raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared', name)
+      raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceDeletionSharesExists', name)
     end
 
     def space_change_not_allowed!
diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb
index 9823829c38c..cb4ba70c566 100644
--- a/spec/request/v2/service_instances_spec.rb
+++ b/spec/request/v2/service_instances_spec.rb
@@ -268,7 +268,7 @@
         expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \
           "Unsharing #{service_instance.name} will automatically delete any bindings " \
           'that have been made to applications in other spaces.'
-        expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared'
+        expect(parsed_response['error_code']).to eq 'CF-ServiceInstanceDeletionSharesExists'
         expect(parsed_response['code']).to eq 390002
       end
     end
diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb
index cb98fcadc02..770a26e1077 100644
--- a/spec/unit/controllers/services/service_instances_controller_spec.rb
+++ b/spec/unit/controllers/services/service_instances_controller_spec.rb
@@ -2381,7 +2381,7 @@ def stub_delete_and_return(status, body)
             delete "/v2/service_instances/#{service_instance.guid}"
 
             expect(last_response).to have_status_code 400
-            expect(last_response.body).to include 'ServiceIsShared'
+            expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists'
             expect(last_response.body).to include(
               'Service instances must be unshared before they can be deleted. ' \
               "Unsharing #{service_instance.name} will automatically delete any bindings " \
@@ -2400,7 +2400,7 @@ def stub_delete_and_return(status, body)
               delete "/v2/service_instances/#{service_instance.guid}"
 
               expect(last_response).to have_status_code 400
-              expect(last_response.body).to include 'ServiceIsShared'
+              expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists'
               expect(last_response.body).to include(
                 'Service instances must be unshared before they can be deleted. ' \
                 "Unsharing #{service_instance.name} will automatically delete any bindings " \
diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml
index b07dc388e50..3b4f5e9afcb 100644
--- a/vendor/errors/v2.yml
+++ b/vendor/errors/v2.yml
@@ -1110,6 +1110,6 @@
   message: "Unshare of service instance failed because one or more bindings could not be deleted.\n\n%s"
 
 390002:
-  name: ServiceIsShared
+  name: ServiceInstanceDeletionSharesExists
   http_code: 400
   message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces."