[WIP] refactor SupportsFeatureMixin to do less metaprogramming

app/models/mixins/supports_feature_mixin.rb

@@ -102,38 +102,77 @@ module SupportsFeatureMixin
     :update_network_router      => 'Network Router Update',
   }.freeze
 
-  # Whenever this mixin is included we define all features as unsupported by default.
-  # This way we can query for every feature
   included do
     QUERYABLE_FEATURES.keys.each do |feature|
-      supports_not(feature)
+      method_name = "supports_#{feature}?"
+
+      # defines the method on the instance
+      define_method(method_name) do
+        supports?(feature)
+      end
+
+      # defines the method on the class
+      define_singleton_method(method_name) do
+        supports?(feature)
+      end
     end
   end
 
   class UnknownFeatureError < StandardError; end
 
+  class FeatureDefinition
+    def initialize(supported: false, block: nil, unsupported_reason: nil)
+      @supported = supported
+      @block = block
+      @unsupported_reason = unsupported_reason
+    end
+
+    def supported?
+      @supported
+    end
+
+    def unsupported_reason
+      SupportsFeatureMixin.reason_or_default(@unsupported_reason) unless supported?
+    end
+
+    def block
+      @block
+    end
+  end
+
   def self.guard_queryable_feature(feature)
     unless QUERYABLE_FEATURES.key?(feature.to_sym)
       raise UnknownFeatureError, "Feature ':#{feature}' is unknown to SupportsFeatureMixin."
     end
   end
 
-  def self.reason_or_default(reason)
+  def self.reason_or_default(reason = nil)
     reason.present? ? reason : _("Feature not available/supported")
   end
 
   # query instance for the reason why the feature is unsupported
   def unsupported_reason(feature)
     SupportsFeatureMixin.guard_queryable_feature(feature)
     feature = feature.to_sym
-    public_send("supports_#{feature}?") unless unsupported.key?(feature)
+    supports?(feature) unless unsupported.key?(feature)
     unsupported[feature]
   end
 
   # query the instance if the feature is supported or not
   def supports?(feature)
     SupportsFeatureMixin.guard_queryable_feature(feature)
-    public_send("supports_#{feature}?")
+
+    feature = feature.to_sym
+    feature_definition = self.class.feature_definition_for(feature)
+    if feature_definition.supported?
+      unsupported.delete(feature)
+      if feature_definition.block
+        instance_eval(&feature_definition.block)
+      end
+    else
+      unsupported_reason_add(feature, feature_definition.unsupported_reason)
+    end
+    !unsupported.key?(feature)
   end
 
   # query the instance if a feature is generally known
@@ -159,72 +198,50 @@ def unsupported
     # This is the DSL used a class level to define what is supported
     def supports(feature, &block)
       SupportsFeatureMixin.guard_queryable_feature(feature)
-      define_supports_feature_methods(feature, &block)
+      supported_feature_definitions[feature.to_sym] = FeatureDefinition.new(supported: true, block: block)
     end
 
     # supports_not does not take a block, because its never supported
     # and not conditionally supported
     def supports_not(feature, reason: nil)
       SupportsFeatureMixin.guard_queryable_feature(feature)
-      define_supports_feature_methods(feature, :is_supported => false, :reason => reason)
+      supported_feature_definitions[feature.to_sym] = FeatureDefinition.new(unsupported_reason: reason)
     end
 
     # query the class if the feature is supported or not
     def supports?(feature)
       SupportsFeatureMixin.guard_queryable_feature(feature)
-      public_send("supports_#{feature}?")
+      feature_definition_for(feature).supported?
     end
 
     # query the class for the reason why something is unsupported
     def unsupported_reason(feature)
       SupportsFeatureMixin.guard_queryable_feature(feature)
-      feature = feature.to_sym
-      public_send("supports_#{feature}?") unless unsupported.key?(feature)
-      unsupported[feature]
+      feature_definition_for(feature).unsupported_reason
     end
 
     # query the class if a feature is generally known
     def feature_known?(feature)
       SupportsFeatureMixin::QUERYABLE_FEATURES.key?(feature.to_sym)
     end
 
-    private
-
-    def unsupported
-      # This is a class variable and it might be modified during runtime
-      # because we dont eager load all classes at boot time, so it needs to be thread safe
-      @unsupported ||= Concurrent::Hash.new
-    end
-
-    # use this for making a class not support a feature
-    def unsupported_reason_add(feature, reason = nil)
-      SupportsFeatureMixin.guard_queryable_feature(feature)
+    def feature_definition_for(feature)
       feature = feature.to_sym
-      unsupported[feature] = SupportsFeatureMixin.reason_or_default(reason)
+      feature_definition = nil
+      ancestors.detect do |ancestor|
+        feature_definition = ancestor.try(:supported_feature_definition, feature)
+      end
+      feature_definition || FeatureDefinition.new
     end
 
-    def define_supports_feature_methods(feature, is_supported: true, reason: nil, &block)
-      method_name = "supports_#{feature}?"
-      feature = feature.to_sym
+    def supported_feature_definition(feature)
+      supported_feature_definitions[feature]
+    end
 
