From 01c5174ba9633c704f8c0be22e74e815c969a89d Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Tue, 11 Feb 2025 16:06:53 +0100 Subject: [PATCH 1/9] Better error message and support logs for /v3/service_offerings and plans --- .../v3/service_instances_controller.rb | 19 ++++++++++++++----- spec/request/service_instances_spec.rb | 4 ++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 42d4e4e2a94..915b2cf3c39 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -254,7 +254,8 @@ def create_user_provided(message) def create_managed(message, space:) service_plan = ServicePlan.first(guid: message.service_plan_guid) - unprocessable_service_plan! unless service_plan_valid?(service_plan, space) + unprocessable_service_plan! unless service_plan_valid?(service_plan) + service_plan_not_visible_in_space! unless resource_exists_in_space?(service_plan, space) action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash) VCAP::CloudController::ManagedServiceInstance.db.transaction do @@ -394,17 +395,21 @@ def admin? permission_queryer.can_write_globally? end - def service_plan_valid?(service_plan, space) + def service_plan_valid?(service_plan) service_plan && - visible_to_current_user?(plan: service_plan) && - service_plan.visible_in_space?(space) + visible_to_current_user?(plan: service_plan) + end + + def resource_exists_in_space?(service_plan, space) + service_plan.visible_in_space?(space) end def raise_if_invalid_service_plan!(service_instance, message) return unless message.service_plan_guid service_plan = ServicePlan.first(guid: message.service_plan_guid) - unprocessable_service_plan! unless service_plan_valid?(service_plan, service_instance.space) + unprocessable_service_plan! unless service_plan_valid?(service_plan) + service_plan_not_visible_in_space! unless resource_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end @@ -424,6 +429,10 @@ def unprocessable_service_plan! unprocessable!('Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.') end + def service_plan_not_visible_in_space! + unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space.') + end + def invalid_service_plan_relation! raise CloudController::Errors::ApiError.new_from_details('InvalidRelation', 'service plan relates to a different service offering') end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index d84aaacc4ff..ba6aeda143d 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,7 +1260,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -2413,7 +2413,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) From 732c2f305b499c6a245eec0109f8658cada89ff0 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Tue, 11 Feb 2025 16:43:15 +0100 Subject: [PATCH 2/9] rename function --- app/controllers/v3/service_instances_controller.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 915b2cf3c39..9875a9ecf6c 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -255,7 +255,7 @@ def create_user_provided(message) def create_managed(message, space:) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - service_plan_not_visible_in_space! unless resource_exists_in_space?(service_plan, space) + service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, space) action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash) VCAP::CloudController::ManagedServiceInstance.db.transaction do @@ -397,10 +397,10 @@ def admin? def service_plan_valid?(service_plan) service_plan && - visible_to_current_user?(plan: service_plan) + visible_to_current_user?(plan: service_plan) end - def resource_exists_in_space?(service_plan, space) + def service_plan_exists_in_space?(service_plan, space) service_plan.visible_in_space?(space) end @@ -409,7 +409,7 @@ def raise_if_invalid_service_plan!(service_instance, message) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - service_plan_not_visible_in_space! unless resource_exists_in_space?(service_plan, service_instance.space) + service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end From 48ea714cf9f58df72dd2f357ef28411a1e45d657 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Wed, 12 Feb 2025 14:16:59 +0100 Subject: [PATCH 3/9] enhance error message --- app/controllers/v3/service_instances_controller.rb | 2 +- spec/request/service_instances_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 9875a9ecf6c..17f5e792dad 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -430,7 +430,7 @@ def unprocessable_service_plan! end def service_plan_not_visible_in_space! - unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space.') + unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.') end def invalid_service_plan_relation! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index ba6aeda143d..7bf58bc2908 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,7 +1260,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -1276,7 +1276,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -2413,7 +2413,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) From 0eb58efaaae4d7ff49f169fc819144317e269f5f Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Feb 2025 12:30:59 +0100 Subject: [PATCH 4/9] add error message if plan is inactive --- app/controllers/v3/service_instances_controller.rb | 12 +++++++++++- spec/request/service_instances_spec.rb | 6 +++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 17f5e792dad..8ec67c6d4eb 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -255,6 +255,7 @@ def create_user_provided(message) def create_managed(message, space:) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) + unavailable_service_plan!(service_plan.name) unless service_plan_active?(service_plan) service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, space) action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash) @@ -400,6 +401,10 @@ def service_plan_valid?(service_plan) visible_to_current_user?(plan: service_plan) end + def service_plan_active?(service_plan) + service_plan.active? + end + def service_plan_exists_in_space?(service_plan, space) service_plan.visible_in_space?(space) end @@ -409,6 +414,7 @@ def raise_if_invalid_service_plan!(service_instance, message) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) + unavailable_service_plan!(service_plan.name) unless service_plan_active?(service_plan) service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end @@ -429,8 +435,12 @@ def unprocessable_service_plan! unprocessable!('Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.') end + def unavailable_service_plan!(service_plan) + unprocessable!("Invalid service plan. The service plan #{service_plan} has been removed from the service broker's catalog. It is not possible to create new service instances using this plan.") + end + def service_plan_not_visible_in_space! - unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.') + unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space.') end def invalid_service_plan_relation! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 7bf58bc2908..ae1ee96518a 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,7 +1260,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', + 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -1276,7 +1276,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', + 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker\'s catalog. It is not possible to create new service instances using this plan.", 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -2413,7 +2413,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space and still available in the broker\'s catalog.', + 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker\'s catalog. It is not possible to create new service instances using this plan.", 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) From 586999dc5bf092725ca20e14c246d6a7a0f1860f Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Feb 2025 12:42:14 +0100 Subject: [PATCH 5/9] fix formatting --- app/controllers/v3/service_instances_controller.rb | 3 ++- spec/request/service_instances_spec.rb | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 8ec67c6d4eb..d15da2ecbff 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -436,7 +436,8 @@ def unprocessable_service_plan! end def unavailable_service_plan!(service_plan) - unprocessable!("Invalid service plan. The service plan #{service_plan} has been removed from the service broker's catalog. It is not possible to create new service instances using this plan.") + unprocessable!("Invalid service plan. The service plan #{service_plan} has been removed from the service broker's catalog." \ + 'It is not possible to create new service instances using this plan.') end def service_plan_not_visible_in_space! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index ae1ee96518a..05573a8fb56 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1276,7 +1276,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker\'s catalog. It is not possible to create new service instances using this plan.", + 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker's catalog." \ + 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -2413,7 +2414,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker\'s catalog. It is not possible to create new service instances using this plan.", + 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker's catalog." \ + 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) From 93828d8f6cbe217bf8dfe524289b00fc8447c544 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Feb 2025 13:25:51 +0100 Subject: [PATCH 6/9] add info about sp and sp to msg --- .../v3/service_instances_controller.rb | 19 +++++++++++-------- spec/request/service_instances_spec.rb | 9 ++++++--- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index d15da2ecbff..30fe2377ccb 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -255,8 +255,9 @@ def create_user_provided(message) def create_managed(message, space:) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - unavailable_service_plan!(service_plan.name) unless service_plan_active?(service_plan) - service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, space) + unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan) + service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \ + unless service_plan_exists_in_space?(service_plan, space) action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash) VCAP::CloudController::ManagedServiceInstance.db.transaction do @@ -414,8 +415,9 @@ def raise_if_invalid_service_plan!(service_instance, message) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - unavailable_service_plan!(service_plan.name) unless service_plan_active?(service_plan) - service_plan_not_visible_in_space! unless service_plan_exists_in_space?(service_plan, service_instance.space) + unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan) + service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \ + unless service_plan_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end @@ -435,13 +437,14 @@ def unprocessable_service_plan! unprocessable!('Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.') end - def unavailable_service_plan!(service_plan) - unprocessable!("Invalid service plan. The service plan #{service_plan} has been removed from the service broker's catalog." \ + def unavailable_service_plan!(service_plan, service_plan_guid) + unprocessable!("Invalid service plan. The service plan #{service_plan} with guid #{service_plan_guid} has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.') end - def service_plan_not_visible_in_space! - unprocessable!('Invalid service plan. Ensure that the service plan is visible in your current space.') + def service_plan_not_visible_in_space!(service_plan, service_plan_guid, space_name, space_guid) + unprocessable!("Invalid service plan. Ensure that the service plan #{service_plan} with guid #{service_plan_guid} \ +is visible in your current space #{space_name} with guid #{space_guid}.") end def invalid_service_plan_relation! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 05573a8fb56..ada3cc7da4f 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,7 +1260,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => 'Invalid service plan. Ensure that the service plan is visible in your current space.', + 'detail' => "Invalid service plan. Ensure that the service plan #{service_plan.name} with \ +guid #{service_plan_guid} is visible in your current space #{space.name} with guid #{space.guid}.", 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -1276,7 +1277,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker's catalog." \ + 'detail' => "Invalid service plan. The service plan #{service_plan.name} with guid #{service_plan.guid} " \ + "has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 @@ -2414,7 +2416,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} has been removed from the service broker's catalog." \ + 'detail' => "Invalid service plan. The service plan #{service_plan.name} with guid #{service_plan.guid} " \ + "has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 From 7665514ae791b91588467377fd60c1c64530a281 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:16:15 +0100 Subject: [PATCH 7/9] set sp and sp name in quotes --- app/controllers/v3/service_instances_controller.rb | 6 +++--- spec/request/service_instances_spec.rb | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 30fe2377ccb..c7f8e5795ee 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -438,13 +438,13 @@ def unprocessable_service_plan! end def unavailable_service_plan!(service_plan, service_plan_guid) - unprocessable!("Invalid service plan. The service plan #{service_plan} with guid #{service_plan_guid} has been removed from the service broker's catalog." \ + unprocessable!("Invalid service plan. The service plan '#{service_plan}' with guid '#{service_plan_guid}' has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.') end def service_plan_not_visible_in_space!(service_plan, service_plan_guid, space_name, space_guid) - unprocessable!("Invalid service plan. Ensure that the service plan #{service_plan} with guid #{service_plan_guid} \ -is visible in your current space #{space_name} with guid #{space_guid}.") + unprocessable!("Invalid service plan. Ensure that the service plan '#{service_plan}' with guid '#{service_plan_guid}' \ +is visible in your current space '#{space_name}' with guid '#{space_guid}'.") end def invalid_service_plan_relation! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index ada3cc7da4f..87e4e13975f 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,8 +1260,8 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. Ensure that the service plan #{service_plan.name} with \ -guid #{service_plan_guid} is visible in your current space #{space.name} with guid #{space.guid}.", + 'detail' => "Invalid service plan. Ensure that the service plan '#{service_plan.name}' with \ +guid '#{service_plan_guid}' is visible in your current space '#{space.name}' with guid '#{space.guid}'.", 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -1277,7 +1277,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} with guid #{service_plan.guid} " \ + 'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \ "has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', @@ -2416,7 +2416,7 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. The service plan #{service_plan.name} with guid #{service_plan.guid} " \ + 'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \ "has been removed from the service broker's catalog." \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', From 4dd08b654b186500e01c4d6cd0ff7f5c995efb3d Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Feb 2025 17:49:55 +0100 Subject: [PATCH 8/9] refactor error mesg + function params --- .../v3/service_instances_controller.rb | 22 +++++++++---------- spec/request/service_instances_spec.rb | 10 +++++---- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index c7f8e5795ee..7cc075ab489 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -255,9 +255,8 @@ def create_user_provided(message) def create_managed(message, space:) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan) - service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \ - unless service_plan_exists_in_space?(service_plan, space) + unavailable_service_plan!(service_plan) unless service_plan_active?(service_plan) + service_plan_not_visible_in_space!(service_plan, space) unless service_plan_exists_in_space?(service_plan, space) action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash) VCAP::CloudController::ManagedServiceInstance.db.transaction do @@ -415,9 +414,8 @@ def raise_if_invalid_service_plan!(service_instance, message) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) - unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan) - service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \ - unless service_plan_exists_in_space?(service_plan, service_instance.space) + unavailable_service_plan!(service_plan) unless service_plan_active?(service_plan) + service_plan_not_visible_in_space!(service_plan, space) unless service_plan_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end @@ -437,14 +435,16 @@ def unprocessable_service_plan! unprocessable!('Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.') end - def unavailable_service_plan!(service_plan, service_plan_guid) - unprocessable!("Invalid service plan. The service plan '#{service_plan}' with guid '#{service_plan_guid}' has been removed from the service broker's catalog." \ + def unavailable_service_plan!(service_plan) + unprocessable!("Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' has been removed from the service broker's catalog. " \ 'It is not possible to create new service instances using this plan.') end - def service_plan_not_visible_in_space!(service_plan, service_plan_guid, space_name, space_guid) - unprocessable!("Invalid service plan. Ensure that the service plan '#{service_plan}' with guid '#{service_plan_guid}' \ -is visible in your current space '#{space_name}' with guid '#{space_guid}'.") + def service_plan_not_visible_in_space!(service_plan, space) + unprocessable!('Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \ + "'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \ + 'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \ + "'#{space.name}' with guid '#{space.guid}'.") end def invalid_service_plan_relation! diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 87e4e13975f..0be444ae773 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1260,8 +1260,10 @@ def check_filtered_instances(*instances) expect(last_response).to have_status_code(422) expect(parsed_response['errors']).to include( include({ - 'detail' => "Invalid service plan. Ensure that the service plan '#{service_plan.name}' with \ -guid '#{service_plan_guid}' is visible in your current space '#{space.name}' with guid '#{space.guid}'.", + 'detail' => 'Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \ + "'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \ + 'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \ + "'#{space.name}' with guid '#{space.guid}'.", 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 }) @@ -1278,7 +1280,7 @@ def check_filtered_instances(*instances) expect(parsed_response['errors']).to include( include({ 'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \ - "has been removed from the service broker's catalog." \ + "has been removed from the service broker's catalog. " \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 @@ -2417,7 +2419,7 @@ def check_filtered_instances(*instances) expect(parsed_response['errors']).to include( include({ 'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \ - "has been removed from the service broker's catalog." \ + "has been removed from the service broker's catalog. " \ 'It is not possible to create new service instances using this plan.', 'title' => 'CF-UnprocessableEntity', 'code' => 10_008 From 5f87e3b98be637646fdf731e1c14b5c7f66e88e1 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Fri, 14 Feb 2025 14:13:57 +0100 Subject: [PATCH 9/9] add test for PATCH --- .../v3/service_instances_controller.rb | 4 ++-- spec/request/service_instances_spec.rb | 22 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 7cc075ab489..41f50d15e7c 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -415,7 +415,7 @@ def raise_if_invalid_service_plan!(service_instance, message) service_plan = ServicePlan.first(guid: message.service_plan_guid) unprocessable_service_plan! unless service_plan_valid?(service_plan) unavailable_service_plan!(service_plan) unless service_plan_active?(service_plan) - service_plan_not_visible_in_space!(service_plan, space) unless service_plan_exists_in_space?(service_plan, service_instance.space) + service_plan_not_visible_in_space!(service_plan, service_instance.space) unless service_plan_exists_in_space?(service_plan, service_instance.space) invalid_service_plan_relation! unless service_plan.service == service_instance.service end @@ -442,7 +442,7 @@ def unavailable_service_plan!(service_plan) def service_plan_not_visible_in_space!(service_plan, space) unprocessable!('Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \ - "'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \ + "'#{service_plan.name}' with guid '#{service_plan.guid}' in another space or that the plan " \ 'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \ "'#{space.name}' with guid '#{space.guid}'.") end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 0be444ae773..fbb84b5f1fa 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -1261,7 +1261,7 @@ def check_filtered_instances(*instances) expect(parsed_response['errors']).to include( include({ 'detail' => 'Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \ - "'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \ + "'#{service_plan.name}' with guid '#{service_plan.guid}' in another space or that the plan " \ 'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \ "'#{space.name}' with guid '#{space.guid}'.", 'title' => 'CF-UnprocessableEntity', @@ -2409,6 +2409,26 @@ def check_filtered_instances(*instances) end end + context 'not enabled in that org' do + let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: false, active: true) } + let(:service_plan_guid) { service_plan.guid } + + it 'fails saying the plan is invalid' do + api_call.call(admin_headers) + expect(last_response).to have_status_code(422) + expect(parsed_response['errors']).to include( + include({ + 'detail' => 'Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \ + "'#{service_plan.name}' with guid '#{service_plan.guid}' in another space or that the plan " \ + 'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \ + "'#{space.name}' with guid '#{space.guid}'.", + 'title' => 'CF-UnprocessableEntity', + 'code' => 10_008 + }) + ) + end + end + context 'not available' do let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: true, active: false) } let(:service_plan_guid) { service_plan.guid }