Add validation onto ActiveRel from/to class arguments. Don't allow co…

…nstants and make sure model class is ActiveNode. DRYing out into ClassArguments module

CHANGELOG.md

@@ -8,8 +8,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 
 - When a `model_class` is specified on an association which is not an ActiveNode model, an error is raised
-- The `model_class` option on associations can no longer be a `Class` object (should be a String, Symbol, nil, false, or an Array of Symbols/Strings)
-- The `rel_class` option on associations can no longer be a `Class` object (should be a String, Symbol, or nil)
+- The `model_class` option on associations can no longer be a `Class` constant (should be a String, Symbol, nil, false, or an Array of Symbols/Strings)
+- The `rel_class` option on associations can no longer be a `Class` constant (should be a String, Symbol, or nil)
+- The `from_class` and `to_class` arguments can no longer be a `Class` constant (should be a String, Symbol, :any, or false)
 
 ### Added
 

lib/neo4j/active_node/has_n/association.rb

@@ -1,4 +1,5 @@
 require 'active_support/inflector/inflections'
+require 'neo4j/class_arguments'
 
 module Neo4j
   module ActiveNode
@@ -32,7 +33,7 @@ def refresh_model_class!
           @pending_model_refresh = @target_classes_or_nil = nil
 
           # Using #to_s on purpose here to take care of classes/strings/symbols
-          @model_class = @model_class.to_s.constantize if @model_class
+          @model_class = ClassArguments.constantize_argument(@model_class.to_s) if @model_class
         end
 
         def queue_model_refresh!
@@ -67,7 +68,7 @@ def target_class_names
         end
 
         def target_classes
-          target_class_names.map(&:constantize)
+          target_class_names.map(&ClassArguments.method(:constantize_argument))
         end
 
         def target_classes_or_nil
@@ -83,25 +84,19 @@ def target_where_clause
         end
 
         def discovered_model
-          target_class_names.map(&:constantize).select do |constant|
+          target_classes.select do |constant|
             constant.ancestors.include?(::Neo4j::ActiveNode)
           end
         end
 
         def target_class
           return @target_class if @target_class
 
-          if target_class_names && target_class_names.size == 1
-            class_const = target_class_names[0].constantize
+          return if !(target_class_names && target_class_names.size == 1)
 
-            if !class_const.included_modules.include?(Neo4j::ActiveNode)
-              fail ArgumentError, "Invalid argument to `#{name}` association: `#{class_const.inspect}` is not an ActiveNode model"
-            end
+          class_const = ClassArguments.constantize_argument(target_class_names[0])
 
-            @target_class = class_const
-          end
-        rescue NameError
-          raise ArgumentError, "Invalid argument to `#{name}` association: Could not find class `#{target_class_names[0]}`"
+          @target_class = class_const
         end
 
         def callback(type)
@@ -215,15 +210,14 @@ def validate_init_arguments(type, direction, name, options)
         VALID_ASSOCIATION_OPTION_KEYS = [:type, :origin, :model_class, :rel_class, :dependent, :before, :after, :unique]
 
         def validate_association_options!(_association_name, options)
+          ClassArguments.validate_argument!(options[:model_class], 'model_class')
+          ClassArguments.validate_argument!(options[:rel_class], 'rel_class')
+
           message = case
                     when (message = type_keys_error_message(options.keys))
                       message
                     when (unknown_keys = options.keys - VALID_ASSOCIATION_OPTION_KEYS).size > 0
                       "Unknown option(s) specified: #{unknown_keys.join(', ')}"
-                    when !rel_class_valid?(options[:rel_class])
-                      'rel_class option must by String, Symbol, or nil'
-                    when !model_class_valid?(options[:model_class])
-                      'model_class option must by String, Symbol, false, nil, or an Array of Symbols/Strings'
                     end
 
           fail ArgumentError, message if message
@@ -238,15 +232,6 @@ def type_keys_error_message(keys)
           end
         end
 
