Add sui_product_features method to miq_group #17007
Merged
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
The SUI needs the ability to see what SUI product features are available to the user before changing groups.
|
Checked commit jntullo@08373ae with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
LGTM!! Thanks @jntullo 👍 |
abellotti
approved these changes
Feb 16, 2018
NickLaMuro
added a commit
to NickLaMuro/manageiq
that referenced
this pull request
Mar 24, 2018
This prevents a large amount (over 200) of CACHE statements from showing up in the rails DEBUG logs for the `/api` endpoint. More detailed explanation of this issue below. * * * Added in ManageIQ#17007 This feature calls the newly created `MiqGroup.sui_product_features`, which is a `virtual_has_one`, and makes use of `MiqUserRole.allow?` for every feature being analyzed. First, once, to fetch all of the feature children for "sui", and they once for every one of those "sui" child features. The code where that happens: return [] unless miq_user_role.allows?(:identifier => 'sui') MiqProductFeature.feature_all_children('sui').each_with_object([]) do |sui_feature, sui_features| sui_features << sui_feature if miq_user_role.allows?(:identifier => sui_feature) end The code for `MiqUserRole#allows?` calls `feature_identifiers`, which is just a call to `miq_product_features.pluck(:identifier)`. The problem with this is that since `.pluck` is called, there is no model caching that would normally be done with the `miq_product_features` relation on `MiqUserRole`. In a Rails request, this will always be cached, so this isn't too big of a deal, though under the hood, there is a decent amount of object `.dup` calls that end up happening. But outside of it's use in a request, this might not be cached, so it actually performs the `.pluck` queries every time. Prior to the change in this commit, this can be simulated by doing the following: User.where(:userid => "admin").first .miq_groups.first .sui_product_features Caching basically prevents performing that operation more than once. The only concern with this is if we had code paths in `manageiq` that relied on fresh permissions of the `MiqUserRole` instance every time we checked, but that was also recently changed at the time of this commit: ManageIQ#17008 And the `TODO` had been there for 2 years, so I think we are good ;) Of note about the previous version of MiqUserRole#feature_identifiers: def feature_identifiers miq_product_features.collect(&:identifier) end While the `pluck` version avoids the `.collect` call every time this is called, the `miq_product_features` are cached, so it is just doing a loop over them every time and would have avoided the CACHE statements that this commit fixes. That said, this version should be the best of both worlds since it will be less CPU cycles (since there isn't a `.collect` for each call of this method) and memory allocations overall (since we aren't doing a `.dup` under the hood in `ActiveRecord::QueryCache`).
NickLaMuro
added a commit
to NickLaMuro/manageiq
that referenced
this pull request
Mar 24, 2018
This prevents a large amount (over 200) of CACHE statements from showing
up in the rails DEBUG logs for the `/api` endpoint. More detailed
explanation of this issue below.
* * *
Added in the following:
ManageIQ/manageiq-api#311
ManageIQ#17007
This feature calls the newly created `MiqGroup.sui_product_features`,
which is a `virtual_has_one`, and makes use of `MiqUserRole.allow?` for
every feature being analyzed. First, once, to fetch all of the feature
children for "sui", and they once for every one of those "sui" child
features. The code where that happens:
return [] unless miq_user_role.allows?(:identifier => 'sui')
MiqProductFeature.feature_all_children('sui').each_with_object([]) do |sui_feature, sui_features|
sui_features << sui_feature if miq_user_role.allows?(:identifier => sui_feature)
end
The code for `MiqUserRole#allows?` calls `feature_identifiers`, which
is just a call to `miq_product_features.pluck(:identifier)`. The
problem with this is that since `.pluck` is called, there is no model
caching that would normally be done with the `miq_product_features`
relation on `MiqUserRole`.
In a Rails request, this will always be cached, so this isn't too big of
a deal, though under the hood, there is a decent amount of object `.dup`
calls that end up happening. But outside of it's use in a request, this
might not be cached, so it actually performs the `.pluck` queries every
time. Prior to the change in this commit, this can be simulated by
doing the following:
User.where(:userid => "admin").first
.miq_groups.first
.sui_product_features
Adding caching here basically prevents performing that operation from
happening more than once.
The only concern with this is if we had code paths in `manageiq` that
relied on fresh permissions of the `MiqUserRole` instance every time we
checked, but that was also recently changed at the time of this commit:
ManageIQ#17008
And the `TODO` had been there for 2 years, so I think we are good ;)
Of note about the previous version of MiqUserRole#feature_identifiers:
def feature_identifiers
miq_product_features.collect(&:identifier)
end
While the `pluck` version avoids the `.collect` call every time this is
called, the `miq_product_features` are cached, so it is just doing a
loop over them every time and would have avoided the CACHE statements
that this commit fixes. That said, this version should be the best of
both worlds since it will be less CPU cycles (since there isn't a
`.collect` for each call of this method) and memory allocations
overall (since we aren't doing a `.dup` under the hood in
`ActiveRecord::QueryCache`).
JakubKubista
pushed a commit
to JakubKubista/manageiq
that referenced
this pull request
Mar 29, 2018
This prevents a large amount (over 200) of CACHE statements from showing
up in the rails DEBUG logs for the `/api` endpoint. More detailed
explanation of this issue below.
* * *
Added in the following:
ManageIQ/manageiq-api#311
ManageIQ#17007
This feature calls the newly created `MiqGroup.sui_product_features`,
which is a `virtual_has_one`, and makes use of `MiqUserRole.allow?` for
every feature being analyzed. First, once, to fetch all of the feature
children for "sui", and they once for every one of those "sui" child
features. The code where that happens:
return [] unless miq_user_role.allows?(:identifier => 'sui')
MiqProductFeature.feature_all_children('sui').each_with_object([]) do |sui_feature, sui_features|
sui_features << sui_feature if miq_user_role.allows?(:identifier => sui_feature)
end
The code for `MiqUserRole#allows?` calls `feature_identifiers`, which
is just a call to `miq_product_features.pluck(:identifier)`. The
problem with this is that since `.pluck` is called, there is no model
caching that would normally be done with the `miq_product_features`
relation on `MiqUserRole`.
In a Rails request, this will always be cached, so this isn't too big of
a deal, though under the hood, there is a decent amount of object `.dup`
calls that end up happening. But outside of it's use in a request, this
might not be cached, so it actually performs the `.pluck` queries every
time. Prior to the change in this commit, this can be simulated by
doing the following:
User.where(:userid => "admin").first
.miq_groups.first
.sui_product_features
Adding caching here basically prevents performing that operation from
happening more than once.
The only concern with this is if we had code paths in `manageiq` that
relied on fresh permissions of the `MiqUserRole` instance every time we
checked, but that was also recently changed at the time of this commit:
ManageIQ#17008
And the `TODO` had been there for 2 years, so I think we are good ;)
Of note about the previous version of MiqUserRole#feature_identifiers:
def feature_identifiers
miq_product_features.collect(&:identifier)
end
While the `pluck` version avoids the `.collect` call every time this is
called, the `miq_product_features` are cached, so it is just doing a
loop over them every time and would have avoided the CACHE statements
that this commit fixes. That said, this version should be the best of
both worlds since it will be less CPU cycles (since there isn't a
`.collect` for each call of this method) and memory allocations
overall (since we aren't doing a `.dup` under the hood in
`ActiveRecord::QueryCache`).
JakubKubista
pushed a commit
to JakubKubista/manageiq
that referenced
this pull request
Apr 12, 2018
This prevents a large amount (over 200) of CACHE statements from showing
up in the rails DEBUG logs for the `/api` endpoint. More detailed
explanation of this issue below.
* * *
Added in the following:
ManageIQ/manageiq-api#311
ManageIQ#17007
This feature calls the newly created `MiqGroup.sui_product_features`,
which is a `virtual_has_one`, and makes use of `MiqUserRole.allow?` for
every feature being analyzed. First, once, to fetch all of the feature
children for "sui", and they once for every one of those "sui" child
features. The code where that happens:
return [] unless miq_user_role.allows?(:identifier => 'sui')
MiqProductFeature.feature_all_children('sui').each_with_object([]) do |sui_feature, sui_features|
sui_features << sui_feature if miq_user_role.allows?(:identifier => sui_feature)
end
The code for `MiqUserRole#allows?` calls `feature_identifiers`, which
is just a call to `miq_product_features.pluck(:identifier)`. The
problem with this is that since `.pluck` is called, there is no model
caching that would normally be done with the `miq_product_features`
relation on `MiqUserRole`.
In a Rails request, this will always be cached, so this isn't too big of
a deal, though under the hood, there is a decent amount of object `.dup`
calls that end up happening. But outside of it's use in a request, this
might not be cached, so it actually performs the `.pluck` queries every
time. Prior to the change in this commit, this can be simulated by
doing the following:
User.where(:userid => "admin").first
.miq_groups.first
.sui_product_features
Adding caching here basically prevents performing that operation from
happening more than once.
The only concern with this is if we had code paths in `manageiq` that
relied on fresh permissions of the `MiqUserRole` instance every time we
checked, but that was also recently changed at the time of this commit:
ManageIQ#17008
And the `TODO` had been there for 2 years, so I think we are good ;)
Of note about the previous version of MiqUserRole#feature_identifiers:
def feature_identifiers
miq_product_features.collect(&:identifier)
end
While the `pluck` version avoids the `.collect` call every time this is
called, the `miq_product_features` are cached, so it is just doing a
loop over them every time and would have avoided the CACHE statements
that this commit fixes. That said, this version should be the best of
both worlds since it will be less CPU cycles (since there isn't a
`.collect` for each call of this method) and memory allocations
overall (since we aren't doing a `.dup` under the hood in
`ActiveRecord::QueryCache`).
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.
The SUI needs the ability to see what SUI product features are available to the user before changing groups. (Needed for ManageIQ/manageiq-api#311 / https://bugzilla.redhat.com/show_bug.cgi?id=1540733). This adds it as a virtual attribute.
Returning all product features can result in very large payloads (2MB) depending on how many groups a user belongs to. This reduces that size by only returning the feature identifiers specifically for the SUI.
@miq-bot assign @gtanzillo
cc: @abellotti