Skip to content

Commit

Permalink
v3(services) Override default order by service cred bindings
Browse files Browse the repository at this point in the history
The `order_by` applied in the View that service credential binding controller depends on was being overriden by the defult pagination order_by paginator option -which is the `id` column.
For this endpoint the `id` is not a good one to order by, since the view is joining two tables and the same `id` might be duplicated. We override the default `order_by` in the controller to be `created_at`

Co-Authored-By: Marcela Campo <[email protected]>
  • Loading branch information
FelisiaM and pivotal-marcela-campo committed Jan 12, 2021
1 parent 38be355 commit 309e311
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 13 deletions.
20 changes: 14 additions & 6 deletions app/controllers/v3/service_credential_bindings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@ def index

results = list_fetcher.fetch(space_guids: space_guids, message: message)

default_order_by_overriden = override_default_order_by(message)
page_results = SequelPaginator.new.get_page(results, message.try(:pagination_options))
page_results.pagination_options.order_by = nil if default_order_by_overriden

presenter = Presenters::V3::PaginatedListPresenter.new(
presenter: Presenters::V3::ServiceCredentialBindingPresenter,
paginated_result: SequelPaginator.new.get_page(results, message.try(:pagination_options)),
paginated_result: page_results,
path: '/v3' + service_credential_bindings_path,
message: message,
decorators: decorators(message)
Expand Down Expand Up @@ -117,6 +121,15 @@ def parameters

private

def override_default_order_by(message)
order_by_set = false
unless message.pagination_options.ordering_configured?
message.pagination_options.order_by = 'created_at'
order_by_set = true
end
order_by_set
end

def create_key_binding(message, service_instance)
action = V3::ServiceCredentialBindingKeyCreate.new(user_audit_info, message.audit_hash)
binding = action.precursor(service_instance, message: message)
Expand Down Expand Up @@ -263,11 +276,6 @@ def space_guids
end
end

def pagination_options
query_params_with_order_by = query_params.reverse_merge(order_by: :created_at)
ListMessage.from_params(query_params_with_order_by, []).pagination_options
end

def serialized(message)
Presenters::V3::ServiceCredentialBindingPresenter.new(service_credential_binding, decorators: decorators(message)).to_hash
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/services/service_credential_binding_view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ module Types
SERVICE_BINDING_VIEW
].inject do |statement, sub_select|
statement.union(sub_select, all: true, from_self: false)
end.from_self.order_by(:created_at).freeze
end.from_self.freeze

class View < Sequel::Model(VIEW)
plugin :single_table_inheritance,
Expand Down
12 changes: 6 additions & 6 deletions spec/request/service_credential_bindings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,27 +55,27 @@
let(:now) { Time.now }
let(:instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) }
let(:other_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: other_space) }
let!(:key_binding) { VCAP::CloudController::ServiceKey.make(service_instance: instance, created_at: now - 4.seconds) }
let!(:other_key_binding) { VCAP::CloudController::ServiceKey.make(service_instance: other_instance, created_at: now - 3.seconds) }
let!(:key_binding) { VCAP::CloudController::ServiceKey.make(service_instance: instance) }
let!(:other_key_binding) { VCAP::CloudController::ServiceKey.make(service_instance: other_instance) }
let!(:app_binding) do
VCAP::CloudController::ServiceBinding.make(service_instance: instance, created_at: now - 2.seconds).tap do |binding|
VCAP::CloudController::ServiceBinding.make(service_instance: instance).tap do |binding|
operate_on(binding)
end
end
let!(:other_app_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: other_instance, created_at: now - 1.second, name: Sham.name) }
let!(:other_app_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: other_instance, name: Sham.name) }

describe 'permissions' do
let(:labels) { { foo: 'bar' } }
let(:annotations) { { baz: 'wow' } }
let!(:key_binding) do
VCAP::CloudController::ServiceKey.make(service_instance: instance, created_at: now - 4.seconds) do |binding|
VCAP::CloudController::ServiceKey.make(service_instance: instance) do |binding|
operate_on(binding)
VCAP::CloudController::ServiceKeyLabelModel.make(key_name: 'foo', value: 'bar', service_key: binding)
VCAP::CloudController::ServiceKeyAnnotationModel.make(key_name: 'baz', value: 'wow', service_key: binding)
end
end
let!(:other_app_binding) do
VCAP::CloudController::ServiceBinding.make(service_instance: other_instance, created_at: now - 1.second, name: Sham.name) do |binding|
VCAP::CloudController::ServiceBinding.make(service_instance: other_instance, name: Sham.name) do |binding|
operate_on(binding)
VCAP::CloudController::ServiceBindingLabelModel.make(key_name: 'foo', value: 'bar', service_binding: binding)
VCAP::CloudController::ServiceBindingAnnotationModel.make(key_name: 'baz', value: 'wow', service_binding: binding)
Expand Down

0 comments on commit 309e311

Please sign in to comment.