Merge pull request ManageIQ#15893 from lfu/generic_object_enhancement

Generic object enhancement.

app/models/generic_object.rb

@@ -8,7 +8,7 @@ class GenericObject < ApplicationRecord
   delegate :property_attribute_defined?,
            :property_defined?,
            :type_cast,
-           :property_associations, :property_association_defined?,
+           :property_association_defined?,
            :property_methods, :property_method_defined?,
            :to => :generic_object_definition, :allow_nil => true
 
@@ -35,31 +35,48 @@ def property_attributes
     end
   end
 
+  def property_associations
+    properties.select { |k, _| property_association_defined?(k) }.each_with_object({}) do |(k, _), h|
+      h[k] = _property_getter(k)
+    end
+  end
+
   def delete_property(name)
-    properties.delete(name.to_s)
-    save!
+    if !property_attribute_defined?(name) && !property_association_defined?(name)
+      valid_property_names = generic_object_definition.property_attributes.keys + generic_object_definition.property_associations.keys
+      raise "Invalid property [#{name}]: must be one of #{valid_property_names.join(", ")}"
+    end
+
+    _property_getter(name).tap do
+      properties.delete(name.to_s)
+      save!
+    end
   end
 
   def add_to_property_association(name, objs)
     objs = [objs] unless objs.kind_of?(Array)
     name = name.to_s
     properties[name] ||= []
 
-    klass = property_associations[name].constantize
+    klass = generic_object_definition.property_associations[name].constantize
     selected = objs.select { |obj| obj.kind_of?(klass) }
     properties[name] = (properties[name] + selected.pluck(:id)).uniq if selected
-    save
+    save!
   end
 
   def delete_from_property_association(name, objs)
     objs = [objs] unless objs.kind_of?(Array)
     name = name.to_s
     properties[name] ||= []
 
-    klass = property_associations[name].constantize
+    klass = generic_object_definition.property_associations[name].constantize
     selected = objs.select { |obj| obj.kind_of?(klass) }
-    properties[name] = properties[name] - selected.pluck(:id)
-    save
+    common_ids = properties[name] & selected.pluck(:id)
+    properties[name] = properties[name] - common_ids
+    return unless properties_changed?
+
+    save!
+    klass.where(:id => common_ids).to_a
   end
 
   def inspect
@@ -68,7 +85,7 @@ def inspect
     end
 
     attributes_as_string += ["attributes: #{property_attributes}"]
-    attributes_as_string += ["associations: #{property_associations.keys}"]
+    attributes_as_string += ["associations: #{generic_object_definition.property_associations.keys}"]
     attributes_as_string += ["methods: #{property_methods}"]
 
     prefix = Kernel.instance_method(:inspect).bind(self).call.split(' ', 2).first
@@ -114,18 +131,19 @@ def respond_to_missing?(method_name, _include_private = false)
   end
 
   def _property_getter(name)
-    generic_object_definition.property_getter(name, properties[name])
+    generic_object_definition.property_getter(name.to_s, properties[name.to_s])
   end
 
   def _property_setter(name, value)
     name = name.to_s
+
     val =
       if property_attribute_defined?(name)
         # property attribute is of single value, for now
         type_cast(name, value)
       elsif property_association_defined?(name)
         # property association is of multiple values
-        value.select { |v| v.kind_of?(property_associations[name].constantize) }.uniq.map(&:id)
+        value.select { |v| v.kind_of?(generic_object_definition.property_associations[name].constantize) }.uniq.map(&:id)
       end
 
     self.properties = properties.merge(name => val)

app/models/generic_object_definition.rb

@@ -80,7 +80,7 @@ def property_getter(attr, val)
   end
 
   def type_cast(attr, value)
-    TYPE_MAP.fetch(property_attributes[attr]).cast(value)
+    TYPE_MAP.fetch(property_attributes[attr.to_s]).cast(value)
   end
 
   def properties=(props)
@@ -90,15 +90,15 @@ def properties=(props)
 
   def add_property_attribute(name, type)
     properties[:attributes][name.to_s] = type.to_sym
-    save
+    save!
   end
 
   def delete_property_attribute(name)
     transaction do
+      generic_objects.find_each { |o| o.delete_property(name) }
+
       properties[:attributes].delete(name.to_s)
       save!
-
-      generic_objects.find_each { |o| o.delete_property(name) }
     end
   end
 
