Skip to content
Merged
17 changes: 16 additions & 1 deletion app/controllers/services/service_instances_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ def update(guid)
validate_access(:read_for_update, service_instance)
validate_access(:update, projected_service_instance(service_instance))

validate_name_update(service_instance)
validate_space_update(related_objects[:space])
validate_plan_update(related_objects[:plan], related_objects[:service], service_instance)

Expand Down Expand Up @@ -360,6 +361,12 @@ def unbind_route(route_guid, instance_guid)

private

class ServiceInstanceSharedToEagerLoader

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class will not be private unless you do

private_constant :ServiceInstanceSharedToEagerLoader

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we've made this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes for this included in #1033 and can be viewed here: cloudfoundry-incubator@ff8276e#diff-9b86a08059c9bdc2b6875a70abf9ee29R366

def eager_load_dataset(spaces, _, _, _, _)
spaces.eager(:organization)
end
end

class ServiceInstanceSharedToSerializer
def initialize(service_instance)
@service_instance = service_instance
Expand All @@ -373,7 +380,7 @@ def serialize(controller, space, opts, orphans=nil)

def create_paginated_collection_renderer(service_instance)
VCAP::CloudController::RestController::PaginatedCollectionRenderer.new(
VCAP::CloudController::RestController::SecureEagerLoader.new,
ServiceInstanceSharedToEagerLoader.new,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a pretty fundamental change to service instances controller. Subverting the framework and how it expects to load associations could lead to confusing behavior later. I'm surprised this didn't break any existing tests.

Is there another way we could configure the framework to eager load organizations in addition to it's usual machinations?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create_paginated_collection_renderer method, the ServiceInstanceSharedToEagerLoader and the ServiceInstanceSharedToSerializer are only used by the '/v2/service_instances/:guid/shared_to' endpoint. These are used to support paginated rendering using the custom ServiceInstanceSharedToPresenter presenter. We could not use the existing serialiser class as SharedTo does not have an associated model object. The eager loader is overridden in order to load the organisation model that is required by the presenter to display the org name.

These changes are confined to the shared_to endpoint, so it hasn't changed behaviour elsewhere in the controller.

ServiceInstanceSharedToSerializer.new(service_instance),
{
max_results_per_page: config.get(:renderer, :max_results_per_page),
Expand Down Expand Up @@ -432,6 +439,14 @@ def validate_space_update(space)
space_change_not_allowed! if space_change_requested?(request_attrs['space_guid'], space)
end

def validate_name_update(service_instance)
return unless request_attrs['name'] && service_instance.shared?

if request_attrs['name'] != service_instance.name
raise CloudController::Errors::ApiError.new_from_details('SharedServiceInstanceCannotBeRenamed')
end
end

def invalid_service_instance!(service_instance)
raise Sequel::ValidationFailed.new(service_instance)
end
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/v3/service_instances_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@
require 'presenters/v3/paginated_list_presenter'
require 'actions/service_instance_share'
require 'actions/service_instance_unshare'
require 'fetchers/service_instance_list_fetcher'
require 'fetchers/managed_service_instance_list_fetcher'

class ServiceInstancesV3Controller < ApplicationController
def index
message = ServiceInstancesListMessage.from_params(query_params)
invalid_param!(message.errors.full_messages) unless message.valid?

dataset = if can_read_globally?
ServiceInstanceListFetcher.new.fetch_all(message: message)
ManagedServiceInstanceListFetcher.new.fetch_all(message: message)
else
ServiceInstanceListFetcher.new.fetch(message: message, space_guids: readable_space_guids)
ManagedServiceInstanceListFetcher.new.fetch(message: message, readable_space_guids: readable_space_guids)
end

render status: :ok, json: Presenters::V3::PaginatedListPresenter.new(
Expand Down
35 changes: 35 additions & 0 deletions app/fetchers/managed_service_instance_list_fetcher.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module VCAP::CloudController
class ManagedServiceInstanceListFetcher
def fetch(message:, readable_space_guids:)
source_space_instance_dataset = ManagedServiceInstance.select_all(ServiceInstance.table_name).
join(Space.table_name, id: :space_id, guid: readable_space_guids)

shared_instance_dataset = ManagedServiceInstance.select_all(ServiceInstance.table_name).
join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: readable_space_guids)

dataset = source_space_instance_dataset.union(shared_instance_dataset, alias: :service_instances)

filter(dataset, message)
end

def fetch_all(message:)
dataset = ManagedServiceInstance.dataset
filter(dataset, message)
end

private

def filter(dataset, message)
if message.requested?(:names)
dataset = dataset.where(service_instances__name: message.names)
end

if message.requested?(:space_guids)
dataset = dataset.select_all(ServiceInstance.table_name).
join_table(:inner, Space.table_name, { id: Sequel.qualify(:service_instances, :space_id), guid: message.space_guids })
end

dataset
end
end
end
30 changes: 0 additions & 30 deletions app/fetchers/service_instance_list_fetcher.rb

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@

module VCAP::CloudController
class ServiceInstancesListMessage < ListMessage
ALLOWED_KEYS = [:page, :per_page, :order_by, :names].freeze
ALLOWED_KEYS = [:page, :per_page, :order_by, :names, :space_guids].freeze

attr_accessor(*ALLOWED_KEYS)

validates_with NoAdditionalParamsValidator
validates :names, array: true, allow_nil: true
validates :space_guids, array: true, allow_nil: true

def initialize(params={})
super(params.symbolize_keys)
Expand All @@ -16,6 +17,7 @@ def initialize(params={})
def self.from_params(params)
opts = params.dup
to_array! opts, 'names'
to_array! opts, 'space_guids'
new(opts.symbolize_keys)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module V2
class ServiceInstanceSharedFromPresenter
def to_hash(space)
{
'space_guid' => space.guid,
'space_name' => space.name,
'organization_name' => space.organization.name
}
Expand Down
18 changes: 17 additions & 1 deletion app/presenters/v3/service_instance_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,28 @@ def to_hash
guid: service_instance.guid,
created_at: service_instance.created_at,
updated_at: service_instance.updated_at,
name: service_instance.name
name: service_instance.name,
relationships: {
space: {
data: {
guid: service_instance.space.guid
}
}
},
links: {
space: {
href: url_builder.build_url(path: "/v3/spaces/#{service_instance.space.guid}")
}
}
}
end

private

def url_builder
VCAP::CloudController::Presenters::ApiUrlBuilder.new
end

def service_instance
@resource
end
Expand Down
20 changes: 16 additions & 4 deletions docs/v3/source/includes/api_resources/_service_instances.erb
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,22 @@
},
"resources": [
{
"guid": "d4c91047-7b29-4fda-b7f9-04033e5c9c9f",
"created_at": "2017-02-02T00:14:30Z",
"updated_at": "2017-02-02T00:14:30Z",
"name": "my_service_instance"
"guid": "85ccdcad-d725-4109-bca4-fd6ba062b5c8",
"created_at": "2017-11-17T13:54:21Z",
"updated_at": "2017-11-17T13:54:21Z",
"name": "my_service_instance",
"relationships": {
"space": {
"data": {
"guid": "ae0031f9-dd49-461c-a945-df40e77c39cb"
}
}
},
"links": {
"space": {
"href": "https://api.example.org/v3/spaces/ae0031f9-dd49-461c-a945-df40e77c39cb"
}
}
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ This includes access granted by service instance sharing.
Name | Type | Description
---- | ---- | ------------
**names** | _list of strings_ | Comma-delimited list of service instance names to filter by.
**space_guids** | _list of strings_ | Comma-delimited list of space guids to filter by.
**page** | _integer_ | Page to display. Valid values are integers >= 1.
**per_page** | _integer_ | Number of results per page. <br>Valid values are 1 through 5000.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
### The service_instance object

Name | Type | Description
---- | ---- | -----------
**name** | _string_ | Name of the service instance.
**guid** | _uuid_ | Unique identifier for the service instance.
**created_at** | _datetime_ | The time with zone when the object was created.
**updated_at** | _datetime_ | The time with zone when the object was last updated.
**space** | [_to-one relationship_](#to-one-relationships) | The space the service instance is contained in.
**links** | [_links object_](#links) | Links to related resources.
1 change: 1 addition & 0 deletions docs/v3/source/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ includes:
- experimental_resources/service_bindings/delete
- experimental_resources/service_bindings/list
- experimental_resources/service_instances/header
- experimental_resources/service_instances/object
- experimental_resources/service_instances/list
- experimental_resources/service_instances/share_to_space
- experimental_resources/service_instances/unshare_from_space
Expand Down
Loading