From a05aaf4f97aac7e90e6406ce1ac50861f35c1fb1 Mon Sep 17 00:00:00 2001 From: Keenan Brock <keenan@thebrocks.net> Date: Fri, 2 Oct 2020 16:33:38 -0400 Subject: [PATCH] Hide deprecated and duplicate attributes Some virtual_attributes are not needed in reporting nor the api. Some deprecated attributes should not be advertised for customers to start using. Now, the attributes are still accessible. So nothing will break. But these attributes are no longer advertised on the public apis (i.e.: report builder and rest api). This allows us to control which parts of our models are exposed. This fixes https://github.com/ManageIQ/manageiq/issues/18130 This enables https://github.com/ManageIQ/manageiq/issues/20532 Hide deprecated and duplicate attributes Some virtual_attributes are not needed in reporting nor the api. Some deprecated attributes should not be advertised for customers to use. After this change, the attributes are still accessible. So nothing will break, but these attributes are no longer advertised in the public apis (i.e.: report builder and rest api). This allows us to control which parts of our models are exposed. This fixes https://github.com/ManageIQ/manageiq/issues/18130 This enables https://github.com/ManageIQ/manageiq/issues/20532 --- app/models/application_record.rb | 1 + app/models/ext_management_system.rb | 3 ++ app/models/mixins/deprecation_mixin.rb | 1 + lib/acts_as_ar_model.rb | 1 + lib/extensions/ar_visible_attribute.rb | 23 ++++++++++++ lib/miq_expression.rb | 2 +- .../extensions/ar_visible_attribute_spec.rb | 37 +++++++++++++++++++ 7 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 lib/extensions/ar_visible_attribute.rb create mode 100644 spec/lib/extensions/ar_visible_attribute_spec.rb diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 0e27a438fb9f..ed2ca68135e7 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -11,6 +11,7 @@ class ApplicationRecord < ActiveRecord::Base include ArNestedCountBy include ArHrefSlug include ToModelHash + include ArVisibleAttribute extend ArTableLock extend ArReferences diff --git a/app/models/ext_management_system.rb b/app/models/ext_management_system.rb index 8df699f5a8b2..81d6a766d01d 100644 --- a/app/models/ext_management_system.rb +++ b/app/models/ext_management_system.rb @@ -3,6 +3,9 @@ class ExtManagementSystem < ApplicationRecord include SupportsFeatureMixin include ExternalUrlMixin include VerifyCredentialsMixin + include DeprecationMixin + + hide_attribute "aggregate_memory" # better to use total_memory (coin toss - they're similar) def self.with_tenant(tenant_id) tenant = Tenant.find(tenant_id) diff --git a/app/models/mixins/deprecation_mixin.rb b/app/models/mixins/deprecation_mixin.rb index 4f1f254bf465..bf905b1f1410 100644 --- a/app/models/mixins/deprecation_mixin.rb +++ b/app/models/mixins/deprecation_mixin.rb @@ -18,6 +18,7 @@ def deprecate_belongs_to(old_belongs_to, new_belongs_to) def deprecate_attribute(old_attribute, new_attribute) deprecate_attribute_methods(old_attribute, new_attribute) virtual_attribute(old_attribute, -> { type_for_attribute(new_attribute.to_s) }) + hide_attribute(old_attribute) end def deprecate_attribute_methods(old_attribute, new_attribute) diff --git a/lib/acts_as_ar_model.rb b/lib/acts_as_ar_model.rb index 2640e906e6a4..70aa219a9fcc 100644 --- a/lib/acts_as_ar_model.rb +++ b/lib/acts_as_ar_model.rb @@ -1,5 +1,6 @@ class ActsAsArModel include Vmdb::Logging + include ArVisibleAttribute def self.connection ActiveRecord::Base.connection diff --git a/lib/extensions/ar_visible_attribute.rb b/lib/extensions/ar_visible_attribute.rb new file mode 100644 index 000000000000..c5e21f846b6c --- /dev/null +++ b/lib/extensions/ar_visible_attribute.rb @@ -0,0 +1,23 @@ +module ArVisibleAttribute + extend ActiveSupport::Concern + + included do + class_attribute :hidden_attribute_names, :default => [] + self.hidden_attribute_names = [] + end + + class_methods do + # @param [String|Symbol] attribute name of attribute to be hidden from the api and reporting + # this attribute is accessible to all ruby methods. But it is not advertised. + # we do this when deprecating an attribute or when introducing an internal attribute + def hide_attribute(attribute) + self.hidden_attribute_names += [attribute.to_s] + end + + # @return Array[String] attribute names that can be advertised in the api and reporting + # Other attributes are accessible, they are just no longer in our public api (or never were) + def visible_attribute_names + self.attribute_names - self.hidden_attribute_names + end + end +end diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 02851f896267..2be55e0380a7 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -909,7 +909,7 @@ def self.build_relats(model, parent = {}, seen = []) parent[:class_path] ||= model.name parent[:assoc_path] ||= model.name parent[:root] ||= model.name - result = {:columns => model.attribute_names, :parent => parent} + result = {:columns => model.visible_attribute_names, :parent => parent} result[:reflections] = {} model.reflections_with_virtual.each do |assoc, ref| diff --git a/spec/lib/extensions/ar_visible_attribute_spec.rb b/spec/lib/extensions/ar_visible_attribute_spec.rb new file mode 100644 index 000000000000..43e8d9bf59d0 --- /dev/null +++ b/spec/lib/extensions/ar_visible_attribute_spec.rb @@ -0,0 +1,37 @@ +RSpec.describe ArVisibleAttribute do + # NOTE: ApplicationRecord already included ArVisibleAttribute + let(:klass) { Class.new(ApplicationRecord) { self.table_name = "vms" } } + let(:other_klass) { Class.new(ApplicationRecord) { self.table_name = "vms" } } + + context ".visible_attribute_names" do + it "shows regular attributes" do + expect(klass.visible_attribute_names).to include("type") + end + + it "hides hidden attributes" do + klass.hide_attribute :type + expect(klass.visible_attribute_names).not_to include("type") + end + + it "only hides for specified class" do + klass.hide_attribute :type + expect(other_klass.visible_attribute_names).to include("type") + end + end + + context ".hidden_attribute_names" do + it "starts with no hidden attributes" do + expect(klass.hidden_attribute_names).to be_empty + end + + it "hides hidden attributes" do + klass.hide_attribute :type + expect(klass.hidden_attribute_names).to include("type") + end + + it "only hides for specified class" do + klass.hide_attribute :type + expect(other_klass.hidden_attribute_names).not_to include("type") + end + end +end