{"action":"created","issue":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/17198","repository_url":"https://api.github.com/repos/ManageIQ/manageiq","labels_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/17198/labels{/name}","comments_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/17198/comments","events_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/17198/events","html_url":"https://github.com/ManageIQ/manageiq/pull/17198","id":308214623,"number":17198,"title":"Fix 200+ SQL CACHE statements when hitting /api in logs","user":{"login":"NickLaMuro","id":314014,"avatar_url":"https://avatars0.githubusercontent.com/u/314014?v=4","gravatar_id":"","url":"https://api.github.com/users/NickLaMuro","html_url":"https://github.com/NickLaMuro","followers_url":"https://api.github.com/users/NickLaMuro/followers","following_url":"https://api.github.com/users/NickLaMuro/following{/other_user}","gists_url":"https://api.github.com/users/NickLaMuro/gists{/gist_id}","starred_url":"https://api.github.com/users/NickLaMuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NickLaMuro/subscriptions","organizations_url":"https://api.github.com/users/NickLaMuro/orgs","repos_url":"https://api.github.com/users/NickLaMuro/repos","events_url":"https://api.github.com/users/NickLaMuro/events{/privacy}","received_events_url":"https://api.github.com/users/NickLaMuro/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-03-24T00:41:54Z","updated_at":"2018-03-24T00:53:03Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/ManageIQ/manageiq/pulls/17198","html_url":"https://github.com/ManageIQ/manageiq/pull/17198","diff_url":"https://github.com/ManageIQ/manageiq/pull/17198.diff","patch_url":"https://github.com/ManageIQ/manageiq/pull/17198.patch"},"body":"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.\n\n\nLong Version (#noOneReadsMe #foreverAlone)\n------------------------------------------\n\nAdded in https://github.com/ManageIQ/manageiq/pull/17007 and https://github.com/ManageIQ/manageiq-api/pull/311\n\nThis 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:\n\n```ruby\nreturn [] unless miq_user_role.allows?(:identifier => 'sui')\nMiqProductFeature.feature_all_children('sui').each_with_object([]) do |sui_feature, sui_features|\n  sui_features << sui_feature if miq_user_role.allows?(:identifier => sui_feature)\nend\n```\n\nThe 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`.\n\nIn 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:\n\n```ruby\nUser.where(:userid => \"admin\").first\n    .miq_groups.first\n    .sui_product_features\n```\n\nAdding caching basically prevents performing that operation from happening more than once.\n\nThe 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:\n\nhttps://github.com/ManageIQ/manageiq/pull/17008\n\nAnd the `TODO` had been there for 2 years, so I think we are good ;)\n\nOf note about the previous version of `MiqUserRole#feature_identifiers`:\n\n```ruby\ndef feature_identifiers\n  miq_product_features.collect(&:identifier)\nend\n```\n\nWhile 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`).\n\n\nLinks\n-----\n\n* [Conversation that noticed the issue](https://gitter.im/ManageIQ/v2v?at=5ab2ba84fa066c5325581d82)\n* [My Response](https://gitter.im/ManageIQ/v2v?at=5ab2bb3135dd17022e86e8a6)\n* API change:  https://github.com/ManageIQ/manageiq-api/pull/311\n* Supporting core PRS: \n    - https://github.com/ManageIQ/manageiq/pull/17007 \n    - https://github.com/ManageIQ/manageiq/pull/17008\n\n\nSteps for Testing/QA\n--------------------\n\nTake your pick:\n\n1.  Run `bin/rails s` and log in.  The UI will eventually hit `/api` and that call shouldn't be loaded with any more than 6ish queries in the log.\n2.  Run `bin/rails c` and run `User.where(:userid => \"admin\").first.miq_groups.first.sui_product_features`.  There should only be a handful of queries run to do that.\n\nIn both of the above, running on master should lead to a lot of `CACHE` statements in the log (assuming you have the rails log set to `DEBUG`)"},"comment":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/comments/375833981","html_url":"https://github.com/ManageIQ/manageiq/pull/17198#issuecomment-375833981","issue_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/17198","id":375833981,"user":{"login":"miq-bot","id":7806257,"avatar_url":"https://avatars1.githubusercontent.com/u/7806257?v=4","gravatar_id":"","url":"https://api.github.com/users/miq-bot","html_url":"https://github.com/miq-bot","followers_url":"https://api.github.com/users/miq-bot/followers","following_url":"https://api.github.com/users/miq-bot/following{/other_user}","gists_url":"https://api.github.com/users/miq-bot/gists{/gist_id}","starred_url":"https://api.github.com/users/miq-bot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/miq-bot/subscriptions","organizations_url":"https://api.github.com/users/miq-bot/orgs","repos_url":"https://api.github.com/users/miq-bot/repos","events_url":"https://api.github.com/users/miq-bot/events{/privacy}","received_events_url":"https://api.github.com/users/miq-bot/received_events","type":"User","site_admin":false},"created_at":"2018-03-24T00:53:03Z","updated_at":"2018-03-24T00:53:03Z","author_association":"MEMBER","body":"<rubocop />Checked commit https://github.com/NickLaMuro/manageiq/commit/3648be43a1f726f4cd4a437ce67f6121834544d4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0\n1 file checked, 0 offenses detected\nEverything looks fine. :cookie:\n"}}
{"actor":{"display_login":"miq-bot"}}