Merge pull request #18538 from jrafanie/deprecate_invalid_column_name…

…s_in_custom_attributes

Deprecate invalid custom attribute names

(cherry picked from commit a26aa9d)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693722

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?

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 = FactoryGirl.create(factory_name)