https://bugzilla.redhat.com/show_bug.cgi?id=788148
Add 'pool family' to permission inheritance for various types. In addition, refactor permission queries to reduce the join complexity: 1) denormalize 'pool family' association into deployments, instances, etc. -- once created it never changes, so this reduces the number of joins for permissions 2) pre-generate a list of roles that include the desired privilege -- this keeps the permission queries from having to join permissions, roles, and privileges tables (all 3 tables) multiple times in a single query. Sometimes two queries are better than one. This resulted in significant speedup in queries where the user was granted permissions via the pool family. --- src/app/models/catalog.rb | 27 +++++++++++++++ src/app/models/deployable.rb | 34 ++++++++++++++++++- src/app/models/deployment.rb | 20 ++++++++--- src/app/models/instance.rb | 23 ++++++++----- src/app/models/permissioned_object.rb | 8 ++-- src/app/models/pool.rb | 19 +++++++++++ src/app/models/provider_account.rb | 7 +--- .../20120227150102_add_pool_family_to_instance.rb | 32 ++++++++++++++++++ 8 files changed, 144 insertions(+), 26 deletions(-) create mode 100644 src/db/migrate/20120227150102_add_pool_family_to_instance.rb
diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb index 3472c59..307c4dd 100644 --- a/src/app/models/catalog.rb +++ b/src/app/models/catalog.rb @@ -32,6 +32,7 @@ class Catalog < ActiveRecord::Base include PermissionedObject
belongs_to :pool + belongs_to :pool_family has_many :catalog_entries, :dependent => :destroy has_many :deployables, :through => :catalog_entries has_many :permissions, :as => :permission_object, :dependent => :destroy, @@ -39,12 +40,34 @@ class Catalog < ActiveRecord::Base :order => "permissions.id ASC"
before_destroy :destroy_deployables_related_only_to_self + before_create :set_pool_family
validates_presence_of :pool validates_presence_of :name validates_uniqueness_of :name validates_length_of :name, :maximum => 1024
+ def object_list + super + [pool, pool_family] + end + class << self + alias orig_list_for_user_include list_for_user_include + alias orig_list_for_user_conditions list_for_user_conditions + end + + def self.list_for_user_include + orig_list_for_user_include + [ {:pool => :permissions}, + {:pool_family => :permissions} ] + end + + def self.list_for_user_conditions + "(#{orig_list_for_user_conditions}) or + (permissions_pools.user_id=:user and + permissions_pools.role_id in (:role_ids)) or + (permissions_pool_families.user_id=:user and + permissions_pool_families.role_id in (:role_ids))" + end + PRESET_FILTERS_OPTIONS = [ {:title => I18n.t("catalogs.preset_filters.belongs_to_default_pool"), :id => "belongs_to_default_pool", :query => includes(:pool).where("pools.name" => "Default")} ] @@ -53,6 +76,10 @@ class Catalog < ActiveRecord::Base deployables.each {|d| d.destroy if d.catalogs.count == 1} end
+ def set_pool_family + self[:pool_family_id] = pool.pool_family_id + end + private
def self.apply_search_filter(search) diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb index c77f094..ad537a7 100644 --- a/src/app/models/deployable.rb +++ b/src/app/models/deployable.rb @@ -34,9 +34,11 @@ class Deployable < ActiveRecord::Base :order => "permissions.id ASC" has_many :catalog_entries, :dependent => :delete_all has_many :catalogs, :through => :catalog_entries + belongs_to :pool_family
belongs_to :owner, :class_name => "User", :foreign_key => "owner_id" after_create "assign_owner_roles(owner)" + before_create :set_pool_family
scope :without_catalog, lambda { deployable_ids_in_association = CatalogEntry.select(:deployable_id).map(&:deployable_id) @@ -45,6 +47,30 @@ class Deployable < ActiveRecord::Base
PRESET_FILTERS_OPTIONS = []
+ def object_list + super + catalogs + catalogs.collect{|c| c.pool} + [pool_family] + end + class << self + alias orig_list_for_user_include list_for_user_include + alias orig_list_for_user_conditions list_for_user_conditions + end + + def self.list_for_user_include + orig_list_for_user_include + [ {:catalogs => [:permissions, + {:pool => :permissions}]}, + {:pool_family => :permissions} ] + end + + def self.list_for_user_conditions + "(#{orig_list_for_user_conditions}) or + (permissions_catalogs.user_id=:user and + permissions_catalogs.role_id in (:role_ids)) or + (permissions_pools.user_id=:user and + permissions_pools.role_id in (:role_ids)) or + (permissions_pool_families.user_id=:user and + permissions_pool_families.role_id in (:role_ids))" + end + def valid_deployable_xml? begin deployable_xml = DeployableXML.new(xml) @@ -133,12 +159,12 @@ class Deployable < ActiveRecord::Base :assembly => assembly.name, :uuid => assembly.image_id) else - if image.environment != catalogs.first.pool.pool_family.name + if image.environment != pool_family.name deployable_errors << I18n.t("deployables.flash.error.wrong_environment", :assembly => assembly.name, :uuid => assembly.image_id, :wrong_env => image.environment, - :environment => catalogs.first.pool.pool_family.name) + :environment => pool_family.name) end images << image assembly_hash[:build_and_target_uuids] = get_build_and_target_uuids(image) @@ -186,6 +212,10 @@ class Deployable < ActiveRecord::Base (latest_build ? latest_build.target_images.collect { |ti| ti.uuid} : nil)] end
+ def set_pool_family + self[:pool_family_id] = catalogs.first.pool.pool_family_id + end + private
def self.apply_search_filter(search) diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb index ec66fdb..c0c88b2 100644 --- a/src/app/models/deployment.rb +++ b/src/app/models/deployment.rb @@ -42,6 +42,7 @@ class Deployment < ActiveRecord::Base end
belongs_to :pool + belongs_to :pool_family
has_many :instances, :dependent => :destroy
@@ -71,6 +72,8 @@ class Deployment < ActiveRecord::Base before_create :inject_launch_parameters before_create :generate_uuid before_save :replace_special_characters_in_name + before_create :set_pool_family + USER_MUTABLE_ATTRS = ['name'] STATE_MIXED = "mixed"
@@ -105,7 +108,7 @@ class Deployment < ActiveRecord::Base end
def object_list - super << pool + super + [pool, pool_family] end class << self alias orig_list_for_user_include list_for_user_include @@ -113,16 +116,16 @@ class Deployment < ActiveRecord::Base end
def self.list_for_user_include - includes = orig_list_for_user_include - includes << { :pool => {:permissions => {:role => :privileges}}} - includes + orig_list_for_user_include + [ {:pool => :permissions}, + {:pool_family => :permissions} ] end
def self.list_for_user_conditions "(#{orig_list_for_user_conditions}) or (permissions_pools.user_id=:user and - privileges_roles.target_type=:target_type and - privileges_roles.action=:action)" + permissions_pools.role_id in (:role_ids)) or + (permissions_pool_families.user_id=:user and + permissions_pool_families.role_id in (:role_ids))" end
def get_action_list(user=nil) @@ -596,4 +599,9 @@ class Deployment < ActiveRecord::Base def replace_special_characters_in_name name.gsub!(/[^a-zA-Z0-9]+/, '-') end + + def set_pool_family + self[:pool_family_id] = pool.pool_family_id + end + end diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 9027e24..09ec599 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -70,6 +70,7 @@ class Instance < ActiveRecord::Base @@per_page = 15
belongs_to :pool + belongs_to :pool_family belongs_to :provider_account belongs_to :deployment
@@ -100,6 +101,7 @@ class Instance < ActiveRecord::Base validates_length_of :name, :maximum => 1024
before_create :generate_uuid + before_create :set_pool_family
STATE_NEW = "new" STATE_PENDING = "pending" @@ -144,7 +146,7 @@ class Instance < ActiveRecord::Base USER_MUTABLE_ATTRS = ['name']
def object_list - super + [pool, deployment] + super + [deployment, pool, pool_family] end class << self alias orig_list_for_user_include list_for_user_include @@ -152,20 +154,19 @@ class Instance < ActiveRecord::Base end
def self.list_for_user_include - includes = orig_list_for_user_include - includes << { :pool => {:permissions => {:role => :privileges}}, - :deployment => {:permissions => {:role => :privileges}}} - includes + orig_list_for_user_include + [ {:deployment => :permissions}, + {:pool => :permissions}, + {:pool_family => :permissions} ] end
def self.list_for_user_conditions "(#{orig_list_for_user_conditions}) or (permissions_deployments.user_id=:user and - privileges_roles.target_type=:target_type and - privileges_roles.action=:action) or + permissions_deployments.role_id in (:role_ids)) or (permissions_pools.user_id=:user and - privileges_roles_2.target_type=:target_type and - privileges_roles_2.action=:action)" + permissions_pools.role_id in (:role_ids)) or + (permissions_pool_families.user_id=:user and + permissions_pool_families.role_id in (:role_ids))" end
def get_action_list(user=nil) @@ -511,6 +512,10 @@ class Instance < ActiveRecord::Base self[:uuid] = UUIDTools::UUID.timestamp_create.to_s end
+ def set_pool_family + self[:pool_family_id] = pool.pool_family_id + end + def do_operation(user, operation) @task = self.queue_action(user, operation) unless @task diff --git a/src/app/models/permissioned_object.rb b/src/app/models/permissioned_object.rb index 36a2f25..c534261 100644 --- a/src/app/models/permissioned_object.rb +++ b/src/app/models/permissioned_object.rb @@ -59,20 +59,20 @@ module PermissionedObject self.name.constantize end def self.list_for_user_include - [{:permissions => {:role => :privileges}}] + [:permissions] end def self.list_for_user_conditions "permissions.user_id=:user and - privileges.target_type=:target_type and - privileges.action=:action" + permissions.role_id in (:role_ids)" end def self.list_for_user(user, action, target_type=self.default_privilege_target_type) return where("1=0") if user.nil? or action.nil? or target_type.nil? if BasePermissionObject.general_permission_scope.has_privilege(user, action, target_type) scoped else + role_ids = Role.includes(:privileges).where("privileges.target_type" => target_type, "privileges.action" => action).collect {|r| r.id} include_clause = self.list_for_user_include - conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action} + conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action, :role_ids => role_ids} conditions_str = self.list_for_user_conditions includes(include_clause).where(conditions_str, conditions_hash) end diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb index be3d156..3fce49b 100644 --- a/src/app/models/pool.rb +++ b/src/app/models/pool.rb @@ -137,6 +137,25 @@ class Pool < ActiveRecord::Base {:title => I18n.t("pools.preset_filters.with_stopped_instances"), :id => "with_stopped_instances", :query => includes(:deployments => :instances).where("instances.state" => "stopped")} ]
+ def object_list + super + [pool_family] + end + class << self + alias orig_list_for_user_include list_for_user_include + alias orig_list_for_user_conditions list_for_user_conditions + end + + def self.list_for_user_include + orig_list_for_user_include + [ {:pool_family => :permissions} ] + end + + def self.list_for_user_conditions + "(#{orig_list_for_user_conditions}) or + (permissions_pool_families.user_id=:user and + permissions_pool_families.role_id in (:role_ids))" + end + + private
def self.apply_search_filter(search) diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index eff59f9..1e0ed69 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -115,16 +115,13 @@ class ProviderAccount < ActiveRecord::Base end
def self.list_for_user_include - includes = orig_list_for_user_include - includes << { :provider => {:permissions => {:role => :privileges}}} - includes + orig_list_for_user_include + [{ :provider => :permissions}] end
def self.list_for_user_conditions "(#{orig_list_for_user_conditions}) or (permissions_providers.user_id=:user and - privileges_roles.target_type=:target_type and - privileges_roles.action=:action)" + permissions_providers.role_id in (:role_ids))" end
def destroyable? diff --git a/src/db/migrate/20120227150102_add_pool_family_to_instance.rb b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb new file mode 100644 index 0000000..00848ba --- /dev/null +++ b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb @@ -0,0 +1,32 @@ +class AddPoolFamilyToInstance < ActiveRecord::Migration + def self.up + add_column :instances, :pool_family_id, :integer + add_column :deployments, :pool_family_id, :integer + add_column :catalogs, :pool_family_id, :integer + add_column :deployables, :pool_family_id, :integer + + Deployment.all.each do |deployment| + deployment.pool_family_id = deployment.pool.pool_family_id + deployment.save! + end + Instance.all.each do |instance| + instance.pool_family_id = instance.pool.pool_family_id + instance.save! + end + Catalog.all.each do |catalog| + catalog.pool_family_id = catalog.pool.pool_family_id + catalog.save! + end + Deployable.all.each do |deployable| + deployable.pool_family_id = deployable.catalogs.first.pool_family_id unless deployable.catalogs.empty? + deployable.save! + end + end + + def self.down + remove_column :instances, :pool_family_id + remove_column :deployments, :pool_family_id + remove_column :catalogs, :pool_family_id + remove_column :deployables, :pool_family_id + end +end