From cd65bf780b3824ecdac8af8520acb0528f060cf4 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 2 Jun 2025 12:42:22 +0200 Subject: [PATCH 1/8] Inform operators which service plan, offering and broker is involved in service instance creation/update/deletion --- .../v3/service_instances_controller.rb | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 1ea023e12e9..caaab000870 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -270,6 +270,26 @@ def create_managed(message, space:) audit_hash: message.audit_hash ) + result = VCAP::CloudController::ServicePlan. + join(:services, id: :service_id). + join(:service_brokers, id: Sequel[:services][:service_broker_id]). + where(Sequel[:service_plans][:id] => service_plan.id). + select( + Sequel[:services][:label].as(:service_name), + Sequel[:service_brokers][:name].as(:broker_name) + ). + first + + service_name = result[:service_name] + broker_name = result[:broker_name] + + logger.info( + "Creating managed service instance with name '#{instance.name}' " \ + "using service plan '#{service_plan.name}' " \ + "from service offering '#{service_name}' " \ + "provided by broker '#{broker_name}'." + ) + pollable_job = Jobs::Enqueuer.new(queue: Jobs::Queues.generic).enqueue_pollable(provision_job) head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{pollable_job.guid}") @@ -297,6 +317,35 @@ def update_managed(service_instance) action = V3::ServiceInstanceUpdateManaged.new(service_instance, message, user_audit_info, message.audit_hash) action.preflight! if action.update_broker_needed? + + plan_scope = if message.service_plan_guid + { Sequel[:service_plans][:guid] => message.service_plan_guid } + else + { Sequel[:service_plans][:id] => service_instance.service_plan_id } + end + + result = VCAP::CloudController::ServicePlan. + join(:services, id: :service_id). + join(:service_brokers, id: Sequel[:services][:service_broker_id]). + where(plan_scope). + select( + Sequel[:service_plans][:name].as(:plan_name), + Sequel[:services][:label].as(:service_name), + Sequel[:service_brokers][:name].as(:broker_name) + ). + first + + plan_name = result[:plan_name] + service_name = result[:service_name] + broker_name = result[:broker_name] + + logger.info( + "Updating managed service instance with name '#{service_instance.name}' " \ + "using service plan '#{plan_name}' " \ + "from service offering '#{service_name}' " \ + "provided by broker '#{broker_name}'." + ) + update_job = action.enqueue_update head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{update_job.guid}") else @@ -352,6 +401,29 @@ def fetch_writable_service_instance(guid) def enqueue_delete_job(service_instance) delete_job = V3::DeleteServiceInstanceJob.new(service_instance.guid, user_audit_info) + + result = VCAP::CloudController::ServicePlan. + join(:services, id: :service_id). + join(:service_brokers, id: Sequel[:services][:service_broker_id]). + where(Sequel[:service_plans][:id] => service_instance.service_plan_id). + select( + Sequel[:service_plans][:name].as(:plan_name), + Sequel[:services][:label].as(:service_name), + Sequel[:service_brokers][:name].as(:broker_name) + ). + first + + plan_name = result[:plan_name] + service_name = result[:service_name] + broker_name = result[:broker_name] + + logger.info( + "Deleting managed service instance with name '#{service_instance.name}' " \ + "using service plan '#{plan_name}' " \ + "from service offering '#{service_name}' " \ + "provided by broker '#{broker_name}'." + ) + pollable_job = Jobs::Enqueuer.new(queue: Jobs::Queues.generic).enqueue_pollable(delete_job) pollable_job.guid end From e7b62731c6d0f95c46393148e42b97462a4a5756 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Wed, 4 Jun 2025 18:08:11 +0200 Subject: [PATCH 2/8] Refactor service plan lookup to eager load service and broker associations --- .../v3/service_instances_controller.rb | 79 ++++++++----------- 1 file changed, 32 insertions(+), 47 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index caaab000870..0beea167786 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -253,7 +253,11 @@ def create_user_provided(message) end def create_managed(message, space:) - service_plan = ServicePlan.first(guid: message.service_plan_guid) + service_plan_relations = ServicePlan.eager_graph(service: :service_broker). + where(Sequel[:service_plans][:guid] => message.service_plan_guid). + all + service_plan = service_plan_relations[0] + service_plan_does_not_exist! unless service_plan service_plan_not_visible_to_user!(service_plan) unless visible_to_current_user?(plan: service_plan) unavailable_service_plan!(service_plan) unless service_plan_active?(service_plan) @@ -270,18 +274,8 @@ def create_managed(message, space:) audit_hash: message.audit_hash ) - result = VCAP::CloudController::ServicePlan. - join(:services, id: :service_id). - join(:service_brokers, id: Sequel[:services][:service_broker_id]). - where(Sequel[:service_plans][:id] => service_plan.id). - select( - Sequel[:services][:label].as(:service_name), - Sequel[:service_brokers][:name].as(:broker_name) - ). - first - - service_name = result[:service_name] - broker_name = result[:broker_name] + service_name = service_plan.service.name + broker_name = service_plan.service.service_broker.name logger.info( "Creating managed service instance with name '#{instance.name}' " \ @@ -318,26 +312,22 @@ def update_managed(service_instance) action.preflight! if action.update_broker_needed? - plan_scope = if message.service_plan_guid - { Sequel[:service_plans][:guid] => message.service_plan_guid } - else - { Sequel[:service_plans][:id] => service_instance.service_plan_id } - end - - result = VCAP::CloudController::ServicePlan. - join(:services, id: :service_id). - join(:service_brokers, id: Sequel[:services][:service_broker_id]). - where(plan_scope). - select( - Sequel[:service_plans][:name].as(:plan_name), - Sequel[:services][:label].as(:service_name), - Sequel[:service_brokers][:name].as(:broker_name) - ). - first - - plan_name = result[:plan_name] - service_name = result[:service_name] - broker_name = result[:broker_name] + service_plan_relations = if message.service_plan_guid + { Sequel[:service_plans][:guid] => message.service_plan_guid } + ServicePlan.eager_graph(service: :service_broker). + where(Sequel[:service_plans][:guid] => message.service_plan_guid). + all + else + { Sequel[:service_plans][:id] => service_instance.service_plan_id } + ServicePlan.eager_graph(service: :service_broker). + where(Sequel[:service_plans][:id] => service_instance.service_plan_id). + all + end + + service_plan = service_plan_relations[0] + plan_name = service_plan.name + service_name = service_plan.service.name + broker_name = service_plan.service.service_broker.name logger.info( "Updating managed service instance with name '#{service_instance.name}' " \ @@ -402,20 +392,15 @@ def fetch_writable_service_instance(guid) def enqueue_delete_job(service_instance) delete_job = V3::DeleteServiceInstanceJob.new(service_instance.guid, user_audit_info) - result = VCAP::CloudController::ServicePlan. - join(:services, id: :service_id). - join(:service_brokers, id: Sequel[:services][:service_broker_id]). - where(Sequel[:service_plans][:id] => service_instance.service_plan_id). - select( - Sequel[:service_plans][:name].as(:plan_name), - Sequel[:services][:label].as(:service_name), - Sequel[:service_brokers][:name].as(:broker_name) - ). - first - - plan_name = result[:plan_name] - service_name = result[:service_name] - broker_name = result[:broker_name] + service_plan_relations = ServicePlan.eager_graph(service: :service_broker). + where(Sequel[:service_plans][:id] => service_instance.service_plan_id). + all + + service_plan = service_plan_relations[0] + + plan_name = service_plan.name + service_name = service_plan.service.name + broker_name = service_plan.service.service_broker.name logger.info( "Deleting managed service instance with name '#{service_instance.name}' " \ From 26a9ab0f0c9f7edb78facb63b2d7bafceaab6e9f Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Wed, 4 Jun 2025 19:20:03 +0200 Subject: [PATCH 3/8] simplify update managed with adding only 1 simple sql query --- .../v3/service_instances_controller.rb | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 0beea167786..02f8c1f0c3b 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -312,22 +312,9 @@ def update_managed(service_instance) action.preflight! if action.update_broker_needed? - service_plan_relations = if message.service_plan_guid - { Sequel[:service_plans][:guid] => message.service_plan_guid } - ServicePlan.eager_graph(service: :service_broker). - where(Sequel[:service_plans][:guid] => message.service_plan_guid). - all - else - { Sequel[:service_plans][:id] => service_instance.service_plan_id } - ServicePlan.eager_graph(service: :service_broker). - where(Sequel[:service_plans][:id] => service_instance.service_plan_id). - all - end - - service_plan = service_plan_relations[0] - plan_name = service_plan.name - service_name = service_plan.service.name - broker_name = service_plan.service.service_broker.name + plan_name = service_instance.service_plan.name + service_name = service_instance.service_plan.service.label + broker_name = service_instance.service_plan.service.service_broker.name logger.info( "Updating managed service instance with name '#{service_instance.name}' " \ From 1ebd8ae71598814b58295a5a39cfd9e979ff3dce Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 5 Jun 2025 08:59:07 +0200 Subject: [PATCH 4/8] use more specific joins for delete request --- .../v3/service_instances_controller.rb | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 02f8c1f0c3b..fb3d38ea0ec 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -379,15 +379,20 @@ def fetch_writable_service_instance(guid) def enqueue_delete_job(service_instance) delete_job = V3::DeleteServiceInstanceJob.new(service_instance.guid, user_audit_info) - service_plan_relations = ServicePlan.eager_graph(service: :service_broker). - where(Sequel[:service_plans][:id] => service_instance.service_plan_id). - all - - service_plan = service_plan_relations[0] - - plan_name = service_plan.name - service_name = service_plan.service.name - broker_name = service_plan.service.service_broker.name + result = VCAP::CloudController::ServicePlan. + join(:services, id: :service_id). + join(:service_brokers, id: Sequel[:services][:service_broker_id]). + where(Sequel[:service_plans][:id] => service_instance.service_plan_id). + select( + Sequel[:service_plans][:name].as(:plan_name), + Sequel[:services][:label].as(:service_name), + Sequel[:service_brokers][:name].as(:broker_name) + ). + first + + plan_name = result[:plan_name] + service_name = result[:service_name] + broker_name = result[:broker_name] logger.info( "Deleting managed service instance with name '#{service_instance.name}' " \ From 8ac7378b29891edde48d5921e5320ef3142a4c02 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 5 Jun 2025 14:28:49 +0200 Subject: [PATCH 5/8] Add tests for the log info --- spec/request/service_instances_spec.rb | 51 ++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index fbb84b5f1fa..d3ae7c6eef6 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1080,6 +1080,12 @@ def check_filtered_instances(*instances) end let(:instance) { VCAP::CloudController::ServiceInstance.last } let(:job) { VCAP::CloudController::PollableJobModel.last } + let(:mock_logger) { instance_double(Steno::Logger, info: nil) } + + before do + allow(Steno).to receive(:logger).and_call_original + allow(Steno).to receive(:logger).with('cc.api').and_return(mock_logger) + end it 'creates a service instance in the database' do api_call.call(space_dev_headers) @@ -1107,6 +1113,17 @@ def check_filtered_instances(*instances) expect(job.resource_type).to eq('service_instances') end + it 'logs the correct names when creating a managed service instance' do + api_call.call(space_dev_headers) + + expect(mock_logger).to have_received(:info).with( + "Creating managed service instance with name '#{instance.name}' " \ + "using service plan '#{service_plan.name}' " \ + "from service offering '#{service_plan.service.name}' " \ + "provided by broker '#{service_plan.service.service_broker.name}'." + ) + end + context 'when the name has already been taken' do it 'fails when the same name is already used in this space' do VCAP::CloudController::ServiceInstance.make(name:, space:) @@ -1847,6 +1864,12 @@ def check_filtered_instances(*instances) } end let(:job) { VCAP::CloudController::PollableJobModel.last } + let(:mock_logger) { instance_double(Steno::Logger, info: nil) } + + before do + allow(Steno).to receive(:logger).and_call_original + allow(Steno).to receive(:logger).with('cc.api').and_return(mock_logger) + end it 'responds with a pollable job' do api_call.call(space_dev_headers) @@ -1883,6 +1906,17 @@ def check_filtered_instances(*instances) ) end + it 'logs the correct names when updating a managed service instance' do + api_call.call(space_dev_headers) + + expect(mock_logger).to have_received(:info).with( + "Updating managed service instance with name '#{service_instance.name}' " \ + "using service plan '#{original_service_plan.name}' " \ + "from service offering '#{service_offering.name}' " \ + "provided by broker '#{original_service_plan.service.service_broker.name}'." + ) + end + describe 'the pollable job' do let(:broker_response) { { dashboard_url: 'http://new-dashboard.url' } } let(:broker_status_code) { 200 } @@ -2970,6 +3004,12 @@ def check_filtered_instances(*instances) }). to_return(status: broker_status_code, body: broker_response.to_json, headers: {}) end + let(:mock_logger) { instance_double(Steno::Logger, info: nil) } + + before do + allow(Steno).to receive(:logger).and_call_original + allow(Steno).to receive(:logger).with('cc.api').and_return(mock_logger) + end it 'responds with job resource' do api_call.call(admin_headers) @@ -2984,6 +3024,17 @@ def check_filtered_instances(*instances) expect(job.resource_type).to eq('service_instance') end + it 'logs the correct names when deleting a managed service instance' do + api_call.call(admin_headers) + + expect(mock_logger).to have_received(:info).with( + "Deleting managed service instance with name '#{instance.name}' " \ + "using service plan '#{instance.service_plan.name}' " \ + "from service offering '#{instance.service_plan.service.name}' " \ + "provided by broker '#{instance.service_plan.service.service_broker.name}'." + ) + end + describe 'the pollable job' do it 'sends a delete request with the right arguments to the service broker' do api_call.call(headers_for(user, scopes: %w[cloud_controller.admin])) From 511e64b9d933de1fe9454e2ccc5108e4a0e9b6a7 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Fri, 13 Jun 2025 11:08:59 +0200 Subject: [PATCH 6/8] Eager load associations of service instance --- .../v3/service_instances_controller.rb | 39 +++++++------------ 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index fb3d38ea0ec..6c3ef8c293b 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -311,16 +311,11 @@ def update_managed(service_instance) action = V3::ServiceInstanceUpdateManaged.new(service_instance, message, user_audit_info, message.audit_hash) action.preflight! if action.update_broker_needed? - - plan_name = service_instance.service_plan.name - service_name = service_instance.service_plan.service.label - broker_name = service_instance.service_plan.service.service_broker.name - logger.info( "Updating managed service instance with name '#{service_instance.name}' " \ - "using service plan '#{plan_name}' " \ - "from service offering '#{service_name}' " \ - "provided by broker '#{broker_name}'." + "using service plan '#{service_instance.service_plan.name}' " \ + "from service offering '#{service_instance.service_plan.service.label}' " \ + "provided by broker '#{service_instance.service_plan.service.service_broker.name}'." ) update_job = action.enqueue_update @@ -368,7 +363,10 @@ def build_create_message(params) end def fetch_writable_service_instance(guid) - service_instance = ServiceInstance.first(guid:) + service_instances = ManagedServiceInstance.eager_graph(service_plan: { service: :service_broker }).where(Sequel[:service_instances][:guid] => guid).all + service_instance = service_instances[0] unless service_instances.empty? + service_instance = UserProvidedServiceInstance.first(guid:) if service_instance.nil? + service_instance_not_found! unless service_instance && can_read_service_instance?(service_instance) unauthorized! unless can_write_to_active_space?(service_instance.space) suspended! unless is_space_active?(service_instance.space) @@ -379,26 +377,15 @@ def fetch_writable_service_instance(guid) def enqueue_delete_job(service_instance) delete_job = V3::DeleteServiceInstanceJob.new(service_instance.guid, user_audit_info) - result = VCAP::CloudController::ServicePlan. - join(:services, id: :service_id). - join(:service_brokers, id: Sequel[:services][:service_broker_id]). - where(Sequel[:service_plans][:id] => service_instance.service_plan_id). - select( - Sequel[:service_plans][:name].as(:plan_name), - Sequel[:services][:label].as(:service_name), - Sequel[:service_brokers][:name].as(:broker_name) - ). - first - - plan_name = result[:plan_name] - service_name = result[:service_name] - broker_name = result[:broker_name] + plan = service_instance.service_plan + service = plan.service + broker = service.service_broker logger.info( "Deleting managed service instance with name '#{service_instance.name}' " \ - "using service plan '#{plan_name}' " \ - "from service offering '#{service_name}' " \ - "provided by broker '#{broker_name}'." + "using service plan '#{plan.name}' " \ + "from service offering '#{service.label}' " \ + "provided by broker '#{broker.name}'." ) pollable_job = Jobs::Enqueuer.new(queue: Jobs::Queues.generic).enqueue_pollable(delete_job) From 84ed291af452055abb3fba9697392150ddf85395 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Wed, 9 Jul 2025 14:21:37 +0200 Subject: [PATCH 7/8] Remove unnecessary reload of service_instance Reloading the service_instance just ensures that follow-up preflight checks run with the original service instance values. This can also be achieved by applying changes on a copy of the object. With that, we don't loose eagerly loaded associations and save a few DB queries. --- .../v3/service_instance_update_managed.rb | 11 ++--- .../v3/service_instances_controller.rb | 29 ++++++++++---- spec/request/service_instances_spec.rb | 40 +++++++++++++++---- .../service_instance_update_managed_spec.rb | 22 ++++++++++ 4 files changed, 82 insertions(+), 20 deletions(-) diff --git a/app/actions/v3/service_instance_update_managed.rb b/app/actions/v3/service_instance_update_managed.rb index 2c7b5be3445..ef534871716 100644 --- a/app/actions/v3/service_instance_update_managed.rb +++ b/app/actions/v3/service_instance_update_managed.rb @@ -268,11 +268,12 @@ def raise_if_cannot_proceed! def raise_if_invalid_update! return unless message.updates.any? - service_instance.set(message.updates) - return service_instance.reload if service_instance.valid? + service_instance_copy = service_instance.dup + service_instance_copy.set(message.updates) + return if service_instance_copy.valid? - service_instance_name_errors = service_instance.errors.on(:name).to_a - service_plan_errors = service_instance.errors.on(:service_plan).to_a + service_instance_name_errors = service_instance_copy.errors.on(:name).to_a + service_plan_errors = service_instance_copy.errors.on(:service_plan).to_a if service_instance_name_errors.include?(:unique) raise UnprocessableUpdate.new_from_details('ServiceInstanceNameTaken', message.name) @@ -282,7 +283,7 @@ def raise_if_invalid_update! raise UnprocessableUpdate.new_from_details('ServiceInstanceServicePlanNotAllowed') end - raise Sequel::ValidationFailed.new(service_instance) + raise Sequel::ValidationFailed.new(service_instance_copy) end def raise_if_renaming_shared_service_instance! diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 6c3ef8c293b..e9826dd776f 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -310,15 +310,10 @@ def update_managed(service_instance) action = V3::ServiceInstanceUpdateManaged.new(service_instance, message, user_audit_info, message.audit_hash) action.preflight! - if action.update_broker_needed? - logger.info( - "Updating managed service instance with name '#{service_instance.name}' " \ - "using service plan '#{service_instance.service_plan.name}' " \ - "from service offering '#{service_instance.service_plan.service.label}' " \ - "provided by broker '#{service_instance.service_plan.service.service_broker.name}'." - ) + if action.update_broker_needed? update_job = action.enqueue_update + log_service_instance_update(message, service_instance) head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{update_job.guid}") else service_instance = action.update_sync @@ -330,6 +325,26 @@ def update_managed(service_instance) raise CloudController::Errors::ApiError.new_from_details('AsyncServiceBindingOperationInProgress', e.service_binding.app.name, e.service_binding.service_instance.name) end + def log_service_instance_update(message, service_instance) + if message.service_plan_guid && message.service_plan_guid != service_instance.service_plan.guid + new_service_plan_name = ServicePlan.first(guid: message.service_plan_guid).name + + logger.info( + "Updating managed service instance with name '#{service_instance.name}' " \ + "changing plan from '#{service_instance.service_plan.name}' to '#{new_service_plan_name}' " \ + "from service offering '#{service_instance.service_plan.service.label}' " \ + "provided by broker '#{service_instance.service_plan.service.service_broker.name}'." + ) + else + logger.info( + "Updating managed service instance with name '#{service_instance.name}' " \ + "using service plan '#{service_instance.service_plan.name}' " \ + "from service offering '#{service_instance.service_plan.service.label}' " \ + "provided by broker '#{service_instance.service_plan.service.service_broker.name}'." + ) + end + end + def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces) unreadable_spaces = found_spaces.reject { |s| can_read_from_space?(s) } unwriteable_spaces = found_spaces.reject { |s| (can_write_to_active_space?(s) && is_space_active?(s)) || unreadable_spaces.include?(s) } diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index d3ae7c6eef6..15f63e6fd90 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1906,15 +1906,39 @@ def check_filtered_instances(*instances) ) end - it 'logs the correct names when updating a managed service instance' do - api_call.call(space_dev_headers) + describe 'logging of updates' do + it 'logs info including the change of service plans' do + api_call.call(space_dev_headers) - expect(mock_logger).to have_received(:info).with( - "Updating managed service instance with name '#{service_instance.name}' " \ - "using service plan '#{original_service_plan.name}' " \ - "from service offering '#{service_offering.name}' " \ - "provided by broker '#{original_service_plan.service.service_broker.name}'." - ) + expect(mock_logger).to have_received(:info).with( + "Updating managed service instance with name '#{service_instance.name}' " \ + "changing plan from '#{original_service_plan.name}' to '#{new_service_plan.name}' " \ + "from service offering '#{service_offering.name}' " \ + "provided by broker '#{original_service_plan.service.service_broker.name}'." + ) + end + + context 'when service plan does not change' do + let(:request_body) do + { + parameters: { + foo: 'bar', + baz: 'qux' + } + } + end + + it 'logs info accordingly' do + api_call.call(space_dev_headers) + + expect(mock_logger).to have_received(:info).with( + "Updating managed service instance with name '#{service_instance.name}' " \ + "using service plan '#{original_service_plan.name}' " \ + "from service offering '#{service_offering.name}' " \ + "provided by broker '#{original_service_plan.service.service_broker.name}'." + ) + end + end end describe 'the pollable job' do diff --git a/spec/unit/actions/v3/service_instance_update_managed_spec.rb b/spec/unit/actions/v3/service_instance_update_managed_spec.rb index 4cb49cf9766..86a82cb6be8 100644 --- a/spec/unit/actions/v3/service_instance_update_managed_spec.rb +++ b/spec/unit/actions/v3/service_instance_update_managed_spec.rb @@ -75,6 +75,28 @@ module V3 end describe '#preflight!' do + context 'when changes were requested' do + let(:body) do + { + name: 'new-name', + relationships: { + service_plan: { + data: { + guid: new_plan.guid + } + } + } + } + end + + let(:new_plan) { ServicePlan.make(service: original_service_offering) } + + it 'does not change the original service instance object' do + action.preflight! + expect(original_instance.changed_columns).not_to be_any + end + end + describe 'invalid name updates' do context 'when the new name is already taken' do let(:instance_in_same_space) { ServiceInstance.make(space: original_instance.space) } From 0f7749ef654cd4fbebce082c1046df439a83a9f5 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Thu, 10 Jul 2025 10:01:59 +0200 Subject: [PATCH 8/8] Further reduce DB queries --- app/controllers/v3/service_instances_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index e9826dd776f..84a24b94858 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -312,8 +312,10 @@ def update_managed(service_instance) action.preflight! if action.update_broker_needed? + # enqueue_update will reload the service instance and we loose the eagerly loaded associations + service_instance_with_eager_load = service_instance.dup update_job = action.enqueue_update - log_service_instance_update(message, service_instance) + log_service_instance_update(message, service_instance_with_eager_load) head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{update_job.guid}") else service_instance = action.update_sync