-        def model_class_valid?(model_class)
-          [NilClass, String, Symbol, FalseClass].include?(model_class.class) ||
-            (model_class.is_a?(Array) && model_class.all? { |c| [Symbol, String].include?(c.class) })
-        end
-
-        def rel_class_valid?(rel_class)
-          [NilClass, String, Symbol].include?(rel_class.class)
-        end
-
         def check_valid_type_and_dir(type, direction)
           fail ArgumentError, "Invalid association type: #{type.inspect} (valid value: :has_many and :has_one)" if ![:has_many, :has_one].include?(type.to_sym)
           fail ArgumentError, "Invalid direction: #{direction.inspect} (valid value: :out, :in, and :both)" if ![:out, :in, :both].include?(direction.to_sym)

lib/neo4j/active_rel/property.rb

@@ -1,3 +1,5 @@
+require 'neo4j/class_arguments'
+
 module Neo4j::ActiveRel
   module Property
     extend ActiveSupport::Concern
@@ -55,13 +57,23 @@ def id_property_name
 
       %w(to_class from_class).each do |direction|
         define_method("#{direction}") do |argument = nil|
-          return self.instance_variable_get("@#{direction}") if argument.nil?
-          instance_variable_set("@#{direction}", argument)
+          if !argument.nil?
+            Neo4j::ClassArguments.validate_argument!(argument, direction)
+
+            instance_variable_set("@#{direction}", argument)
+          end
+
+          self.instance_variable_get("@#{direction}")
         end
 
         define_method("_#{direction}") { instance_variable_get "@#{direction}" }
       end
 
+      def valid_class_argument?(class_argument)
+        [String, Symbol, FalseClass].include?(class_argument.class) ||
+          (class_argument.is_a?(Array) && class_argument.all? { |c| [String, Symbol].include?(c.class) })
+      end
+
       alias_method :start_class,  :from_class
       alias_method :end_class,    :to_class
 

lib/neo4j/active_rel/query.rb

@@ -73,9 +73,7 @@ def cypher_label(dir = :outbound)
 
       def as_constant(given_class)
         case given_class
-        when String
-          given_class.constantize
-        when Symbol
+        when String, Symbol
           given_class.to_s.constantize
         when Array
           fail "ActiveRel query methods are being deprecated and do not support Array (from|to)_class options. Current value: #{given_class}"

lib/neo4j/class_arguments.rb

@@ -0,0 +1,39 @@
+module Neo4j
+  module ClassArguments
+    class << self
+      INVALID_CLASS_ARGUMENT_ERROR = 'option must by String, Symbol, false, nil, or an Array of Symbols/Strings'
+
+      def valid_argument?(class_argument)
+        [NilClass, String, Symbol, FalseClass].include?(class_argument.class) ||
+          (class_argument.is_a?(Array) && class_argument.all? { |c| [Symbol, String].include?(c.class) })
+      end
+
+      def validate_argument!(class_argument, context)
+        return if valid_argument?(class_argument)
+
+        fail ArgumentError, "#{context} #{INVALID_CLASS_ARGUMENT_ERROR} (was #{class_argument.inspect})"
+      end
+
+      def active_node_model?(class_constant)
+        class_constant.included_modules.include?(Neo4j::ActiveNode)
+      end
+
+      def constantize_argument(class_argument)
+        case class_argument
+        when 'any', :any, false, nil
+          nil
+        when Array
+          class_argument.map(&method(:constantize_argument))
+        else
+          class_argument.to_s.constantize.tap do |class_constant|
+            if !active_node_model?(class_constant)
+              fail ArgumentError, "#{class_constant} is not an ActiveNode model"
+            end
+          end
+        end
+      rescue NameError
+        raise ArgumentError, "Could not find class: #{class_argument}"
+      end
+    end
+  end
+end

spec/e2e/active_rel/persistence/query_factory_spec.rb

@@ -15,8 +15,8 @@
 
     stub_active_rel_class('RelClass') do
       type 'HAS_REL'
