Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add second order_by parameter, as created_at might not be unique #3658

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/decorators/field_service_instance_broker_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def decorate(hash, service_instances)
join(:service_instances, service_plan_id: :service_plans__id).
where(service_instances__id: managed_service_instances.map(&:id)).
distinct.
order_by(:service_brokers__created_at).
order_by(:service_brokers__created_at, :service_brokers__guid).
select(:service_brokers__name, :service_brokers__guid, :service_brokers__created_at).
all

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def decorate(hash, service_instances)
join(:service_instances, service_plan_id: :service_plans__id).
where(service_instances__id: managed_service_instances.map(&:id)).
distinct.
order_by(:services__created_at).
order_by(:services__created_at, :services__guid).
select(:services__label, :services__guid, :services__description, :services__tags, :services__extra, :services__service_broker_id, :services__created_at).
all

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def decorate(hash, resources)
spaces = resources.map { |r| r.try(:space) || r }.uniq
orgs = spaces.map(&:organization).uniq

hash[:included][:organizations] = orgs.sort_by(&:created_at).map do |org|
hash[:included][:organizations] = orgs.sort_by { |o| [o.created_at, o.guid] }.map do |org|
org_view = {}
org_view[:name] = org.name if @fields.include?('name')
org_view[:guid] = org.guid if @fields.include?('guid')
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/field_service_instance_plan_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def decorate(hash, service_instances)
hash[:included] ||= {}
plans = managed_service_instances.map(&:service_plan).uniq

hash[:included][:service_plans] = plans.sort_by(&:created_at).map do |plan|
hash[:included][:service_plans] = plans.sort_by { |p| [p.created_at, p.guid] }.map do |plan|
plan_view = {}
plan_view[:guid] = plan.guid if @fields.include?('guid')
plan_view[:name] = plan.name if @fields.include?('name')
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/field_service_instance_space_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def decorate(hash, resources)

spaces = resources.map { |r| r.try(:space) || r }.uniq

hash[:included][:spaces] = spaces.sort_by(&:created_at).map do |space|
hash[:included][:spaces] = spaces.sort_by { |s| [s.created_at, s.guid] }.map do |space|
temp = {}
temp[:guid] = space.guid if @fields.include?('guid')
temp[:name] = space.name if @fields.include?('name')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def initialize(fields)
def decorate(hash, service_offerings)
hash[:included] ||= {}
service_brokers = service_offerings.map(&:service_broker).uniq
hash[:included][:service_brokers] = service_brokers.sort_by(&:created_at).map do |broker|
hash[:included][:service_brokers] = service_brokers.sort_by { |sb| [sb.created_at, sb.guid] }.map do |broker|
broker_view = {}
broker_view[:name] = broker.name if @fields.include?('name')
broker_view[:guid] = broker.guid if @fields.include?('guid')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def decorate(hash, service_plans)
hash[:included] ||= {}
service_offerings = service_plans.map(&:service).uniq
service_brokers = service_offerings.map(&:service_broker).uniq
hash[:included][:service_brokers] = service_brokers.sort_by(&:created_at).map do |broker|
hash[:included][:service_brokers] = service_brokers.sort_by { |sb| [sb.created_at, sb.guid] }.map do |broker|
broker_view = {}
broker_view[:name] = broker.name if @fields.include?('name')
broker_view[:guid] = broker.guid if @fields.include?('guid')
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_binding_app_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def apps(bindings)
app_guids = bindings.pluck(:app_guid).compact.uniq
return if app_guids.empty?

AppModel.where(guid: app_guids).order(:created_at).
AppModel.where(guid: app_guids).order(:created_at, :guid).
eager(Presenters::V3::AppPresenter.associated_resources).all
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_binding_route_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def match?(include_params)

def routes(route_bindings)
route_ids = route_bindings.map(&:route_id)
routes = Route.where(id: route_ids).order_by(:created_at).
routes = Route.where(id: route_ids).order_by(:created_at, :guid).
eager(Presenters::V3::RoutePresenter.associated_resources).all
routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash }
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def decorate(hash, bindings)
private

def service_instances(bindings)
ServiceInstance.where(guid: bindings.map(&:service_instance_guid).uniq).order(:created_at).
ServiceInstance.where(guid: bindings.map(&:service_instance_guid).uniq).order(:created_at, :guid).
eager(Presenters::V3::ServiceInstancePresenter.associated_resources).all
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_organization_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def match?(include)
def decorate(hash, resources)
hash[:included] ||= {}
organization_ids = resources.map { |r| r.space.organization_id }.uniq
organizations = Organization.where(id: organization_ids).order(:created_at).
organizations = Organization.where(id: organization_ids).order(:created_at, :guid).
eager(Presenters::V3::OrganizationPresenter.associated_resources).all

