Skip to content

Commit

Permalink
Fix 200+ SQL CACHE statements when hitting /api in logs
Browse files Browse the repository at this point in the history
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`).
  • Loading branch information
NickLaMuro committed Mar 24, 2018
1 parent 388f266 commit 3648be4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion app/models/miq_user_role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class MiqUserRole < ApplicationRecord
}

def feature_identifiers
miq_product_features.pluck(:identifier)
@feature_identifiers ||= miq_product_features.pluck(:identifier)
end

# @param identifier [String] Product feature identifier to check if this role allows access to it
Expand Down

0 comments on commit 3648be4

Please sign in to comment.