Return product features for all of a user's groups on the API entrypoint #311
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
AllenBW
approved these changes
Feb 1, 2018
jntullo
changed the title
|
the pre/post output size of the entrypoint goes from 11K to 250K or so. This should be requested optionally. We currently do provide the information for the user's current group via the authorization entry returned when we have a GET /api?attributes=authorization. I think we should only return the group's product features if attributes=authorization is specified. One other concern is where we show this. Here we're merging "product_features" as part of the group info which is not an actual attribute/virtual, it's really group.miq_user_role.miq_product_features. might cause confusion (folks expecting to be able do GET /api/groups/:id?attributes=product_features). We can extend the "authorization" hash to include the per group info, or maybe instead of merging "product_features" in the group, we can call it "miq_product_features" and provide that in the model in groups (has_one :miq_product_features, :through => :miq_user_role), and make it virtual so folks can do a GET /api/groups/:id?attributes=miq_product_features. |
This was referenced Feb 15, 2018
jntullo
force-pushed
the
return_product_features_all_groups
branch
2 times, most recently
from
df6a06f to
8293086
Compare
…rypoint This is needed so that the users can know what features they will have before changing groups. When they are a part of multiple groups, the current way of getting groups /api/groups may not be available to them based off of their current groups permissions. This will fix that.
jntullo
force-pushed
the
return_product_features_all_groups
branch
from
8293086 to
402a304
Compare
|
Checked commit jntullo@402a304 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Thanks @jntullo for the enhancement. LGTM!! 👍 |
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 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.
This is needed so that the users can know what features they will have before changing groups. When they are a part of multiple groups, the current way of getting groups
/api/groups?attributes=miq_product_featuresmay not be available to them based off of their current groups permissions. This will allow them to see them.It also refactors a bit as was necessary to use memoization and not have to create the product features for the current group multiple times.
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1540733
cc: @AllenBW