From 3648be43a1f726f4cd4a437ce67f6121834544d4 Mon Sep 17 00:00:00 2001
From: Nick LaMuro <nicklamuro@gmail.com>
Date: Fri, 23 Mar 2018 19:03:51 -0500
Subject: [PATCH] Fix 200+ SQL CACHE statements when hitting /api in logs

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:

    https://github.com/ManageIQ/manageiq-api/pull/311
    https://github.com/ManageIQ/manageiq/pull/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:

    https://github.com/ManageIQ/manageiq/pull/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`).
---
 app/models/miq_user_role.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/miq_user_role.rb b/app/models/miq_user_role.rb
index 3e22daea181..08eb8f5a3c4 100644
--- a/app/models/miq_user_role.rb
+++ b/app/models/miq_user_role.rb
@@ -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