hash[:included][:organizations] = organizations.map { |organization| Presenters::V3::OrganizationPresenter.new(organization).to_hash }
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_role_organization_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def decorate(hash, roles)
hash[:included] ||= {}
organization_ids = roles.reject(&:for_space?).map(&:organization_id).uniq
unless organization_ids.empty?
organizations = Organization.where(id: organization_ids).order(:created_at).
organizations = Organization.where(id: organization_ids).order(:created_at, :guid).
eager(Presenters::V3::OrganizationPresenter.associated_resources).all
end

Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_role_space_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def decorate(hash, roles)
hash[:included] ||= {}
space_ids = roles.select(&:for_space?).map(&:space_id).uniq
unless space_ids.empty?
spaces = Space.where(id: space_ids).order(:created_at).
spaces = Space.where(id: space_ids).order(:created_at, :guid).
eager(Presenters::V3::SpacePresenter.associated_resources).all
end

Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_role_user_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def match?(include_params)
def decorate(hash, roles)
hash[:included] ||= {}
user_guids = roles.map(&:user_guid).uniq
users = User.where(guid: user_guids).order(:created_at).
users = User.where(guid: user_guids).order(:created_at, :guid).
eager(Presenters::V3::UserPresenter.associated_resources).all
uaa_users = User.uaa_users_info(user_guids)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def match?(include)

def decorate(hash, service_plans)
hash[:included] ||= {}
service_offerings = Service.where(id: service_plans.map(&:service_id).uniq).order(:created_at).
service_offerings = Service.where(id: service_plans.map(&:service_id).uniq).order(:created_at, :guid).
eager(Presenters::V3::ServiceOfferingPresenter.associated_resources).all

hash[:included][:service_offerings] = service_offerings.map do |service_offering|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ def decorate(hash, service_plans)
hash[:included] ||= {}
space_ids = service_plans.map { |p| p.service.service_broker.space_id }.compact.uniq
unless space_ids.empty?
spaces = Space.where(id: space_ids).order(:created_at).
spaces = Space.where(id: space_ids).order(:created_at, :guid).
eager(Presenters::V3::SpacePresenter.associated_resources).all
orgs = Organization.where(id: spaces.map(&:organization_id).uniq).order(:created_at).
orgs = Organization.where(id: spaces.map(&:organization_id).uniq).order(:created_at, :guid).
eager(Presenters::V3::OrganizationPresenter.associated_resources).all
end

Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_space_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def match?(include)
def decorate(hash, resources)
hash[:included] ||= {}
space_guids = resources.map(&:space_guid).uniq
spaces_query = Space.where(guid: space_guids).order(:created_at).
spaces_query = Space.where(guid: space_guids).order(:created_at, :guid).
eager(Presenters::V3::SpacePresenter.associated_resources)
spaces_query = with_readable_space_guids(spaces_query)

Expand Down
2 changes: 1 addition & 1 deletion app/decorators/include_space_organization_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def match?(include)
def decorate(hash, spaces)
hash[:included] ||= {}
organization_ids = spaces.map(&:organization_id).uniq
organizations = Organization.where(id: organization_ids).order(:created_at).
organizations = Organization.where(id: organization_ids).order(:created_at, :guid).
eager(Presenters::V3::OrganizationPresenter.associated_resources).all

hash[:included][:organizations] = organizations.map { |organization| Presenters::V3::OrganizationPresenter.new(organization).to_hash }
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/runtime/prune_completed_builds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def perform
builds_dataset = BuildModel.where(app_guid:)

builds_to_keep = builds_dataset.
order(Sequel.desc(:created_at)).
order(Sequel.desc(:created_at), Sequel.desc(:id)).
limit(max_retained_builds_per_app).
select(:id)

Expand Down
2 changes: 1 addition & 1 deletion app/jobs/runtime/prune_completed_deployments.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def perform
deployments_dataset = DeploymentModel.where(app_guid:)

deployments_to_keep = deployments_dataset.
order(Sequel.desc(:created_at)).
order(Sequel.desc(:created_at), Sequel.desc(:id)).
limit(max_retained_deployments_per_app).
select(:id)

Expand Down
2 changes: 1 addition & 1 deletion app/jobs/runtime/prune_excess_app_revisions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def perform
revision_dataset = RevisionModel.where(app_guid:)
next if revision_dataset.count <= max_retained_revisions_per_app

