From 938a22493bfd80829ef746d4767cf2bbed2eda8f Mon Sep 17 00:00:00 2001 From: Giovanni Sakti Date: Mon, 8 Oct 2018 14:19:52 +0700 Subject: [PATCH 1/2] Avoid N+1 when fetching user response --- app/controllers/nss_controller.rb | 2 -- app/models/group.rb | 2 +- app/models/user.rb | 37 +++++++++++++++++++++++-------- spec/models/user_spec.rb | 2 +- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/app/controllers/nss_controller.rb b/app/controllers/nss_controller.rb index 4830594d..0c78e99d 100755 --- a/app/controllers/nss_controller.rb +++ b/app/controllers/nss_controller.rb @@ -59,10 +59,8 @@ def passwd @response = REDIS_CACHE.get( "P:" + params[:token]) @response = JSON.parse(@response) if @response.present? if @response.blank? - host_machine = HostMachine.find_by(access_key: params[:token]) sysadmins = host_machine.sysadmins if host_machine.present? - if sysadmins.present? && sysadmins.count > 0 @response = User.get_sysadmins sysadmins REDIS_CACHE.set( "P:" + params[:token], @response.to_json) diff --git a/app/models/group.rb b/app/models/group.rb index a604a62d..6ddb5ad6 100755 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -154,7 +154,7 @@ def self.get_default_sysadmin_group_for_host sysadmins_login_ids, default_admins if default_admins group = Group.find_by(name: "sysadmins") - + if group.present? sysadmins = sysadmins + group.get_user_ids end diff --git a/app/models/user.rb b/app/models/user.rb index 359dc56f..24bbd585 100755 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -94,11 +94,25 @@ def name_email "#{name} (#{email})" end - def self.get_sysadmins uids - user_list = [] - uids.each do |uid| - user_list << get_passwd_uid_response(User.find(uid).uid) - end + def self.get_sysadmins user_ids + users = User. + select(%Q( + id, + name, + uid, + user_login_id, + ( + SELECT gid + FROM groups + INNER JOIN group_associations + ON groups.id = group_associations.group_id + WHERE group_associations.user_id = users.id + AND groups.name = users.user_login_id + LIMIT 1 + ) AS gid + )). + where(id: user_ids) + user_list = users.map{ |user| get_passwd_uid_response(user) } user_list end @@ -295,9 +309,7 @@ def self.response_array response user_response end - - def self.get_passwd_uid_response uid - user = User.where(uid: uid).first + def self.get_passwd_uid_response user return [] if user.blank? user.user_passwd_response end @@ -362,7 +374,14 @@ def user_passwd_response user_hash[:pw_name] = user_login_id user_hash[:pw_passwd] = "x" user_hash[:pw_uid] = uid.to_i - user_hash[:pw_gid] = groups.where(name: user_login_id).first.gid if groups.where(name: user_login_id).count > 0 + + # If gid is supplied (avoid N+1) + if self.respond_to?(:gid) && gid + user_hash[:pw_gid] = gid.to_i + else + user_hash[:pw_gid] = groups.where(name: user_login_id).first.gid if groups.where(name: user_login_id).count > 0 + end + user_hash[:pw_gecos] = "#{name}" user_hash[:pw_dir] = "#{HOME_DIR}/#{user_login_id}" user_hash[:pw_shell] = "/bin/bash" diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 75bd42af..991fd102 100755 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -324,7 +324,7 @@ it "should return false if user is not active" do user = create(:user) - response = User.get_passwd_uid_response user.uid + response = User.get_passwd_uid_response user expect(response[:pw_name]).to eq(user.user_login_id) end From 53c2283dc058f37d4281e44c7707f5df76b785ed Mon Sep 17 00:00:00 2001 From: Giovanni Sakti Date: Mon, 8 Oct 2018 14:41:32 +0700 Subject: [PATCH 2/2] use constants to centralize all cache key prefix --- app/controllers/nss_controller.rb | 12 ++++++------ app/models/group.rb | 16 ++++++++-------- app/models/host_machine.rb | 6 +++--- app/models/user.rb | 8 ++++---- config/initializers/redis.rb | 7 +++++++ 5 files changed, 28 insertions(+), 21 deletions(-) diff --git a/app/controllers/nss_controller.rb b/app/controllers/nss_controller.rb index 0c78e99d..f9825e27 100755 --- a/app/controllers/nss_controller.rb +++ b/app/controllers/nss_controller.rb @@ -39,15 +39,15 @@ def add_host end def group - @response = REDIS_CACHE.get( "G:" + params[:token]) + @response = REDIS_CACHE.get("#{GROUP_RESPONSE}:#{params[:token]}") @response = JSON.parse(@response) if @response.present? if @response.blank? host_machine = HostMachine.find_by(access_key: params[:token]) sysadmins = host_machine.sysadmins if host_machine.present? if sysadmins.present? && sysadmins.count > 0 @response = Group.get_sysadmins_and_groups sysadmins, host_machine.default_admins - REDIS_CACHE.set( "G:" + params[:token], @response.to_json) - REDIS_CACHE.expire( "G:" + params[:token], REDIS_KEY_EXPIRY) + REDIS_CACHE.set("#{GROUP_RESPONSE}:#{params[:token]}", @response.to_json) + REDIS_CACHE.expire("#{GROUP_RESPONSE}:#{params[:token]}", REDIS_KEY_EXPIRY) end end @@ -56,15 +56,15 @@ def group def passwd - @response = REDIS_CACHE.get( "P:" + params[:token]) + @response = REDIS_CACHE.get("#{PASSWD_RESPONSE}:#{params[:token]}") @response = JSON.parse(@response) if @response.present? if @response.blank? host_machine = HostMachine.find_by(access_key: params[:token]) sysadmins = host_machine.sysadmins if host_machine.present? if sysadmins.present? && sysadmins.count > 0 @response = User.get_sysadmins sysadmins - REDIS_CACHE.set( "P:" + params[:token], @response.to_json) - REDIS_CACHE.expire( "P:" + params[:token], REDIS_KEY_EXPIRY) + REDIS_CACHE.set("#{PASSWD_RESPONSE}:#{params[:token]}", @response.to_json) + REDIS_CACHE.expire("#{PASSWD_RESPONSE}:#{params[:token]}", REDIS_KEY_EXPIRY) end end render json: @response diff --git a/app/models/group.rb b/app/models/group.rb index 6ddb5ad6..8fc0f39a 100755 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -28,8 +28,8 @@ def burst_host_cache if host_machines.count > 0 host_machines.each do |host| if host.access_key.present? - REDIS_CACHE.del ("G:" + host.access_key) - REDIS_CACHE.del ("P:" + host.access_key) + REDIS_CACHE.del ("#{GROUP_RESPONSE}:#{host.access_key}") + REDIS_CACHE.del ("#{PASSWD_RESPONSE}:#{host.access_key}") Rails.logger.info "hello #{host.name} #{host.access_key}" end end @@ -100,7 +100,7 @@ def group_response end def self.group_nss_response name - group_response = REDIS_CACHE.get( "UG:" + name) + group_response = REDIS_CACHE.get("#{GROUP_NAME_RESPONSE}:#{name}") group_response = JSON.parse(group_response) if group_response.present? if group_response.blank? @@ -111,8 +111,8 @@ def self.group_nss_response name response_hash[:gr_passwd] = "x" response_hash[:gr_gid] = group.gid response_hash[:gr_mem] = group.users.collect { |u| u.user_login_id} - REDIS_CACHE.set( "UG:" + group.name, response_hash.to_json) - REDIS_CACHE.expire( "UG:" + group.name, REDIS_KEY_EXPIRY) + REDIS_CACHE.set("#{GROUP_NAME_RESPONSE}:#{group.name}", response_hash.to_json) + REDIS_CACHE.expire("#{GROUP_NAME_RESPONSE}:#{group.name}", REDIS_KEY_EXPIRY) group_response = response_hash end end @@ -137,13 +137,13 @@ def self.get_groups_for_host sysadmins end def get_user_ids - user_ids = REDIS_CACHE.get( "G_UID:" + name) + user_ids = REDIS_CACHE.get("#{GROUP_UID_RESPONSE}:#{name}") user_ids = JSON.parse(user_ids) if user_ids.present? if user_ids.blank? user_ids = users.collect {|u| u.user_login_id} - REDIS_CACHE.set( "G_UID:" + name, user_ids.to_json) - REDIS_CACHE.expire( "G_UID:" + name, REDIS_KEY_EXPIRY) + REDIS_CACHE.set("#{GROUP_UID_RESPONSE}:#{name}", user_ids.to_json) + REDIS_CACHE.expire("#{GROUP_UID_RESPONSE}:#{name}", REDIS_KEY_EXPIRY) end return user_ids end diff --git a/app/models/host_machine.rb b/app/models/host_machine.rb index ed1f984b..69870a72 100755 --- a/app/models/host_machine.rb +++ b/app/models/host_machine.rb @@ -28,11 +28,11 @@ def self.get_group_response name end def remove_host_cache - REDIS_CACHE.del("HOST_UID:" + name) + REDIS_CACHE.del("#{HOST_UID_PREFIX}:#{name}") end def sysadmins - host_users = REDIS_CACHE.get("HOST_UID:" + name) + host_users = REDIS_CACHE.get("#{HOST_UID_PREFIX}:#{name}") host_users = JSON.parse(host_users) if host_users.present? users = [] if host_users.blank? @@ -41,7 +41,7 @@ def sysadmins where("group_id IN (?)", groups.collect(&:id)). collect(&:user_id) host_users = users.uniq - REDIS_CACHE.set("HOST_UID:" + name, host_users.to_json) + REDIS_CACHE.set("#{HOST_UID_PREFIX}:#{name}", host_users.to_json) end host_users end diff --git a/app/models/user.rb b/app/models/user.rb index 24bbd585..65e468b4 100755 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -17,16 +17,16 @@ class User < ActiveRecord::Base before_save :remove_user_cache def remove_user_cache - REDIS_CACHE.del( "USER_CACHE:" + "#{id}") if id.present? + REDIS_CACHE.del("#{USER_CACHE_PREFIX}:#{id}") if id.present? end def self.from_cache id - user = REDIS_CACHE.get( "USER_CACHE:" + "#{id}") + user = REDIS_CACHE.get("#{USER_CACHE_PREFIX}:#{id}") user = JSON.parse(user) if user.present? if user.blank? user = User.find(id).to_json - REDIS_CACHE.set( "USER_CACHE:" + "#{id}", user) - REDIS_CACHE.expire( "USER_CACHE:" + "#{id}", REDIS_KEY_EXPIRY) + REDIS_CACHE.set("#{USER_CACHE_PREFIX}:#{id}", user) + REDIS_CACHE.expire("#{USER_CACHE_PREFIX}:#{id}", REDIS_KEY_EXPIRY) user = JSON.parse(user) end return user diff --git a/config/initializers/redis.rb b/config/initializers/redis.rb index 51a3992f..17e67df6 100644 --- a/config/initializers/redis.rb +++ b/config/initializers/redis.rb @@ -11,6 +11,9 @@ GROUP_NAME_RESPONSE = "GROUP_NAME:" GROUP_GID_RESPONSE = "GROUP_GID:" GROUP_ALL_RESPONSE = "GROUP_ALL" +GROUP_RESPONSE = "G" +GROUP_NAME_RESPONSE = "UG" +GROUP_UID_RESPONSE = "G_UID" SHADOW_NAME_RESPONSE = "SHADOW_NAME:" SHADOW_ALL_RESPONSE = "SHADOW_ALL" @@ -18,7 +21,11 @@ PASSWD_NAME_RESPONSE = "PASSWD_NAME:" PASSWD_UID_RESPONSE = "PASSWD_UID:" PASSWD_ALL_RESPONSE = "PASSWD_ALL" +PASSWD_RESPONSE = "P" HOST_GROUP_RESPONSE = "HG:" +HOST_UID_PREFIX = "HOST_UID" + +USER_CACHE_PREFIX = "USER_CACHE" REDIS_KEY_EXPIRY = 7 * 60