-      # defines the method on the instance
-      define_method(method_name) do
-        unsupported.delete(feature)
-        if block_given?
-          instance_eval(&block)
-        else
-          unsupported_reason_add(feature, reason) unless is_supported
-        end
-        !unsupported.key?(feature)
-      end
+    private
 
-      # defines the method on the class
-      define_singleton_method(method_name) do
-        unsupported.delete(feature)
-        # TODO: durandom - make reason evaluate in class context, to e.g. include the name of a subclass (.to_proc?)
-        unsupported_reason_add(feature, reason) unless is_supported
-        !unsupported.key?(feature)
-      end
+    def supported_feature_definitions
+      @supported_feature_definitions ||= Concurrent::Hash.new
     end
   end
 end

spec/models/manageiq/providers/redhat/infra_manager_spec.rb

@@ -156,46 +156,45 @@
     let(:supported_api_versions) { [3, 4] }
     context "#process_api_features_support" do
       before(:each) do
-        allow(SupportsFeatureMixin).to receive(:guard_queryable_feature).and_return(true)
         allow(described_class).to receive(:api_features)
-          .and_return('3' => %w(feature1 feature3), '4' => %w(feature2 feature3))
+          .and_return('3' => %w(delete suspend), '4' => %w(create suspend))
         described_class.process_api_features_support
         allow(ems).to receive(:supported_api_versions).and_return(supported_api_versions)
       end
 
       context "no versions supported" do
         let(:supported_api_versions) { [] }
         it 'supports the right features' do
-          expect(ems.supports_feature1?).to be_falsey
-          expect(ems.supports_feature2?).to be_falsey
-          expect(ems.supports_feature3?).to be_falsey
+          expect(ems.supports_delete?).to be_falsey
+          expect(ems.supports_create?).to be_falsey
+          expect(ems.supports_suspend?).to be_falsey
         end
       end
 
       context "version 3 supported" do
         let(:supported_api_versions) { [3] }
         it 'supports the right features' do
-          expect(ems.supports_feature1?).to be_truthy
-          expect(ems.supports_feature2?).to be_falsey
-          expect(ems.supports_feature3?).to be_truthy
+          expect(ems.supports_delete?).to be_truthy
+          expect(ems.supports_create?).to be_falsey
+          expect(ems.supports_suspend?).to be_truthy
         end
       end
 
       context "version 4 supported" do
         let(:supported_api_versions) { [4] }
         it 'supports the right features' do
-          expect(ems.supports_feature1?).to be_falsey
-          expect(ems.supports_feature2?).to be_truthy
-          expect(ems.supports_feature3?).to be_truthy
+          expect(ems.supports_delete?).to be_falsey
+          expect(ems.supports_create?).to be_truthy
+          expect(ems.supports_suspend?).to be_truthy
         end
       end
 
       context "all versions supported" do
         let(:supported_api_versions) { [3, 4] }
         it 'supports the right features' do
-          expect(ems.supports_feature1?).to be_truthy
-          expect(ems.supports_feature2?).to be_truthy
-          expect(ems.supports_feature3?).to be_truthy
+          expect(ems.supports_delete?).to be_truthy
+          expect(ems.supports_create?).to be_truthy
+          expect(ems.supports_suspend?).to be_truthy
         end
       end
     end

spec/models/mixins/supports_feature_mixin_spec.rb

@@ -262,10 +262,14 @@ class MegaPost
   end
 
   context "feature that is implicitly unsupported" do
-    it "class responds to supports_feature?" do
+    it "is unsupported by a class" do
       expect(Post.supports_nuke?).to be false
     end
 
+    it "is unsupported by an instance of the class" do
+      expect(Post.new.supports_nuke?).to be false
+    end
+
     it "can be supported by the class" do
       stub_const("NukeablePost", Class.new(SpecialPost) do
         supports :nuke do
@@ -276,4 +280,33 @@ class MegaPost
       expect(NukeablePost.new(:bribe => false).supports_nuke?).to be true
     end
   end
+
+  context 'included in a module' do
+    before do
+      stub_const('MyModule', Module.new do
+        include SupportsFeatureMixin
+        supports_not :publish  # supported in base class Post
+        supports :fake do      # unsupported in base class Post
+          unsupported_reason_add(:fake, "We never fake")
+        end
+      end)
+      stub_const('SomePost', Class.new(Post) do
+        include MyModule
+      end)
+    end
+
+    it "does not affect undefined features" do
+      expect(SomePost.new.supports_delete?).to be false
+      expect(SomePost.supports_delete?).to be false
+    end
+
+    it "overrides features defined on the class with those in the module" do
+      expect(SomePost.new.supports_publish?).to be false
+      expect(SomePost.supports_publish?).to be false
+
+      expect(SomePost.supports_fake?).to be true
+      expect(SomePost.new.supports_fake?).to be false
+      expect(SomePost.new.unsupported_reason(:fake)).to eq('We never fake')
+    end
+  end
 end