Refactoring: Clarify complex Permission queries
This commit was prepared to facilitate a larger refactoring assignment as part of the Pundit migration. As a bonus, it includes some SQL query optimizations! === Why was this refactoring necessary? It's not, strictly speaking. But the Pundit migration involves taking complex querying logic and moving it into Scope classes as appropriate, and deciding where things belong is really difficult when you can't see what they're doing. === So how does this refactoring fix the problem? * It replaces raw SQL queries with Ruby-esque ActiveRecord queries. * It replaces complex, procedural code that's full of loops and obscure local variable assignment with compact, cleanly-formatted code that follows Ruby idioms and uses meaningful variable names. In my opinion, it's much faster and easier to understand what the code does this way. === What kinds of SQL query optimizations are included? * n+1 query: user_access_token#index instantiated all active permissions and then called current_user.permissions? on _every single one._ A fresh installation of Zammad contains 57 permissions, so this was a lot of unnecessary queries. The method has been rewritten to make only one query instead. * User#permissions? used to query the DB once for each argument it was given. Now, it only queries the DB once, even when given many arguments. * We had a couple SQL queries that used both #select and #pluck. (When using #pluck, #select is redundant.) Removing #select from these calls did not improve performance, but it did clean up unnecessary code.
This commit is contained in:
Ryan Lue
Thorsten Eckel
parent
b37e80df9a
commit
80b330d73f
|
|
@ -27,42 +27,28 @@ curl http://localhost/api/v1/user_access_token -v -u #{login}:#{password}
|
|||
=end
|
||||
|
||||
def index
|
||||
tokens = Token.where(action: 'api', persistent: true, user_id: current_user.id).order(updated_at: :desc, label: :asc)
|
||||
token_list = []
|
||||
tokens.each do |token|
|
||||
attributes = token.attributes
|
||||
attributes.delete('persistent')
|
||||
attributes.delete('name')
|
||||
token_list.push attributes
|
||||
end
|
||||
local_permissions = current_user.permissions
|
||||
local_permissions_new = {}
|
||||
local_permissions.each_key do |key|
|
||||
keys = ::Permission.with_parents(key)
|
||||
keys.each do |local_key|
|
||||
next if local_permissions_new.key?([local_key])
|
||||
tokens = Token.select(Token.column_names - %w[persistent name])
|
||||
.where(action: 'api', persistent: true, user_id: current_user.id)
|
||||
.order(updated_at: :desc, label: :asc)
|
||||
|
||||
if local_permissions[local_key] == true
|
||||
local_permissions_new[local_key] = true
|
||||
next
|
||||
end
|
||||
local_permissions_new[local_key] = false
|
||||
end
|
||||
end
|
||||
permissions = []
|
||||
Permission.all.where(active: true).order(:name).each do |permission|
|
||||
next if !local_permissions_new.key?(permission.name) && !current_user.permissions?(permission.name)
|
||||
base_query = Permission.order(:name).where(active: true)
|
||||
permission_names = current_user.permissions.keys
|
||||
ancestor_names = permission_names.flat_map { |name| Permission.with_parents(name) }.uniq -
|
||||
permission_names
|
||||
descendant_names = permission_names.map { |name| "#{name}.%" }
|
||||
|
||||
permission_attributes = permission.attributes
|
||||
if local_permissions_new[permission.name] == false
|
||||
permission_attributes['preferences']['disabled'] = true
|
||||
end
|
||||
permissions.push permission_attributes
|
||||
permissions = base_query.where(name: [*ancestor_names, *permission_names])
|
||||
|
||||
descendant_names.each do |name|
|
||||
permissions = permissions.or(base_query.where('permissions.name LIKE ?', name))
|
||||
end
|
||||
|
||||
permissions.select { |permission| permission.name.in?(ancestor_names) }
|
||||
.each { |permission| permission.preferences['disabled'] = true }
|
||||
|
||||
render json: {
|
||||
tokens: token_list,
|
||||
permissions: permissions,
|
||||
tokens: tokens.map(&:attributes),
|
||||
permissions: permissions.map(&:attributes),
|
||||
}, status: :ok
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -392,13 +392,12 @@ returns
|
|||
=end
|
||||
|
||||
def permissions
|
||||
list = {}
|
||||
::Permission.select('permissions.name, permissions.preferences').joins(:roles).where('roles.id IN (?) AND permissions.active = ?', role_ids, true).pluck(:name, :preferences).each do |permission|
|
||||
next if permission[1]['selectable'] == false
|
||||
|
||||
list[permission[0]] = true
|
||||
end
|
||||
list
|
||||
::Permission.joins(roles: :users)
|
||||
.where(users: { id: id }, roles: { active: true }, active: true)
|
||||
.pluck(:name, :preferences)
|
||||
.each_with_object({}) do |(name, preferences), hash|
|
||||
hash[name] = true if preferences['selectable'] != false
|
||||
end
|
||||
end
|
||||
|
||||
=begin
|
||||
|
|
@ -419,23 +418,24 @@ returns
|
|||
|
||||
=end
|
||||
|
||||
def permissions?(key)
|
||||
Array(key).each do |local_key|
|
||||
list = []
|
||||
if local_key.end_with?('.*')
|
||||
local_key = local_key.sub('.*', '.%')
|
||||
permissions = ::Permission.with_parents(local_key)
|
||||
list = ::Permission.select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND (permissions.name IN (?) OR permissions.name LIKE ?) AND permissions.active = ?', role_ids, true, permissions, local_key, true).pluck(:preferences)
|
||||
else
|
||||
permission = ::Permission.lookup(name: local_key)
|
||||
break if permission&.active == false
|
||||
def permissions?(names)
|
||||
base_query = ::Permission.joins(roles: :users)
|
||||
.where(users: { id: id })
|
||||
.where(roles: { active: true })
|
||||
.where(active: true)
|
||||
|
||||
permissions = ::Permission.with_parents(local_key)
|
||||
list = ::Permission.select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND permissions.name IN (?) AND permissions.active = ?', role_ids, true, permissions, true).pluck(:preferences)
|
||||
end
|
||||
return true if list.present?
|
||||
permission_names = Array(names).reject { |name| ::Permission.lookup(name: name)&.active == false }
|
||||
verbatim_names = permission_names.flat_map { |name| ::Permission.with_parents(name) }.uniq
|
||||
wildcard_names = permission_names.select { |name| name.end_with?('.*') }
|
||||
.map { |name| name.sub('.*', '.%') }
|
||||
|
||||
permissions = base_query.where(name: verbatim_names)
|
||||
|
||||
wildcard_names.each do |name|
|
||||
permissions = permissions.or(base_query.where('permissions.name LIKE ?', name))
|
||||
end
|
||||
false
|
||||
|
||||
permissions.exists?
|
||||
end
|
||||
|
||||
=begin
|
||||
|
|
|
|||
|
|
@ -369,6 +369,52 @@ RSpec.describe User, type: :model do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#permissions' do
|
||||
let(:user) { create(:agent).tap { |u| u.roles = [u.roles.first] } }
|
||||
let(:role) { user.roles.first }
|
||||
let(:permissions) { role.permissions }
|
||||
|
||||
it 'returns a hash of <permissions> => true' do
|
||||
expect(user.permissions).to eq(permissions.each.with_object({}) { |p, hash| hash[p.name] = true })
|
||||
end
|
||||
|
||||
shared_examples 'for omissions' do
|
||||
it 'omits them from the returned hash' do
|
||||
expect(user.permissions.keys).not_to include(*omitted_permissions.map(&:name))
|
||||
end
|
||||
end
|
||||
|
||||
context 'for permissions that do not belong to this user' do
|
||||
let(:omitted_permissions) { Permission.all - permissions }
|
||||
|
||||
include_examples 'for omissions'
|
||||
end
|
||||
|
||||
context 'for inactive permissions' do
|
||||
before { omitted_permissions.each { |p| p.update(active: false) } }
|
||||
|
||||
let!(:omitted_permissions) { permissions.first(1) }
|
||||
|
||||
include_examples 'for omissions'
|
||||
end
|
||||
|
||||
context 'for permissions on inactive roles' do
|
||||
before { role.update(active: false) }
|
||||
|
||||
let(:omitted_permissions) { permissions }
|
||||
|
||||
include_examples 'for omissions'
|
||||
end
|
||||
|
||||
context 'for permissions with !preferences["selectable"]' do
|
||||
before { omitted_permissions.each { |p| p.update(preferences: { selectable: false }) } }
|
||||
|
||||
let(:omitted_permissions) { permissions.first(1) }
|
||||
|
||||
include_examples 'for omissions'
|
||||
end
|
||||
end
|
||||
|
||||
describe '#permissions?' do
|
||||
subject(:user) { create(:user, roles: [role]) }
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue