From 03284cf578110215524633633d7f652c2f2847c7 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Fri, 19 Aug 2016 17:40:40 -0500 Subject: [PATCH] Fix calling reference on polymorphic relationships For the record: I am not really proud or satisfied with the code added here in it's current state, but it fixes the problem it seems. Recently, the `lib/extensions/ar_merge_conditions.rb file was removed from the repo as it was deemed no longer to be needed, and we had converted everything over to using AREL. The main method that was removed was the following: def self.apply_legacy_finder_options(options) unknown_keys = options.keys - LEGACY_FINDER_METHODS.map(&:first) raise "Unsupported options #{unknown_keys}" unless unknown_keys.empty? # Determine whether any of the included associations are polymorphic has_polymorphic = included_associations(options[:include]).any? { |name| self.reflection_is_polymorphic?(name) } LEGACY_FINDER_METHODS.inject(all) do |scope, (key, method)| # Don't call references method on scope if polymorphic associations are # included to avoid ActiveRecord::EagerLoadPolymorphicError next(scope) if method == :references && has_polymorphic # options[key] ? scope.send(method, options[key]) : scope end end Which basically mapped the old Rails 2.3 methods (LEGACY_FINDER_METHODS, not shown) to the new(er) Rails 3+ AREL syntax. The important part of note is the lines with `has_polymorphic`, which prevents applying the values from the `:include` key (which is mapped to both the `.includes` and `.reference` methods) to the `.reference` method. When the refactoring was done in 062263c (part of the https://github.com/ManageIQ/manageiq/pull/10175 pull request), the check for the reference methods was not brought along, so when an invalid `:include_for_find` key was passed in with a polymorphic relationship, the code would break with a not so obvious error message: Error caught: [ActiveRecord::ConfigurationError] nil Most of the code in ar_merge_conditions.rb is actually for properly determining when a polymorphic relationship exists, so in reality, this code maybe should have stayed in some way or another. The changes here are an attempt to reduce the amount of code added, but it might be to limited in what keys are checked, and thus, not thorough enough. Regardless, it does seem to prevent most cases. --- lib/rbac/filterer.rb | 19 ++++++++++++-- spec/lib/rbac/filterer_spec.rb | 46 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index d8b3d3ca843..c937337bf77 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -199,10 +199,10 @@ def search(options = {}) scope = scope.except(:offset, :limit) scope = scope_targets(klass, scope, user_filters, user, miq_group) .where(conditions).where(sub_filter).where(where_clause).where(exp_sql).where(ids_clause) - .includes(include_for_find).references(include_for_find) - .includes(exp_includes).references(exp_includes) + .includes(include_for_find).includes(exp_includes) .order(options[:order]) + scope = include_references(scope, klass, include_for_find, exp_includes) scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql] targets = scope @@ -230,6 +230,21 @@ def search(options = {}) return targets, attrs end + def include_references(scope, klass, include_for_find, exp_includes) + ref_includes = Hash(include_for_find).merge(Hash(exp_includes)) + unless polymorphic_include?(klass, ref_includes) + scope = scope.references(include_for_find).references(exp_includes) + end + scope + end + + def polymorphic_include?(target_klass, includes) + includes.keys.any? do |incld| + reflection = target_klass.reflect_on_association(incld) + reflection && reflection.polymorphic? + end + end + def filtered(objects, options = {}) Rbac.search(options.reverse_merge(:targets => objects)).first end diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 363def82779..8a62b8c843b 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -234,6 +234,52 @@ expect(results).to include(vm) end end + + context "for Metrics::Rollup" do + before(:each) do + vm = FactoryGirl.create(:vm_vmware) + FactoryGirl.create( + :metric_rollup_vm_daily, + :resource_id => vm.id, + :timestamp => "2010-04-14T00:00:00Z" + ) + + # Typical includes for rendering daily metrics charts + @include = { + :max_derived_cpu_available => {}, + :max_derived_cpu_reserved => {}, + :min_cpu_usagemhz_rate_average => {}, + :max_cpu_usagemhz_rate_average => {}, + :min_cpu_usage_rate_average => {}, + :max_cpu_usage_rate_average => {}, + :v_pct_cpu_ready_delta_summation => {}, + :v_pct_cpu_wait_delta_summation => {}, + :v_pct_cpu_used_delta_summation => {}, + :max_derived_memory_available => {}, + :max_derived_memory_reserved => {}, + :min_derived_memory_used => {}, + :max_derived_memory_used => {}, + :min_disk_usage_rate_average => {}, + :max_disk_usage_rate_average => {}, + :min_net_usage_rate_average => {}, + :max_net_usage_rate_average => {}, + :v_derived_storage_used => {}, + :resource => {} + } + end + + # NOTE: Think long and hard before you consider removing this test. + # Many-a-hours wasted here determining this bug that resulted in + # re-adding this test again. + it "should not raise an error when a polymorphic reflection is included" do + result = nil + expect do + result = described_class.search :class => "MetricRollup", + :include_for_find => @include + end.not_to raise_error + expect(result.first.length).to eq(1) + end + end end context "common setup" do