@@ -107,32 +107,32 @@ def add_property_association(name, type)
     raise "invalid model for association: [#{type}]" unless type.in?(ALLOWED_ASSOCIATION_TYPES)
 
     properties[:associations][name.to_s] = type
-    save
+    save!
   end
 
   def delete_property_association(name)
     transaction do
+      generic_objects.find_each { |o| o.delete_property(name) }
+
       properties[:associations].delete(name.to_s)
       save!
-
-      generic_objects.find_each { |o| o.delete_property(name) }
     end
   end
 
   def add_property_method(name)
     properties[:methods] << name.to_s unless properties[:methods].include?(name.to_s)
-    save
+    save!
   end
 
   def delete_property_method(name)
     properties[:methods].delete(name.to_s)
-    save
+    save!
   end
 
   private
 
   def get_objects_of_association(attr, values)
-    property_associations[attr].constantize.where(:id => values).to_a
+    property_associations[attr.to_s].constantize.where(:id => values).to_a
   end
 
   def normalize_property_attributes

spec/models/generic_object_spec.rb

@@ -159,6 +159,15 @@
       go_assoc.save!
       expect(go_assoc.vms.count).to eq(1)
     end
+
+    it 'method returns all associations' do
+      host = FactoryGirl.create(:host)
+      go_assoc.hosts = [host]
+
+      result = go_assoc.property_associations
+      expect(result["vms"]).to match_array([vm1, vm2])
+      expect(result["hosts"]).to match_array([host])
+    end
   end
 
   describe 'property methods' do
@@ -221,4 +230,86 @@
       expect(go.my_host).to eq("some_return_value")
     end
   end
+
+  describe '#delete_property' do
+    it 'an attriute' do
+      max = go.max_number
+      expect(go.delete_property("max_number")).to eq(max)
+      expect(go.max_number).to be_nil
+    end
+
+    it 'an association' do
+      vm2 = FactoryGirl.create(:vm_vmware)
+      go.vms = [vm1, vm2]
+      expect(go.delete_property("vms")).to match_array([vm1, vm2])
+      expect(go.vms).to be_empty
+    end
+
+    it 'a method' do
+      expect { go.delete_property("my_host") }.to raise_error(RuntimeError)
+    end
+
+    it 'an invalid property name' do
+      expect { go.delete_property("some_attribute_not_defined") }.to raise_error(RuntimeError)
+    end
+  end
+
+  describe '#add_to_property_association' do
+    let(:new_vm) { FactoryGirl.create(:vm_vmware) }
+    subject { go.add_to_property_association("vms", vm1) }
+
+    it 'adds objects into association' do
+      subject
+      expect(go.vms.count).to eq(1)
+
+      go.add_to_property_association(:vms, [new_vm])
+      expect(go.vms.count).to eq(2)
+    end
+
+    it 'does not add duplicate object' do
+      subject
+      expect(go.vms.count).to eq(1)
+
+      subject
+      expect(go.vms.count).to eq(1)
+    end
+
+    it 'does not add object from differnt class' do
+      go.add_to_property_association("vms", FactoryGirl.create(:host))
+      expect(go.vms.count).to eq(0)
+    end
+
+    it 'does not accept object id' do
+      go.add_to_property_association(:vms, new_vm.id)
+      expect(go.vms.count).to eq(0)
+    end
+  end
+
+  describe '#delete_from_property_association' do
+    before { go.add_to_property_association("vms", [vm1]) }
+    let(:new_vm) { FactoryGirl.create(:vm_vmware) }
+
+    it 'deletes objects from association' do
+      result = go.delete_from_property_association(:vms, [vm1])
+      expect(go.vms.count).to eq(0)
+      expect(result).to match_array([vm1])
+    end
+
+    it 'does not delete object that is not in association' do
+      expect(go.vms.count).to eq(1)
+      result = go.delete_from_property_association(:vms, [new_vm])
+      expect(go.vms).to match_array([vm1])
+      expect(result).to be_nil
+    end
+
+    it 'does not delete object from differnt class' do
+      result = go.delete_from_property_association(:vms, [FactoryGirl.create(:host)])
+      expect(go.vms).to match_array([vm1])
+      expect(result).to be_nil
+    end
+
+    it 'does not accept object id' do
+      expect(go.delete_from_property_association(:vms, vm1.id)).to be_nil
+    end
+  end
 end