Skip to content

Replace .include? / .map with more efficient SQL statements #3041

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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/access/space_quota_definition_access.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def delete?(space_quota_definition, _params=nil)
def read?(space_quota_definition, *_)
context.admin_override || (
!context.user.nil? && (
(context.user.managed_organizations.include? space_quota_definition.organization) ||
context.user.managed_organizations_dataset.where(id: space_quota_definition.organization_id).any? ||
!(context.user.managed_spaces & space_quota_definition.spaces).empty? ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely an improvement, but you can go further and cut out organizations table by doing something like

OrganizationManager.where(user_id: context.user.id, organization_id: space_quota_definition.organization.id).any?

Copy link
Contributor

Choose a reason for hiding this comment

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

managed organizations_dataset produces this query:

SELECT  "organizations".*
FROM    "organizations"
       INNER JOIN  "organizations_managers"
               ON (  "organizations_managers" . "organization_id" = 
                    "organizations" . "id" )
WHERE  (  "organizations_managers" . "user_id" = 118 ) 

!(context.user.audited_spaces & space_quota_definition.spaces).empty? ||
!(context.user.spaces & space_quota_definition.spaces).empty?
Expand Down
4 changes: 2 additions & 2 deletions app/actions/process_create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def create(app, args)
process = nil
app.class.db.transaction do
process = app.add_process(attrs)
route_mappings = process.route_mappings
process.update(ports: route_mappings.map(&:app_port)) unless route_mappings.empty?
app_ports = process.route_mappings_dataset.select_map(:app_port)
process.update(ports: app_ports) unless app_ports.empty?
Repositories::ProcessEventRepository.record_create(process, @user_audit_info, manifest_triggered: @manifest_triggered)
end

Expand Down
4 changes: 2 additions & 2 deletions app/actions/service_instance_share.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ def validate_plan_visibility!(service_instance, space)
end

def validate_name_uniqueness!(service_instance, space)
if space.service_instances.map(&:name).include?(service_instance.name)
if space.service_instances_dataset.where(name: service_instance.name).any?
error_msg = "A service instance called #{service_instance.name} already exists in #{space.name}."
error!(error_msg)
end

return unless space.service_instances_shared_from_other_spaces.map(&:name).include?(service_instance.name)
return unless space.service_instances_shared_from_other_spaces_dataset.where(name: service_instance.name).any?

error_msg = "A service instance called #{service_instance.name} has already been shared with #{space.name}."
error!(error_msg)
Expand Down
2 changes: 1 addition & 1 deletion app/actions/v3/service_instance_delete.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def remove_associations

def unshare_all_spaces
# The array from `service_instance.shared_spaces` gets updated as spaces are unshared, so we make list of guids
space_guids = service_instance.shared_spaces.map(&:guid)
space_guids = service_instance.shared_spaces_dataset.select_map(:guid)

unshare_action = ServiceInstanceUnshare.new
space_guids.each_with_object([]) do |space_guid, errors|
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/v3/roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def destroy

if role.type == VCAP::CloudController::RoleTypes::ORGANIZATION_USER
org = Organization.find(id: role.organization_id)
no_space_role = Role.where(space_id: org.spaces.map(&:id), user_id: role.user_id).empty?
no_space_role = Role.where(space_id: org.spaces_dataset.select(:id), user_id: role.user_id).empty?
unprocessable!('Cannot delete organization_user role while user has roles in spaces in that organization.') unless no_space_role
end
end
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/v3/security_groups_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def delete_running_spaces
unprocessable_space! unless space
unauthorized! unless permission_queryer.can_update_active_space?(space.id, space.organization_id)
suspended! unless permission_queryer.is_space_active?(space.id)
unprocessable_space! unless security_group.spaces.include?(space)
unprocessable_space! unless security_group.spaces_dataset.where(id: space.id).any?

SecurityGroupUnapply.unapply_running(security_group, space)

Expand All @@ -140,7 +140,7 @@ def delete_staging_spaces
unprocessable_space! unless space
unauthorized! unless permission_queryer.can_update_active_space?(space.id, space.organization_id)
suspended! unless permission_queryer.is_space_active?(space.id)
unprocessable_space! unless security_group.staging_spaces.include?(space)
unprocessable_space! unless security_group.staging_spaces_dataset.where(id: space.id).any?

SecurityGroupUnapply.unapply_staging(security_group, space)

Expand Down
15 changes: 11 additions & 4 deletions app/decorators/field_service_instance_broker_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,18 @@ def decorate(hash, service_instances)
managed_service_instances = service_instances.select(&:managed_instance?)
return hash if managed_service_instances.empty?

hash[:included] ||= {}
plans = managed_service_instances.map(&:service_plan).uniq
brokers = plans.map(&:service_broker).uniq
brokers = ServiceBroker.
join(:services, service_broker_id: :service_brokers__id).
join(:service_plans, service_id: :services__id).
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).
select(:service_brokers__name, :service_brokers__guid).
all

hash[:included][:service_brokers] = brokers.sort_by(&:created_at).map do |broker|
hash[:included] ||= {}
hash[:included][:service_brokers] = brokers.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
16 changes: 10 additions & 6 deletions app/decorators/field_service_instance_offering_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,21 @@ def initialize(fields)
@fields = fields[:'service_plan.service_offering'].to_set.intersection(self.class.allowed)
end

# rubocop:todo Metrics/CyclomaticComplexity
def decorate(hash, service_instances)
managed_service_instances = service_instances.select(&:managed_instance?)
return hash if managed_service_instances.empty?

hash[:included] ||= {}
plans = managed_service_instances.map(&:service_plan).uniq
offerings = plans.map(&:service).uniq
offerings = Service.
join(:service_plans, service_id: :services__id).
join(:service_instances, service_plan_id: :service_plans__id).
where(service_instances__id: managed_service_instances.map(&:id)).
distinct.
order_by(:services__created_at).
select(:services__label, :services__guid, :services__description, :services__tags, :services__extra, :services__service_broker_id).
all

hash[:included][:service_offerings] = offerings.sort_by(&:created_at).map do |offering|
hash[:included] ||= {}
hash[:included][:service_offerings] = offerings.map do |offering|
offering_view = {}
offering_view[:name] = offering.name if @fields.include?('name')
offering_view[:guid] = offering.guid if @fields.include?('guid')
Expand All @@ -43,7 +48,6 @@ def decorate(hash, service_instances)

hash
end
# rubocop:enable Metrics/CyclomaticComplexity

private

Expand Down
2 changes: 1 addition & 1 deletion app/fetchers/droplet_list_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def filter(message, app, space_guids, dataset)
droplet_table_name = DropletModel.table_name

if message.requested?(:organization_guids)
space_guids_from_orgs = Organization.where(guid: message.organization_guids).map(&:spaces).flatten.map(&:guid)
space_guids_from_orgs = Spaces.join(:organizations, id: :organization_id).where(organization__guid: message.organization_guids).select(:guid)
dataset = dataset.select_all(droplet_table_name).
join_table(:inner, AppModel.table_name, { guid: Sequel[:droplets][:app_guid], space_guid: space_guids_from_orgs }, { table_alias: :apps_orgs })
end
Expand Down
5 changes: 4 additions & 1 deletion app/fetchers/route_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ def filter(message, dataset)
dataset = dataset.where(port: message.ports) if message.requested?(:ports)

if message.requested?(:organization_guids)
space_ids = Organization.where(guid: message.organization_guids).map(&:spaces).flatten.map(&:id)
space_ids = Space.
join(:organizations, id: :organization_id).
where(organizations__guid: message.organization_guids).
select(:spaces__id)
dataset = dataset.where(space_id: space_ids)
end

Expand Down
2 changes: 1 addition & 1 deletion app/fetchers/space_quota_list_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def filter(message, dataset, readable_space_quota_guids)
dataset = dataset.where(name: message.names) if message.requested? :names

if message.requested? :organization_guids
org_ids = Organization.where(guid: message.organization_guids).map(:id)
org_ids = Organization.where(guid: message.organization_guids).select(:id)
dataset = dataset.where(organization_id: org_ids)
end

Expand Down
3 changes: 2 additions & 1 deletion app/fetchers/task_list_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ def filter(message)
def filter_app_dataset(message, app_dataset)
app_dataset = app_dataset.where(space_guid: message.space_guids) if message.requested?(:space_guids)
if message.requested?(:organization_guids)
app_dataset = app_dataset.where(space_guid: Organization.where(guid: message.organization_guids).map(&:spaces).flatten.map(&:guid))
space_guids_from_orgs = Spaces.join(:organizations, id: :organization_id).where(organization__guid: message.organization_guids).select(:guid)
app_dataset = app_dataset.where(space_guid: space_guids_from_orgs)
end
app_dataset = app_dataset.where(guid: message.app_guids) if message.requested?(:app_guids)
app_dataset
Expand Down
2 changes: 1 addition & 1 deletion app/models/runtime/organization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ def billing_enabled?
end

def isolation_segment_guids
isolation_segment_models.map(&:guid)
isolation_segment_models_dataset.select_map(:guid)
end

def has_user?(user)
Expand Down
2 changes: 1 addition & 1 deletion app/models/runtime/process_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ def stopped?
end

def uris
routes.map(&:uri)
routes_dataset.select_map(:uri)
end

def buildpack
Expand Down
6 changes: 4 additions & 2 deletions app/models/runtime/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,15 @@ def validate
end

def validate_organization(org)
return if org && organizations.include?(org)
return if org && organizations_dataset.where(id: org.id).any?

raise InvalidOrganizationRelation.new("Cannot add role, user does not belong to Organization with guid #{org.guid}")
end

def validate_organization_roles(org)
return unless org && (managed_organizations.include?(org) || billing_managed_organizations.include?(org) || audited_organizations.include?(org))
return unless org && (managed_organizations_dataset.where(id: org.id).any? ||
billing_managed_organizations_dataset.where(id: org.id).any? ||
audited_organizations_dataset.where(id: org.id).any?)

raise InvalidOrganizationRelation.new("Cannot remove user from Organization with guid #{org.guid} if the user has the OrgManager, BillingManager, or Auditor role")
end
Expand Down
8 changes: 4 additions & 4 deletions app/models/services/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ class Service < Sequel::Model

class << self
def public_visible
public_active_plans = ServicePlan.where(active: true, public: true).all
service_ids = public_active_plans.map(&:service_id).uniq
dataset.filter(id: service_ids)
public_active_plans = ServicePlan.where(active: true, public: true)

dataset.filter(id: public_active_plans.select(:service_id).distinct)
end

def user_visibility_filter(current_user, operation=nil)
Expand All @@ -45,7 +45,7 @@ def user_visibility_for_read(current_user, _admin_override)
end

def unauthenticated_visibility_filter
{ id: public_visible.map(&:id) }
{ id: public_visible.select(:id) }
end

def space_or_org_visible_for_user(space, user)
Expand Down
7 changes: 3 additions & 4 deletions app/models/services/service_broker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,9 @@ def space_scoped?
end

def has_service_instances?
VCAP::CloudController::ServiceInstance.
join(:service_plans, id: :service_plan_id).
join(:services, id: :service_id).
where(services__service_broker_id: id).
services_dataset.
join(:service_plans, service_id: :services__id).
join(:service_instances, service_plan_id: :service_plans__id).
any?
end

Expand Down
18 changes: 9 additions & 9 deletions app/presenters/v3/security_group_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ def to_hash
},
relationships: {
running_spaces: {
data: space_guid_hash_for(security_group.spaces)
data: space_guid_hash_for(security_group.spaces_dataset)
},
staging_spaces: {
data: space_guid_hash_for(security_group.staging_spaces)
data: space_guid_hash_for(security_group.staging_spaces_dataset)
}
},
links: build_links
Expand All @@ -43,13 +43,13 @@ def security_group
@resource
end

