From da84f01258843c039cfa00bbbefc57e5c2e3837e Mon Sep 17 00:00:00 2001 From: Merric de Launey Date: Fri, 5 Aug 2022 20:56:32 +0000 Subject: [PATCH] Wrap space guid within data field in PATCH v3/routes/:guid/relationships/space This change ensures the endpoint adheres to the v3 api style guide https://github.com/cloudfoundry/cloud_controller_ng/blob/main/docs/v3_style_guide.md [#182668597] Authored-by: Merric de Launey --- app/controllers/v3/routes_controller.rb | 4 +- app/messages/route_transfer_owner_message.rb | 23 +++- spec/request/routes_spec.rb | 12 +- .../route_transfer_owner_message_spec.rb | 127 +++++++++++++++--- 4 files changed, 134 insertions(+), 32 deletions(-) diff --git a/app/controllers/v3/routes_controller.rb b/app/controllers/v3/routes_controller.rb index 3b7847f3eb0..e15e3651351 100644 --- a/app/controllers/v3/routes_controller.rb +++ b/app/controllers/v3/routes_controller.rb @@ -172,9 +172,9 @@ def transfer_owner unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.guid) suspended! unless permission_queryer.is_space_active?(route.space.guid) - target_space = Space.first(guid: message.guid) + target_space = Space.first(guid: message.space_guid) target_space_error = check_if_space_is_accessible(target_space) - unprocessable!("Unable to transfer owner of route '#{route.uri}' to space '#{message.guid}'. #{target_space_error}") unless target_space_error.nil? + unprocessable!("Unable to transfer owner of route '#{route.uri}' to space '#{message.space_guid}'. #{target_space_error}") unless target_space_error.nil? RouteTransferOwner.transfer(route, target_space, user_audit_info) diff --git a/app/messages/route_transfer_owner_message.rb b/app/messages/route_transfer_owner_message.rb index 399791e9240..8a029252463 100644 --- a/app/messages/route_transfer_owner_message.rb +++ b/app/messages/route_transfer_owner_message.rb @@ -2,9 +2,28 @@ module VCAP::CloudController class RouteTransferOwnerMessage < BaseMessage - register_allowed_keys [:guid] + register_allowed_keys [:data] validates_with NoAdditionalKeysValidator - validates :guid, presence: true, string: true, allow_nil: false, allow_blank: false + validates :data, presence: true, hash: true, allow_nil: false + validate :data_content + + def space_guid + HashUtils.dig(data, :guid) + end + + def data_content + return if data.nil? + + errors.add(:data, 'can only accept one key') unless data.keys.length == 1 + errors.add(:data, "can only accept key 'guid'") unless data.key?(:guid) + if space_guid && !space_guid.is_a?(String) + errors.add(:data, "#{space_guid} must be a string") + return + end + if space_guid && space_guid.empty? + errors.add(:data, "guid can't be blank") + end + end end end diff --git a/spec/request/routes_spec.rb b/spec/request/routes_spec.rb index e68ac754b85..790e869d069 100644 --- a/spec/request/routes_spec.rb +++ b/spec/request/routes_spec.rb @@ -3214,7 +3214,7 @@ let(:target_space) { VCAP::CloudController::Space.make(organization: org) } let(:request_body) do { - 'guid' => target_space.guid + data: { 'guid' => target_space.guid } } end let(:space_dev_headers) do @@ -3261,7 +3261,7 @@ let(:suspended_space) { VCAP::CloudController::Space.make } let(:request_body) do { - 'guid' => suspended_space.guid + data: { 'guid' => suspended_space.guid } } end @@ -3315,7 +3315,7 @@ let(:target_space_guid) { 'fake-target' } let(:request_body) do { - 'guid' => target_space_guid + data: { 'guid' => target_space_guid } } end @@ -3338,7 +3338,7 @@ let(:no_access_target_space) { VCAP::CloudController::Space.make(organization: org) } let(:request_body) do { - 'guid' => no_access_target_space.guid + data: { 'guid' => no_access_target_space.guid } } end @@ -3360,7 +3360,7 @@ let(:no_write_access_target_space) { VCAP::CloudController::Space.make(organization: org) } let(:request_body) do { - 'guid' => no_write_access_target_space.guid + data: { 'guid' => no_write_access_target_space.guid } } end @@ -3401,7 +3401,7 @@ context 'when there are additional keys' do let(:request_body) do { - 'guid' => target_space.guid, + data: { 'guid' => target_space.guid }, 'fake-key' => 'foo' } end diff --git a/spec/unit/messages/route_transfer_owner_message_spec.rb b/spec/unit/messages/route_transfer_owner_message_spec.rb index 7bc004552fd..2563d6d83aa 100644 --- a/spec/unit/messages/route_transfer_owner_message_spec.rb +++ b/spec/unit/messages/route_transfer_owner_message_spec.rb @@ -4,38 +4,121 @@ module VCAP::CloudController RSpec.describe RouteTransferOwnerMessage do - context 'when unexpected keys are requested' do - let(:params) { { guid: 'some-guid', unexpected: 'foo' } } + describe 'validations' do + context 'when it contains the right content' do + let(:symbolized_body) do + { + data: { guid: 'some-guid' } + } + end - it 'is not valid' do - message = RouteTransferOwnerMessage.new(params) + it 'it is valid' do + message = RouteTransferOwnerMessage.new(symbolized_body) - expect(message).not_to be_valid - expect(message.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") + expect(message).to be_valid + expect(message.errors.count).to eq(0) + end end - end - context 'when guid is not a string' do - let(:params) { { guid: 5 } } + context 'when there is no data in the object' do + let(:symbolized_body) do + {} + end - it 'is not valid' do - message = RouteTransferOwnerMessage.new(params) + it 'returns an error' do + message = RouteTransferOwnerMessage.new(symbolized_body) - expect(message).not_to be_valid - expect(message.errors.count).to eq(1) - expect(message.errors[:guid]).to include('must be a string') + expect(message).to_not be_valid + expect(message.errors[:data]).to include("can't be blank") + end + end + + context 'when there is no guid in the data' do + let(:symbolized_body) do + { + data: {} + } + end + + it 'returns an error' do + message = RouteTransferOwnerMessage.new(symbolized_body) + + expect(message).to_not be_valid + expect(message.errors[:data]).to include("can't be blank") + end + end + + context 'when data is nil' do + let(:symbolized_body) do + { + data: nil + } + end + + it 'does not error and returns the correct message' do + message = RouteTransferOwnerMessage.new(symbolized_body) + + expect(message).not_to be_valid + expect(message.errors[:data]).to include("can't be blank") + end + end + + context 'when unexpected keys are requested' do + let(:params) { { data: { guid: 'some-guid' }, unexpected: 'foo' } } + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(params) + + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") + end + + context 'when there are unexpected keys inside data hash' do + let(:symbolized_body) { + { + data: { blah: 'awesome-guid' }, + } + } + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(symbolized_body) + + expect(message).to_not be_valid + expect(message.errors[:data]).to include("can only accept key 'guid'") + end + end + end + + context 'when guid is not a string' do + let(:symbolized_body) do + { + data: { guid: 32.77 } + } + end + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(symbolized_body) + + expect(message).not_to be_valid + expect(message.errors.count).to eq(1) + expect(message.errors[:data]).to include('32.77 must be a string') + end end - end - context 'when guid is not present' do - let(:params) { { guid: '' } } + context 'when guid is not present' do + let(:symbolized_body) do + { + data: { guid: '' } + } + end - it 'is not valid' do - message = RouteTransferOwnerMessage.new(params) + it 'is not valid' do + message = RouteTransferOwnerMessage.new(symbolized_body) - expect(message).not_to be_valid - expect(message.errors.count).to eq(1) - expect(message.errors[:guid]).to include("can't be blank") + expect(message).not_to be_valid + expect(message.errors.count).to eq(1) + expect(message.errors[:data]).to include("guid can't be blank") + end end end end