From 0ad75c2117faa8ce3678932fc8d383429d55b4e5 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 19 Jun 2018 19:03:05 -0500 Subject: [PATCH 1/3] Add MiqReport#all_custom_attributes_are_virtual_sql_attributes? https://bugzilla.redhat.com/show_bug.cgi?id=1592480 This method is responsible for checking if the virtual_attributes in the Rbac query will contain all of the custom_attributes virtual_attributes. To be used to determine if we can safely remove: { :custom_attributes => {} } From the `include` hash on MiqReport, avoiding needing to load a lot of records. --- app/models/miq_report.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/miq_report.rb b/app/models/miq_report.rb index 67179180c2d..1b35733d828 100644 --- a/app/models/miq_report.rb +++ b/app/models/miq_report.rb @@ -200,6 +200,11 @@ def page_size rpt_options.try(:fetch_path, :pdf, :page_size) || "a4" end + def all_custom_attributes_are_virtual_sql_attributes? + ca_va_cols = CustomAttributeMixin.select_virtual_custom_attributes(cols) + ca_va_cols.all? { |custom_attribute| va_sql_cols.include?(custom_attribute) } + end + def load_custom_attributes return unless db_klass < CustomAttributeMixin || Chargeback.db_is_chargeback?(db) From e7fd484acc9813462744046dd8b464a08b443527 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 19 Jun 2018 19:03:48 -0500 Subject: [PATCH 2/3] Remove custom_attributes from MiqReport includes https://bugzilla.redhat.com/show_bug.cgi?id=1592480 Conditionally removes `:custom_attributes => {}` from the includes clause if all of the `custom_attribute` columns happen to be in the select statement (the `@va_sql_cols` value). When the `custom_attribute` `virtual_attributes` can be converted to Arel, this will have some value, but for now this will mostly do nothing --- app/models/miq_report/generator.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/models/miq_report/generator.rb b/app/models/miq_report/generator.rb index 23f890e42be..92fc1488cdb 100644 --- a/app/models/miq_report/generator.rb +++ b/app/models/miq_report/generator.rb @@ -307,6 +307,13 @@ def generate_basic_results(options = {}) # TODO: add once only_cols is fixed # targets = targets.select(only_cols) where_clause = MiqExpression.merge_where_clauses(self.where_clause, options[:where_clause]) + + # Remove custom_attributes as part of the `includes` if all of them exist + # in the select statement + if all_custom_attributes_are_virtual_sql_attributes? + remove_loading_relations_for_virtual_custom_attributes + end + rbac_opts = options.merge( :targets => targets, :filter => conditions, From 065405d49975c80b3b852ddb21d413719bec976e Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 19 Jun 2018 19:06:35 -0500 Subject: [PATCH 3/3] Convert custom_attributes to arel virtual_attributes https://bugzilla.redhat.com/show_bug.cgi?id=1592480 This change allows `custom_attributes` to be accessible via SQL. Most of the additions are found in the `custom_attribute_arel`, included by `CustomAttributeMixin`, and will simply generate the Arel equivalent necessary for determined the `virtual_attribute` value via SQL. Other notable changes: - The method defined for accessing the `virtual_attribute` from the instance will now check the attributes hash prior to executing the rest of the method, and return something if it has a key - Some shared values calculated in both the method definition and would be in the `custom_attribute_arel` method have been extracted out so it is only calculated once (minor performance benefit). --- app/models/mixins/custom_attribute_mixin.rb | 34 +++++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/app/models/mixins/custom_attribute_mixin.rb b/app/models/mixins/custom_attribute_mixin.rb index ae2f412b5f7..d5e4c74da39 100644 --- a/app/models/mixins/custom_attribute_mixin.rb +++ b/app/models/mixins/custom_attribute_mixin.rb @@ -41,19 +41,41 @@ def self.load_custom_attributes_for(cols) def self.add_custom_attribute(custom_attribute) return if respond_to?(custom_attribute) - virtual_column(custom_attribute.to_sym, :type => :string, :uses => :custom_attributes) + ca_sym = custom_attribute.to_sym + without_prefix = custom_attribute.sub(CUSTOM_ATTRIBUTES_PREFIX, "") + name_val, section = without_prefix.split(SECTION_SEPARATOR) + sanatized_column_alias = custom_attribute.tr('.', 'DOT').tr('/', 'BS').tr(':', 'CLN') + ca_arel = custom_attribute_arel(name_val, section, sanatized_column_alias) - define_method(custom_attribute.to_sym) do - custom_attribute_without_prefix = custom_attribute.sub(CUSTOM_ATTRIBUTES_PREFIX, "") - custom_attribute_without_section, section = custom_attribute_without_prefix.split(SECTION_SEPARATOR) + virtual_column(ca_sym, :type => :string, :uses => :custom_attributes, :arel => ca_arel) - where_args = {} - where_args[:name] = custom_attribute_without_section + define_method(ca_sym) do + return self[sanatized_column_alias] if has_attribute?(sanatized_column_alias) + + where_args = {} + where_args[:name] = name_val where_args[:section] = section if section custom_attributes.find_by(where_args).try(:value) end end + + def self.custom_attribute_arel(name_val, section, column_alias) + lambda do |t| + ca_field = CustomAttribute.arel_table + + field_where = ca_field[:resource_id].eq(t[:id]) + field_where = field_where.and(ca_field[:resource_type].eq(base_class.name)) + field_where = field_where.and(ca_field[:name].eq(name_val)) + field_where = field_where.and(ca_field[:section].eq(section)) if section + + # Because there is a `find_by` in the `define_method` above, we are + # using a `take(1)` here as well, since a limit is assumed in each. + # Without it, there can be some invalid queries if more than one result + # is returned. + ca_field.project(:value).where(field_where).take(1).as(column_alias) + end + end end def self.to_human(column)