def space_guid_hash_for(spaces)
visible_spaces = if @all_spaces_visible
spaces
else
spaces.select { |space| @visible_space_guids.include? space.guid }
end
visible_spaces.map { |space| { guid: space.guid } }
def space_guid_hash_for(spaces_dataset)
visible_spaces_dataset = if @all_spaces_visible
spaces_dataset
else
spaces_dataset.where(guid: @visible_space_guids)
end
visible_spaces_dataset.select_map(:guid).map { |guid| { guid: guid } }
end

def build_links
Expand Down
12 changes: 6 additions & 6 deletions app/presenters/v3/space_quota_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ def space_quota
end

def filtered_visible_spaces
visible_spaces = if @all_spaces_visible
space_quota.spaces
else
space_quota.spaces.select { |space| @visible_space_guids.include? space.guid }
end
visible_spaces.map { |space| { guid: space.guid } }
visible_spaces_dataset = if @all_spaces_visible
space_quota.spaces_dataset
else
space_quota.spaces_dataset.where(guid: @visible_space_guids)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately @visible_space_guids comes from permissions.readable_space_guids but maybe we should instead that should be permissions.readable_space_guids_query (though we changed that recently to raise an error if the user reads globally..)

end
visible_spaces_dataset.select_map(:guid).map { |guid| { guid: guid } }
end

def build_links
Expand Down
4 changes: 2 additions & 2 deletions lib/cloud_controller/permissions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,8 @@ def can_read_route?(space_id)
space = VCAP::CloudController::Space.where(id: space_id).first

space.has_member?(@user) || space.has_supporter?(@user) ||
@user.managed_organizations.map(&:id).include?(space.organization_id) ||
@user.audited_organizations.map(&:id).include?(space.organization_id)
@user.managed_organizations_dataset.where(id: space.organization_id).any? ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my first comment - the OrganizationManager has user_id and organization_id columns, so if you use that you can cut organizations out entirely.

@user.audited_organizations_dataset.where(id: space.organization_id).any?
end

def space_guids_with_readable_routes_query
Expand Down