Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

Merged
merged 3 commits into from
Jun 21, 2018
Merged

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ad75c2
Add MiqReport#all_custom_attributes_are_virtual_sql_attributes?
NickLaMuro Jun 20, 2018
e7fd484
Remove custom_attributes from MiqReport includes
NickLaMuro Jun 20, 2018
065405d
Convert custom_attributes to arel virtual_attributes
NickLaMuro Jun 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/models/miq_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
7 changes: 7 additions & 0 deletions app/models/miq_report/generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to understand this more:

def _generate_table
  if ...
    4 generate_* methods
  else
    generate_basic_results # A
  end

  build_table # B
end

This change adds remove_loading_relations_for_virtual_custom_attributes A (optionally).
build_table always calls remove_loading_relations_for_virtual_custom_attributes.

  1. Do we need to consider using that method for other generate_* methods?
  2. Do we need to make it optional in build_table?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Do we need to consider using that method for other generate_* methods?

Possibly, though not the priority for this BZ. Should function still without it at the moment, and I think the way we gather results for reports is vastly different in each of those methods, so I think waiting to do that makes sense for this PR.

2. Do we need to make it optional in build_table?

No, this should be idempotent to remove the value from the include hash. It is not saving the record with it removed anyway, so we are just removing it earlier than originally planed.


rbac_opts = options.merge(
:targets => targets,
:filter => conditions,
Expand Down
34 changes: 28 additions & 6 deletions app/models/mixins/custom_attribute_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the trend is to always define the uses even with arel (as you have here)
Could we remove the :uses from the virtual_column and get rid of the remove_loading_relations_for_virtual_custom_attributes? ?

(speculation here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we can't remove the remove_loading_relations_for_virtual_custom_attributes.

The remove_loading_relations_for_virtual_custom_attributes is making sure we don't have a :custom_attributes =>{} in the include of the report, which will get added to the .includes and .references in Rbac, causing the join bomb we were observing.

The :custom_attributes =>{} is also added via remove_loading_relations_for_virtual_custom_attributes's sister method, add_includes_for_virtual_custom_attributes, and that is only call as part of of the controller in the UI prior to saving the record.

The call to remove_loading_relations_for_virtual_custom_attributes is making sure we don't get that LEFT JOIN happening in the query, while the rest of the PR is avoiding not introducing an N+1 as a result.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding :uses, unsure if keeping it has any value, though not really sure it hurts. Don't care either way though.


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

where_args = {:name => name_val}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't write this originally, just updated the variable name. Just left it the same as it was.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😊

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)
Expand Down