-      from_class FromClass
-      to_class ToClass
+      from_class :FromClass
+      to_class :ToClass
 
       property :score
 

spec/e2e/active_rel/unpersisted_spec.rb

@@ -48,8 +48,8 @@ def log_after
     end
 
     stub_active_rel_class('MyRelClass') do
-      from_class FromClass
-      to_class ToClass
+      from_class :FromClass
+      to_class :ToClass
       type 'rel_class_type'
 
       property :score, type: Integer

spec/e2e/active_rel_spec.rb

@@ -12,6 +12,7 @@
       property :after_run
 
       has_many :out, :others, model_class: 'ToClass', rel_class: 'MyRelClass'
+      has_many :out, :other_others, rel_class: 'MyRelClass'
 
       def log_before
         self.before_run = true
@@ -31,6 +32,8 @@ def log_after
       has_many :in, :others, model_class: 'FromClass', rel_class: 'MyRelClass'
       has_many :in, :string_others, model_class: 'FromClass', rel_class: 'MyRelClass'
 
+      has_many :out, :other_others, rel_class: 'MyRelClass'
+
       def log_before
         self.before_run = true
       end
@@ -41,8 +44,8 @@ def log_after
     end
 
     stub_active_rel_class('MyRelClass') do
-      from_class FromClass
-      to_class ToClass
+      from_class :FromClass
+      to_class :ToClass
       type 'rel_class_type'
 
       property :score, type: Integer
@@ -59,18 +62,24 @@ def log_after
 
   describe 'from_class, to_class' do
     it 'spits back the current variable if no argument is given' do
-      expect(MyRelClass.from_class).to eq FromClass
-      expect(MyRelClass.to_class).to eq ToClass
+      expect(MyRelClass.from_class).to eq :FromClass
+      expect(MyRelClass.to_class).to eq :ToClass
     end
 
     it 'sets the value with the argument given' do
-      expect(MyRelClass.from_class).not_to eq Object
-      expect(MyRelClass.from_class(Object)).to eq Object
-      expect(MyRelClass.from_class).to eq Object
+      expect(MyRelClass.from_class).not_to eq :Object
+      expect(MyRelClass.from_class(:Object)).to eq :Object
+      expect(MyRelClass.from_class).to eq :Object
+
+      expect(MyRelClass.to_class).not_to eq :Object
+      expect(MyRelClass.to_class(:Object)).to eq :Object
+      expect(MyRelClass.to_class).to eq :Object
+    end
+
+    it 'validates that the class is ' do
+      MyRelClass.to_class(:Object)
 
-      expect(MyRelClass.to_class).not_to eq Object
-      expect(MyRelClass.to_class(Object)).to eq Object
-      expect(MyRelClass.to_class).to eq Object
+      expect { FromClass.create.other_others.to_a }.to raise_error ArgumentError, /Object is not an ActiveNode model/
     end
   end
 
@@ -91,8 +100,8 @@ def log_after
   describe 'creation' do
     before(:each) do
       stub_active_rel_class('RelClassWithValidations') do
-        from_class FromClass
-        to_class ToClass
+        from_class :FromClass
+        to_class :ToClass
         type 'rel_class_type'
 
         property :score
@@ -258,15 +267,15 @@ def self.it_is_expected_to_satisfy(class_method_value)
       it_is_expected_to_satisfy(:FromClass)
 
       class FromClass; end
-      it_is_expected_to_satisfy(FromClass)
+      class ToClass; end
 
       context 'Array' do
         # stub_const does not behave with `it_is_expected_to_satisfy` so making it explicit for now...
         class OtherAcceptableClass
           include Neo4j::ActiveNode
         end
 
-        it_is_expected_to_satisfy([OtherAcceptableClass, FromClass])
+        it_is_expected_to_satisfy([:OtherAcceptableClass, :FromClass])
       end
     end
   end
