Replace .include? / .map with more efficient SQL statements #3041
Conversation
| @@ -70,7 +70,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? || | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 )
| @@ -233,8 +233,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? || | |||
There was a problem hiding this comment.
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.
app/models/runtime/user.rb
Outdated
| @@ -82,13 +82,13 @@ def validate | |||
| end | |||
|
|
|||
| def validate_organization(org) | |||
| unless org && organizations.include?(org) | |||
| unless org && organizations_dataset.where(id: org).any? | |||
There was a problem hiding this comment.
I might be misunderstanding something here - and this logic already existed before you touched it - but is it actually possible to have a situation where org is true and organizations_dataset.where(id: org).any? is false?
| visible_spaces_dataset = if @all_spaces_visible | ||
| space_quota.spaces_dataset | ||
| else | ||
| space_quota.spaces_dataset.where(guid: @visible_space_guids) |
There was a problem hiding this comment.
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..)
philippthun
force-pushed
the
efficient-include-map
branch
3 times, most recently
from
2a885c1
to
6f0f056
Compare
philippthun
force-pushed
the
efficient-include-map
branch
from
6f0f056
to
77955dd
Compare
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests