From eaad9fb1536c4123351477cf30bde41a7f98d881 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 8 Mar 2019 18:04:03 -0500 Subject: [PATCH] Deprecate invalid custom attribute names add_custom_attribute/miq_custom_set will now log a deprecation if we try to define or add a custom attribute name that is an invalid column name. https://bugzilla.redhat.com/show_bug.cgi?id=1662319 From: https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS SQL identifiers and key words must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters in an identifier or key word can be letters, underscores, digits (0-9), or dollar signs ($). Note that dollar signs are not allowed in identifiers according to the letter of the SQL standard, so their use might render applications less portable. The SQL standard will not define a key word that contains digits or starts or ends with an underscore, so identifiers of this form are safe against possible conflict with future extensions of the standard. Similar to https://bugzilla.redhat.com/show_bug.cgi?id=1558618 --- app/models/mixins/custom_attribute_mixin.rb | 9 +++++ .../mixins/custom_attribute_mixin_spec.rb | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/app/models/mixins/custom_attribute_mixin.rb b/app/models/mixins/custom_attribute_mixin.rb index d5e4c74da39..eec14a3b5a1 100644 --- a/app/models/mixins/custom_attribute_mixin.rb +++ b/app/models/mixins/custom_attribute_mixin.rb @@ -5,6 +5,9 @@ module CustomAttributeMixin SECTION_SEPARATOR = ":SECTION:".freeze DEFAULT_SECTION_NAME = 'Custom Attribute'.freeze + CUSTOM_ATTRIBUTE_INVALID_NAME_WARNING = "A custom attribute name must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters can be letters, underscores, digits (0-9), or dollar signs ($)".freeze + CUSTOM_ATTRIBUTE_VALID_NAME_REGEXP = /\A[\p{Alpha}_][\p{Alpha}_\d\$]*\z/ + included do has_many :custom_attributes, :as => :resource, :dependent => :destroy has_many :miq_custom_attributes, -> { where(:source => 'EVM') }, :as => :resource, :dependent => :destroy, :class_name => "CustomAttribute" @@ -38,8 +41,13 @@ def self.load_custom_attributes_for(cols) custom_attributes.each { |custom_attribute| add_custom_attribute(custom_attribute) } end + def self.invalid_custom_attribute_message(attribute) + "Invalid custom attribute: '#{attribute}'. #{CUSTOM_ATTRIBUTE_INVALID_NAME_WARNING}" + end + def self.add_custom_attribute(custom_attribute) return if respond_to?(custom_attribute) + ActiveSupport::Deprecation.warn(invalid_custom_attribute_message(custom_attribute)) unless custom_attribute.to_s =~ CUSTOM_ATTRIBUTE_VALID_NAME_REGEXP ca_sym = custom_attribute.to_sym without_prefix = custom_attribute.sub(CUSTOM_ATTRIBUTES_PREFIX, "") @@ -102,6 +110,7 @@ def miq_custom_get(key) def miq_custom_set(key, value) return miq_custom_delete(key) if value.blank? + ActiveSupport::Deprecation.warn(self.class.invalid_custom_attribute_message(key)) unless key.to_s =~ self.class::CUSTOM_ATTRIBUTE_VALID_NAME_REGEXP record = miq_custom_attributes.find_by(:name => key.to_s) if record.nil? diff --git a/spec/models/mixins/custom_attribute_mixin_spec.rb b/spec/models/mixins/custom_attribute_mixin_spec.rb index d350559759d..8273fda0474 100644 --- a/spec/models/mixins/custom_attribute_mixin_spec.rb +++ b/spec/models/mixins/custom_attribute_mixin_spec.rb @@ -93,6 +93,40 @@ def self.name; "TestClass"; end end end + context ".add_custom_attribute" do + it "regular key" do + test_class.add_custom_attribute("foo") + expect(test_class.new).to respond_to(:foo) + expect(test_class.new).to respond_to(:foo=) + end + + it "key with a letter followed by a number" do + test_class.add_custom_attribute("fun4all") + expect(test_class.new).to respond_to(:"fun4all") + expect(test_class.new).to respond_to(:"fun4all=") + end + + it "key with a space(deprecated)" do + ActiveSupport::Deprecation.silence { test_class.add_custom_attribute("exit message") } + expect(test_class.new).to respond_to(:"exit message") + expect(test_class.new).to respond_to(:"exit message=") + end + + it "key with leading number(deprecated)" do + ActiveSupport::Deprecation.silence { test_class.add_custom_attribute("4fun") } + expect(test_class.new).to respond_to(:"4fun") + expect(test_class.new).to respond_to(:"4fun=") + end + end + + it "#miq_custom_set with a space(deprecated)" do + object = test_class.create! + ActiveSupport::Deprecation.silence { object.miq_custom_set("hello world", "baz") } + ca = CustomAttribute.find_by(:resource_type => test_class.name, :resource_id => object.id) + expect(ca.name).to eq("hello world") + expect(ca.value).to eq("baz") + end + it "#miq_custom_set" do supported_factories.each do |factory_name| object = FactoryBot.create(factory_name)