@@ -431,7 +440,7 @@ class ActiveRelSpecTypesInheritedRelClass < ActiveRelSpecTypesAutomaticRelType
       end
 
       context 'array of from/to class' do
-        before { MyRelClass.from_class([FromClass, ToClass]) }
+        before { MyRelClass.from_class([:FromClass, :ToClass]) }
 
         it 'joins with ||' do
           next if Neo4j::VERSION >= '6.0.0'

spec/e2e/query_spec.rb

@@ -52,8 +52,8 @@ def self.ordered_by_subject
     end
 
     stub_active_rel_class('IsEnrolledFor') do
-      from_class Student
-      to_class Lesson
+      from_class :Student
+      to_class :Lesson
       type 'is_enrolled_for'
 
       property :grade, type: Integer
@@ -127,13 +127,13 @@ def self.ordered_by_subject
       context 'Assumed model does not exist' do
         before(:each) { Bar.has_many :out, :foosrs, origin: :bars }
 
-        it { expect { subject.foosrs.to_a }.to raise_error(NameError) }
+        it { expect { subject.foosrs.to_a }.to raise_error(ArgumentError, /Could not find class.*Foosr/) }
       end
 
       context 'Specified model does not exist' do
         before(:each) { Bar.has_many :out, :foosrs, model_class: 'Foosrs', origin: :bars }
 
-        it { expect { subject.foosrs.to_a }.to raise_error(NameError) }
+        it { expect { subject.foosrs.to_a }.to raise_error(ArgumentError, /Could not find class.*Foosrs/) }
       end
 
       context 'Origin does not exist' do

spec/e2e/wrapped_transactions_spec.rb

@@ -14,8 +14,8 @@
     end
 
     stub_active_rel_class('StudentTeacher') do
-      from_class Student
-      to_class Teacher
+      from_class :Student
+      to_class :Teacher
       type 'teacher'
       property :appreciation, type: Integer
     end

spec/unit/active_node/has_n/association_spec.rb

@@ -323,7 +323,7 @@ def self.name
           context 'outbound' do
             before(:each) do
               stub_const 'Fizzl', Class.new { include Neo4j::ActiveNode }
-              TheRel.to_class(Fizzl)
+              TheRel.to_class(:Fizzl)
             end
 
             it { should == ['::Fizzl'] }
@@ -334,7 +334,7 @@ def self.name
 
             before(:each) do
               stub_const 'Buzz', Class.new { include Neo4j::ActiveNode }
-              TheRel.from_class(Buzz)
+              TheRel.from_class(:Buzz)
             end
 
             it { should == ['::Buzz'] }
@@ -344,7 +344,7 @@ def self.name
     end
 
     describe 'target_class' do
-      subject { association.target_class }
+      subject { association.target_classes }
 
       let(:options) { {type: nil, model_class: 'BadClass'} }
 
@@ -433,8 +433,8 @@ def self.name
     describe 'refresh_model_class!' do
       context 'with model class set' do
         before do
-          association.instance_variable_set(:@model_class, named_class('MyModel'))
-          stub_named_class('MyModel')
+          stub_active_node_class('MyModel')
+          association.instance_variable_set(:@model_class, 'MyModel')
         end
 
         it 'changes the value of #derive_model_class' do

spec/unit/active_rel/property_spec.rb

@@ -51,10 +51,10 @@ def self.name
         it 'sets @from_class and @to_class' do
           expect(clazz.instance_variable_get(:@from_class)).to be_nil
           expect(clazz.instance_variable_get(:@to_class)).to be_nil
-          clazz.from_class Object
-          clazz.to_class Object
-          expect(clazz.instance_variable_get(:@from_class)).to eq Object
-          expect(clazz.instance_variable_get(:@to_class)).to eq Object
+          clazz.from_class :Object
+          clazz.to_class :Object
+          expect(clazz.instance_variable_get(:@from_class)).to eq :Object
+          expect(clazz.instance_variable_get(:@to_class)).to eq :Object
         end
       end
     end