Remove unnecessary db query as admin user for update_route_destinations #2986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Improve cases for admin user in
(lib/cloud_controller/permissions.rb: `readable_app_guids')
and (app/controllers/v3/routes_controller.rb: `validate_app_guids!')
to avoid unnecessary SELECT statements without condition ("SELECT guid FROM spaces",
"SELECT guid FROM apps")
- Adapting the route_destinations_spec to the changes.
Signed-off-by: Philipp Thun <[email protected]>
philippthun
approved these changes
Sep 28, 2022
kathap
added a commit
to sap-contributions/cloud_controller_ng
that referenced
this pull request
Oct 5, 2022
With cloudfoundry#2986, cloudfoundry#2995, cloudfoundry#2997 no case is left where readable_space_guids_query or readable_org_guids_query is called in an admin use case. Therefore the select for all guids as admin user is not needed any more. Instead raise an exception if this query is called for admin users.
5 tasks
kathap
added a commit
to sap-contributions/cloud_controller_ng
that referenced
this pull request
Oct 6, 2022
With cloudfoundry#2986, cloudfoundry#2995, cloudfoundry#2997, SELECT all guids for can_read_globally users is not called anymore, the tests for readable_space_guids and readbale_org_guids needed to be adjusted
will-gant
pushed a commit
to sap-contributions/cloud_controller_ng
that referenced
this pull request
Oct 20, 2022
With cloudfoundry#2986, cloudfoundry#2995, cloudfoundry#2997 no case is left where readable_space_guids_query or readable_org_guids_query is called in an admin use case. Therefore the select for all guids as admin user is not needed any more. Instead raise an exception if this query is called for admin users.
will-gant
pushed a commit
to sap-contributions/cloud_controller_ng
that referenced
this pull request
Dec 16, 2022
With cloudfoundry#2986, cloudfoundry#2995, cloudfoundry#2997 no case is left where readable_space_guids_query or readable_org_guids_query is called in an admin use case. Therefore the select for all guids as admin user is not needed any more. Instead raise an exception if this query is called for admin users.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
lib/cloud_controller/permissions.rb: 'readable_app_guids'andapp/controllers/v3/routes_controller.rb: 'validate_app_guids!'to avoid unnecessary SELECT statements without condition (SELECT guid FROM spaces,SELECT guid FROM apps).In one of our dashboards we saw a lot of
SELECT guid FROM spacesqueries without any filter. We found out that this query was originally triggered by readable_app_guids which used to call readable_space_guids in permissions.rb for the case when the user is an admin. To remove the unnecessary db query for admins, we replaced the readable_space_guidspart in readable_app_guids with:
VCAP::CloudController::AppModel.user_visible(@user, can_read_globally?).select(:guid).map(&:guid)which is already done similar in other functions. With this change we make use of the user_visibility_filter of the sequel_plugin vcap_user_visibility, which is possible because this filter is implemented in the app_model.The above change showed in the dashboards another db query without filter,
SELECT guid FROM apps, triggered in routes_controller.rb where readable_app_guids is called in validate_app_guids! for the case when the user is admin. In validate_app_guids we splitted the original unprocessable! case up into two scenarios:With this change the unnecessary db query for admin users can be avoided.
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