revisions_to_keep = revision_dataset.order(Sequel.desc(:created_at)).
revisions_to_keep = revision_dataset.order(Sequel.desc(:created_at), Sequel.desc(:id)).
limit(max_retained_revisions_per_app).
select(:id)

Expand Down
2 changes: 1 addition & 1 deletion app/presenters/v3/route_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require 'presenters/v3/base_presenter'
require 'presenters/v3/route_destination_presenter'
require 'presenters/v3/route_destinations_presenter'
require 'presenters/mixins/metadata_presentation_helpers'
require 'presenters/helpers/censorship'

Expand Down
2 changes: 1 addition & 1 deletion lib/cloud_controller/bits_expiration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def enqueue_package_delete_job(package_guid)
end

def filter_non_expirable(dataset, storage_count)
data_to_keep = dataset.order_by(Sequel.desc(:created_at)).limit(storage_count).select(:id)
data_to_keep = dataset.order_by(Sequel.desc(:created_at), Sequel.desc(:id)).limit(storage_count).select(:id)
dataset.exclude(id: data_to_keep)
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cloud_controller/deployment_updater/updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def deploying_web_process
end

def oldest_web_process_with_instances
@oldest_web_process_with_instances ||= app.web_processes.select { |process| process.instances > 0 }.min_by(&:created_at)
@oldest_web_process_with_instances ||= app.web_processes.select { |process| process.instances > 0 }.min_by { |p| [p.created_at, p.id] }
end

def interim_web_process
Expand Down
6 changes: 3 additions & 3 deletions spec/request/roles_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
RSpec.describe 'Roles Request' do
let(:user) { VCAP::CloudController::User.make(guid: 'user_guid') }
let(:admin_header) { admin_headers_for(user) }
let(:org) { VCAP::CloudController::Organization.make(guid: 'big-org') }
let(:space) { VCAP::CloudController::Space.make(guid: 'big-space', organization: org) }
let(:org) { VCAP::CloudController::Organization.make(guid: 'big-org', created_at: Time.now.utc - 1.second) }
let(:space) { VCAP::CloudController::Space.make(guid: 'big-space', organization: org, created_at: Time.now.utc - 1.second) }
let(:user_with_role) { VCAP::CloudController::User.make(guid: 'user_with_role') }
let(:user_guid) { user.guid }
let(:space_guid) { space.guid }
Expand Down Expand Up @@ -822,7 +822,7 @@

describe 'GET /v3/roles' do
let(:api_call) { ->(user_headers) { get '/v3/roles', nil, user_headers } }
let(:other_user) { VCAP::CloudController::User.make(guid: 'other-user-guid') }
let(:other_user) { VCAP::CloudController::User.make(guid: 'other-user-guid', created_at: Time.now.utc - 1.second) }

let!(:space_auditor) do
VCAP::CloudController::SpaceAuditor.make(
Expand Down
10 changes: 7 additions & 3 deletions spec/request/service_credential_bindings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@
end

context 'given a mixture of bindings' do
let(:now) { Time.now }
let(:instance) { VCAP::CloudController::ManagedServiceInstance.make(space:) }
let(:now) { Time.now.utc }
let(:instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space, created_at: now - 1.second) }
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!(:app_binding) do
VCAP::CloudController::ServiceBinding.make(service_instance: instance, created_at: now - 2.seconds).tap do |binding|
VCAP::CloudController::ServiceBinding.make(
app: VCAP::CloudController::AppModel.make(space: space, created_at: now - 1.second),
service_instance: instance,
created_at: now - 2.seconds
).tap do |binding|
operate_on(binding)
end
end
Expand Down
30 changes: 26 additions & 4 deletions spec/request/service_instances_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

RSpec.describe 'V3 service instances' do
let(:user) { VCAP::CloudController::User.make }
let(:org) { VCAP::CloudController::Organization.make }
let(:org) { VCAP::CloudController::Organization.make(created_at: Time.now.utc - 1.second) }
let!(:org_annotation) { VCAP::CloudController::OrganizationAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'foo', value: 'bar', resource_guid: org.guid) }
let(:space) { VCAP::CloudController::Space.make(organization: org) }
let(:space) { VCAP::CloudController::Space.make(organization: org, created_at: Time.now.utc - 1.second) }
let!(:space_annotation) { VCAP::CloudController::SpaceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'baz', value: 'wow', space: space) }
let(:another_space) { VCAP::CloudController::Space.make }

Expand Down Expand Up @@ -189,8 +189,30 @@
end

context 'given a mixture of managed, user-provided and shared service instances' do
let!(:msi_1) { VCAP::CloudController::ManagedServiceInstance.make(space:) }
let!(:msi_2) { VCAP::CloudController::ManagedServiceInstance.make(space: another_space) }
let!(:msi_1) do
VCAP::CloudController::ManagedServiceInstance.make(
service_plan: VCAP::CloudController::ServicePlan.make(
service: VCAP::CloudController::Service.make(
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 2.seconds),
created_at: Time.now.utc - 2.seconds
),
created_at: Time.now.utc - 2.seconds
),
space: space
)
end
let!(:msi_2) do
VCAP::CloudController::ManagedServiceInstance.make(
service_plan: VCAP::CloudController::ServicePlan.make(
service: VCAP::CloudController::Service.make(
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 1.second),
created_at: Time.now.utc - 1.second
),
created_at: Time.now.utc - 1.second
),
space: another_space
)
end
let!(:upsi_1) { VCAP::CloudController::UserProvidedServiceInstance.make(space:) }
let!(:upsi_2) { VCAP::CloudController::UserProvidedServiceInstance.make(space: another_space) }
let!(:ssi) { VCAP::CloudController::ManagedServiceInstance.make(space: another_space) }
Expand Down
6 changes: 5 additions & 1 deletion spec/request/service_offerings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,11 @@
end

describe 'fields' do
let!(:service_1) { VCAP::CloudController::Service.make }
let!(:service_1) do
VCAP::CloudController::Service.make(
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 1.second)
)
end
let!(:service_2) { VCAP::CloudController::Service.make }

it 'can include service broker name and guid' do
Expand Down
15 changes: 13 additions & 2 deletions spec/request/service_plans_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,12 @@
end

describe 'includes' do
let(:space_1) { VCAP::CloudController::Space.make }
let(:space_1) do
VCAP::CloudController::Space.make(
organization: VCAP::CloudController::Organization.make(created_at: Time.now.utc - 1.second),
created_at: Time.now.utc - 1.second
)
end
let(:space_2) { VCAP::CloudController::Space.make }

context 'when including `space.organization`' do
Expand Down Expand Up @@ -608,7 +613,13 @@
end

describe 'fields' do
let!(:plan_1) { VCAP::CloudController::ServicePlan.make }
let!(:plan_1) do
VCAP::CloudController::ServicePlan.make(
service: VCAP::CloudController::Service.make(
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 1.second)
)
)
end
let!(:plan_2) { VCAP::CloudController::ServicePlan.make }

it 'can include service broker name and guid' do
Expand Down
4 changes: 2 additions & 2 deletions spec/request/service_route_bindings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@

describe 'include' do
context 'when including `service_instance`' do
let(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(:routing) }
let(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(:routing, created_at: Time.now.utc - 1.second) }
let(:other_instance) { VCAP::CloudController::UserProvidedServiceInstance.make(:routing) }

before do
Expand Down Expand Up @@ -244,7 +244,7 @@
end

it 'can include `route`' do
route = VCAP::CloudController::Route.make
route = VCAP::CloudController::Route.make(created_at: Time.now.utc - 1.second)
other_route = VCAP::CloudController::Route.make

si = VCAP::CloudController::ManagedServiceInstance.make(:routing, space: route.space)
Expand Down
6 changes: 3 additions & 3 deletions spec/unit/actions/revision_create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ module VCAP::CloudController
)
end.to change { RevisionModel.where(app:).count }.by(1)

expect(RevisionModel.order_by(:created_at).map(&:version)).to eq([1, 2])
expect(RevisionModel.order_by(:created_at, :id).map(&:version)).to eq([1, 2])
end

it 'rolls over to version 1 when we pass version 9999' do
Expand All @@ -130,7 +130,7 @@ module VCAP::CloudController
commands_by_process_type: { 'web' => 'run my app' },
user_audit_info: user_audit_info
)
expect(RevisionModel.order_by(:created_at).map(&:version)).to eq([2, 9998, 9999, 1])
expect(RevisionModel.order_by(:created_at, :id).map(&:version)).to eq([2, 9998, 9999, 1])
end

it 'replaces any existing revisions after rolling over' do
Expand All @@ -148,7 +148,7 @@ module VCAP::CloudController
commands_by_process_type: { 'web' => 'run my app' },
user_audit_info: user_audit_info
)
expect(RevisionModel.order_by(:created_at).map(&:version)).to eq([9998, 9999, 1, 2])
expect(RevisionModel.order_by(:created_at, :id).map(&:version)).to eq([9998, 9999, 1, 2])
end
end
end
Expand Down
Loading