From 56f1cb51e2c5824296a1c59fb463026c59d20afd Mon Sep 17 00:00:00 2001 From: Chris Grigg Date: Thu, 30 Apr 2015 11:48:49 -0400 Subject: [PATCH 1/2] started separating property management logic --- lib/neo4j.rb | 2 + lib/neo4j/active_node/initialize.rb | 3 +- lib/neo4j/active_node/persistence.rb | 5 +- lib/neo4j/active_rel/initialize.rb | 3 +- lib/neo4j/active_rel/persistence.rb | 2 +- lib/neo4j/shared.rb | 1 + lib/neo4j/shared/declared_property.rb | 62 ++++++++++++ lib/neo4j/shared/declared_property_manager.rb | 94 +++++++++++++++++++ lib/neo4j/shared/persistence.rb | 11 ++- lib/neo4j/shared/property.rb | 68 ++++---------- lib/neo4j/shared/serialized_properties.rb | 20 ---- lib/neo4j/shared/type_converters.rb | 29 ++---- spec/e2e/active_model_spec.rb | 10 +- spec/unit/shared/property_spec.rb | 8 +- 14 files changed, 212 insertions(+), 106 deletions(-) create mode 100644 lib/neo4j/shared/declared_property.rb create mode 100644 lib/neo4j/shared/declared_property_manager.rb diff --git a/lib/neo4j.rb b/lib/neo4j.rb index 94b512ee9..e4f08c330 100644 --- a/lib/neo4j.rb +++ b/lib/neo4j.rb @@ -26,6 +26,8 @@ require 'neo4j/paginated' require 'neo4j/shared/callbacks' +require 'neo4j/shared/declared_property' +require 'neo4j/shared/declared_property_manager' require 'neo4j/shared/property' require 'neo4j/shared/persistence' require 'neo4j/shared/validations' diff --git a/lib/neo4j/active_node/initialize.rb b/lib/neo4j/active_node/initialize.rb index 1aca4ffbe..0d67d28ca 100644 --- a/lib/neo4j/active_node/initialize.rb +++ b/lib/neo4j/active_node/initialize.rb @@ -1,6 +1,5 @@ module Neo4j::ActiveNode::Initialize extend ActiveSupport::Concern - include Neo4j::Shared::TypeConverters attr_reader :called_by # called when loading the node from the database @@ -13,7 +12,7 @@ def init_on_load(persisted_node, properties) attr = @attributes || self.class.attributes_nil_hash.dup @attributes = attr.merge!(properties).stringify_keys! self.default_properties = properties - @attributes = convert_properties_to :ruby, @attributes + @attributes = self.class.declared_property_manager.convert_properties_to(self, :ruby, @attributes) end # Implements the Neo4j::Node#wrapper and Neo4j::Relationship#wrapper method diff --git a/lib/neo4j/active_node/persistence.rb b/lib/neo4j/active_node/persistence.rb index cef482d53..5d50f3d62 100644 --- a/lib/neo4j/active_node/persistence.rb +++ b/lib/neo4j/active_node/persistence.rb @@ -10,6 +10,7 @@ def initialize(record) end extend ActiveSupport::Concern + extend Forwardable include Neo4j::Shared::Persistence # Saves the model. @@ -48,13 +49,11 @@ def create_model(*) create_magic_properties set_timestamps create_magic_properties - properties = convert_properties_to :db, props + properties = self.class.declared_property_manager.convert_properties_to(self, :db, props) node = _create_node(properties) init_on_load(node, node.props) send_props(@relationship_props) if @relationship_props @relationship_props = nil - # Neo4j::IdentityMap.add(node, self) - # write_changed_relationships true end diff --git a/lib/neo4j/active_rel/initialize.rb b/lib/neo4j/active_rel/initialize.rb index 4b8ccd53f..fd17100d4 100644 --- a/lib/neo4j/active_rel/initialize.rb +++ b/lib/neo4j/active_rel/initialize.rb @@ -1,7 +1,6 @@ module Neo4j::ActiveRel module Initialize extend ActiveSupport::Concern - include Neo4j::Shared::TypeConverters attr_reader :_persisted_obj @@ -17,7 +16,7 @@ def init_on_load(persisted_rel, from_node_id, to_node_id, type) @attributes = attributes.merge(persisted_rel.props.stringify_keys) load_nodes(from_node_id, to_node_id) self.default_properties = persisted_rel.props - @attributes = convert_properties_to :ruby, @attributes + @attributes = self.class.declared_property_manager.convert_properties_to(self, :ruby, @attributes) end # Implements the Neo4j::Node#wrapper and Neo4j::Relationship#wrapper method diff --git a/lib/neo4j/active_rel/persistence.rb b/lib/neo4j/active_rel/persistence.rb index e93ada987..3ef6f1cd5 100644 --- a/lib/neo4j/active_rel/persistence.rb +++ b/lib/neo4j/active_rel/persistence.rb @@ -22,7 +22,7 @@ def create_model(*) validate_node_classes! create_magic_properties set_timestamps - properties = convert_properties_to :db, props + properties = self.class.declared_property_manager.convert_properties_to(self, :db, props) rel = _create_rel(from_node, to_node, properties) return self unless rel.respond_to?(:_persisted_obj) init_on_load(rel._persisted_obj, from_node, to_node, @rel_type) diff --git a/lib/neo4j/shared.rb b/lib/neo4j/shared.rb index 9919560b9..7d3eb9903 100644 --- a/lib/neo4j/shared.rb +++ b/lib/neo4j/shared.rb @@ -28,6 +28,7 @@ def neo4j_session included do self.include_root_in_json = Neo4j::Config.include_root_in_json + @_declared_property_manager ||= Neo4j::Shared::DeclaredPropertyManager.new(self) def self.i18n_scope :neo4j diff --git a/lib/neo4j/shared/declared_property.rb b/lib/neo4j/shared/declared_property.rb new file mode 100644 index 000000000..958033dca --- /dev/null +++ b/lib/neo4j/shared/declared_property.rb @@ -0,0 +1,62 @@ +module Neo4j::Shared + # Contains methods related to the management + class DeclaredProperty + class IllegalPropertyError < StandardError; end + + ILLEGAL_PROPS = %w(from_node to_node start_node end_node) + attr_reader :name, :name_string, :name_sym, :options, :magic_typecaster + + def initialize(name, options) + fail IllegalPropertyError, "#{name} is an illegal property" if ILLEGAL_PROPS.include?(name.to_s) + @name = @name_sym = name + @name_string = name.to_s + @options = options + end + + def register + register_magic_properties + end + + def type + options[:type] + end + + def typecaster + options[:typecaster] + end + + def default_value + options[:default] + end + + private + + # Tweaks properties + def register_magic_properties + options[:type] ||= DateTime if name.to_sym == :created_at || name.to_sym == :updated_at + # TODO: Custom typecaster to fix the stuff below + # ActiveAttr does not handle "Time", Rails and Neo4j.rb 2.3 did + # Convert it to DateTime in the interest of consistency + options[:type] = DateTime if options[:type] == Time + + register_magic_typecaster + register_type_converter + end + + def register_magic_typecaster + found_typecaster = Neo4j::Shared::TypeConverters.typecaster_for(options[:type]) + return unless found_typecaster && found_typecaster.respond_to?(:primitive_type) + options[:typecaster] = found_typecaster + @magic_typecaster = options[:type] + options[:type] = found_typecaster.primitive_type + end + + def register_type_converter + converter = options[:serializer] + return unless converter + options[:type] = converter.convert_type + options[:typecaster] = ActiveAttr::Typecasting::ObjectTypecaster.new + Neo4j::Shared::TypeConverters.register_converter(converter) + end + end +end diff --git a/lib/neo4j/shared/declared_property_manager.rb b/lib/neo4j/shared/declared_property_manager.rb new file mode 100644 index 000000000..4d9345e0c --- /dev/null +++ b/lib/neo4j/shared/declared_property_manager.rb @@ -0,0 +1,94 @@ +module Neo4j::Shared + class DeclaredPropertyManager + include Neo4j::Shared::TypeConverters + + attr_reader :klass + + def initialize(klass) + @klass = klass + end + + def register(property) + @_attributes_nil_hash = nil + registered_properties[property.name] = property + register_magic_typecaster(property) if property.magic_typecaster + declared_property_defaults[property.name] = property.default_value if property.default_value + end + + def declared_property_defaults + @_default_property_values ||= {} + end + + def registered_properties + @_registered_properties ||= {} + end + + def attributes_nil_hash + @_attributes_nil_hash ||= {}.tap { |attr_hash| registered_properties.each_pair { |k, _v| attr_hash[k.to_s] = nil } }.freeze + end + + def unregister(name) + # might need to be include?(name.to_s) + fail ArgumentError, "Argument `#{name}` not an attribute" if not registered_properties[name] + declared_prop = registered_properties[name] + registered_properties.delete(declared_prop) + unregister_magic_typecaster(name) + unregister_property_default(name) + end + + def serialize(name, coder = JSON) + @serialize ||= {} + @serialize[name] = coder + end + + def serialized_properties=(serialize_hash) + @serialized_property_keys = nil + @serialize = serialize_hash.clone + end + + def serialized_properties + @serialize ||= {} + end + + def serialized_properties_keys + @serialized_property_keys ||= serialized_properties.keys + end + + def magic_typecast_properties_keys + @magic_typecast_properties_keys ||= magic_typecast_properties.keys + end + + def magic_typecast_properties + @magic_typecast_properties ||= {} + end + + # The known mappings of declared properties and their primitive types. + def upstream_primitives + @upstream_primitives ||= {} + end + + protected + + # Prevents repeated calls to :_attribute_type, which isn't free and never changes. + def fetch_upstream_primitive(attr) + upstream_primitives[attr] || upstream_primitives[attr] = klass._attribute_type(attr) + end + + private + + def unregister_magic_typecaster(property) + magic_typecast_properties.delete(property) + @magic_typecast_properties_keys = nil + end + + def unregister_property_default(property) + declared_property_defaults.delete(property) + @_default_property_values = nil + end + + def register_magic_typecaster(property) + magic_typecast_properties[property.name] = property.magic_typecaster + @magic_typecast_properties_keys = nil + end + end +end diff --git a/lib/neo4j/shared/persistence.rb b/lib/neo4j/shared/persistence.rb index eda8abeb1..b534464c7 100644 --- a/lib/neo4j/shared/persistence.rb +++ b/lib/neo4j/shared/persistence.rb @@ -1,7 +1,6 @@ module Neo4j::Shared module Persistence extend ActiveSupport::Concern - include Neo4j::Shared::TypeConverters USES_CLASSNAME = [] @@ -9,7 +8,7 @@ def update_model return if !changed_attributes || changed_attributes.empty? changed_props = attributes.select { |k, _| changed_attributes.include?(k) } - changed_props = convert_properties_to :db, changed_props + changed_props = self.class.declared_property_manager.convert_properties_to(self, :db, changed_props) _persisted_obj.update_props(changed_props) changed_attributes.clear end @@ -33,6 +32,7 @@ def update_attribute!(attribute, value) def create_or_update # since the same model can be created or updated twice from a relationship we have to have this guard @_create_or_updating = true + apply_default_values result = _persisted_obj ? update_model : create_model if result == false Neo4j::Transaction.current.failure if Neo4j::Transaction.current @@ -47,6 +47,13 @@ def create_or_update @_create_or_updating = nil end + def apply_default_values + return if self.class.declared_property_defaults.empty? + self.class.declared_property_defaults.each_pair do |key, value| + self.send("#{key}=", value) if self.send(key).nil? + end + end + # Returns +true+ if the record is persisted, i.e. it's not a new record and it was not destroyed def persisted? !new_record? && !destroyed? diff --git a/lib/neo4j/shared/property.rb b/lib/neo4j/shared/property.rb index 1680c3b03..69f1aee31 100644 --- a/lib/neo4j/shared/property.rb +++ b/lib/neo4j/shared/property.rb @@ -11,9 +11,7 @@ module Property class UndefinedPropertyError < RuntimeError; end class MultiparameterAssignmentError < StandardError; end - class IllegalPropertyError < StandardError; end - - ILLEGAL_PROPS = %w(from_node to_node start_node end_node) + # @_declared_property_manager = DeclaredPropertyManager.new attr_reader :_persisted_obj @@ -123,6 +121,10 @@ def instantiate_object(field, values_with_empty_parameters) end module ClassMethods + extend Forwardable + + def_delegators :declared_property_manager, :serialized_properties, :serialized_properties=, :serialize, :declared_property_defaults + # Defines a property on the class # # See active_attr gem for allowed options, e.g which type @@ -152,21 +154,25 @@ module ClassMethods # property :name, constraint: :unique # end def property(name, options = {}) - @_attributes_nil_hash = nil - check_illegal_prop(name) - magic_properties(name, options) - attribute(name, options) + prop = DeclaredProperty.new(name, options) + prop.register + declared_property_manager.register(prop) + + attribute(name, prop.options) constraint_or_index(name, options) end def undef_property(name) - fail ArgumentError, "Argument `#{name}` not an attribute" if not attribute_names.include?(name.to_s) - + declared_property_manager.unregister(name) attribute_methods(name).each { |method| undef_method(method) } - undef_constraint_or_index(name) end + def declared_property_manager + @_declared_property_manager ||= DeclaredPropertyManager.new(self) + end + + # TODO: Move this to the DeclaredPropertyManager def default_property(name, &block) reset_default_properties(name) if default_properties.respond_to?(:size) default_properties[name] = block @@ -208,15 +214,7 @@ def attribute!(name, options = {}) # @return [Hash] A frozen hash of all model properties with nil values. It is used during node loading and prevents # an extra call to a slow dependency method. def attributes_nil_hash - @_attributes_nil_hash ||= {}.tap { |attr_hash| attribute_names.each { |k, _v| attr_hash[k.to_s] = nil } }.freeze - end - - def magic_typecast_properties - @magic_typecast_properties ||= {} - end - - def magic_typecast_properties_keys - @magic_typecast_properties_keys ||= magic_typecast_properties.keys + declared_property_manager.attributes_nil_hash end private @@ -231,38 +229,6 @@ def constraint_or_index(name, options) index(name, options) if options[:index] == :exact end end - - def check_illegal_prop(name) - fail IllegalPropertyError, "#{name} is an illegal property" if ILLEGAL_PROPS.include?(name.to_s) - end - - # Tweaks properties - def magic_properties(name, options) - magic_typecast(name, options) - type_converter(options) - options[:type] ||= DateTime if name.to_sym == :created_at || name.to_sym == :updated_at - - # ActiveAttr does not handle "Time", Rails and Neo4j.rb 2.3 did - # Convert it to DateTime in the interest of consistency - options[:type] = DateTime if options[:type] == Time - end - - def type_converter(options) - converter = options[:serializer] - return unless converter - options[:type] = converter.convert_type - options[:typecaster] = ActiveAttr::Typecasting::ObjectTypecaster.new - Neo4j::Shared::TypeConverters.register_converter(converter) - end - - def magic_typecast(name, options) - typecaster = Neo4j::Shared::TypeConverters.typecaster_for(options[:type]) - return unless typecaster && typecaster.respond_to?(:primitive_type) - magic_typecast_properties[name] = options[:type] - @magic_typecast_properties_keys = nil - options[:type] = typecaster.primitive_type - options[:typecaster] = typecaster - end end end end diff --git a/lib/neo4j/shared/serialized_properties.rb b/lib/neo4j/shared/serialized_properties.rb index 126416d6b..9e469939e 100644 --- a/lib/neo4j/shared/serialized_properties.rb +++ b/lib/neo4j/shared/serialized_properties.rb @@ -14,25 +14,5 @@ def serialized_properties def serializable_hash(*args) super.merge(id: id) end - - module ClassMethods - def serialized_properties - @serialize ||= {} - end - - def serialized_properties_keys - @serialized_property_keys ||= serialized_properties.keys - end - - def serialized_properties=(serialize_hash) - @serialized_property_keys = nil - @serialize = serialize_hash.clone - end - - def serialize(name, coder = JSON) - @serialize ||= {} - @serialize[name] = coder - end - end end end diff --git a/lib/neo4j/shared/type_converters.rb b/lib/neo4j/shared/type_converters.rb index ce14efcb2..a0c49cbd5 100644 --- a/lib/neo4j/shared/type_converters.rb +++ b/lib/neo4j/shared/type_converters.rb @@ -107,10 +107,10 @@ def to_ruby(value) end end - def convert_properties_to(medium, properties) + def convert_properties_to(obj, medium, properties) converter = medium == :ruby ? :to_ruby : :to_db properties.each_pair do |attr, value| - next if skip_conversion?(attr, value) + next if skip_conversion?(obj, attr, value) properties[attr] = converted_property(primitive_type(attr.to_sym), value, converter) end end @@ -124,30 +124,18 @@ def converted_property(type, value, converter) # If the attribute is to be typecast using a custom converter, which converter should it use? If no, returns the type to find a native serializer. def primitive_type(attr) case - when self.class.serialized_properties_keys.include?(attr) + when self.serialized_properties_keys.include?(attr) serialized_properties[attr] - when self.class.magic_typecast_properties_keys.include?(attr) - self.class.magic_typecast_properties[attr] + when self.magic_typecast_properties_keys.include?(attr) + self.magic_typecast_properties[attr] else - self.class.fetch_upstream_primitive(attr) + self.fetch_upstream_primitive(attr) end end # Returns true if the property isn't defined in the model or it's both nil and unchanged. - def skip_conversion?(attr, value) - !self.class.attributes[attr] || (value.nil? && !changed_attributes[attr]) - end - - module ClassMethods - # Prevents repeated calls to :_attribute_type, which isn't free and never changes. - def fetch_upstream_primitive(attr) - upstream_primitives[attr] || upstream_primitives[attr] = self._attribute_type(attr) - end - - # The known mappings of declared properties and their primitive types. - def upstream_primitives - @upstream_primitives ||= {} - end + def skip_conversion?(obj, attr, value) + !obj.class.attributes[attr] || (value.nil? && !obj.changed_attributes[attr]) end class << self @@ -162,6 +150,7 @@ def included(_) end end + def typecaster_for(primitive_type) return nil if primitive_type.nil? converters.key?(primitive_type) ? converters[primitive_type] : nil diff --git a/spec/e2e/active_model_spec.rb b/spec/e2e/active_model_spec.rb index 299216ee6..a39ee06aa 100644 --- a/spec/e2e/active_model_spec.rb +++ b/spec/e2e/active_model_spec.rb @@ -509,6 +509,15 @@ class Company expect(School.where(name: 'The College of New Jersey').child_of.to_a).to be_empty end + describe 'default property values' do + before { Person.property(:default_prop, default: 'Chopper') } + let(:guy) { Person.create(name: 'Guy Foo') } + + it 'sets the default value if nil on persistence' do + expect(guy.default_prop).to eq 'Chopper' + end + end + describe 'multiparameter attributes' do it 'converts to Date' do person = Person.create('date(1i)' => '2014', 'date(2i)' => '7', 'date(3i)' => '13') @@ -653,7 +662,6 @@ def self.named_bill i.each { |count| Person.create(name: "Billy-#{i}", age: count) } end - after(:all) { Person.delete_all } let(:t) { Person.where } let(:p) { Neo4j::Paginated.create_from(t, 2, 5) } diff --git a/spec/unit/shared/property_spec.rb b/spec/unit/shared/property_spec.rb index 6a6f54904..6a5dc64b8 100644 --- a/spec/unit/shared/property_spec.rb +++ b/spec/unit/shared/property_spec.rb @@ -5,8 +5,8 @@ describe ':property class method' do it 'raises an error when passing illegal properties' do - Neo4j::Shared::Property::ILLEGAL_PROPS.push 'foo' - expect { clazz.property :foo }.to raise_error(Neo4j::Shared::Property::IllegalPropertyError) + Neo4j::Shared::DeclaredProperty::ILLEGAL_PROPS.push 'foo' + expect { clazz.property :foo }.to raise_error(Neo4j::Shared::DeclaredProperty::IllegalPropertyError) end end @@ -129,12 +129,12 @@ def to_ruby(value) it 'uses type converter to serialize node' do instance.range = range - expect(instance.convert_properties_to(:db, instance.props)[:range]).to eq(range.to_s) + expect(instance.class.declared_property_manager.convert_properties_to(instance, :db, instance.props)[:range]).to eq(range.to_s) end it 'uses type converter to deserialize node' do instance.range = range.to_s - expect(instance.convert_properties_to(:ruby, instance.props)[:range]).to eq(range) + expect(instance.class.declared_property_manager.convert_properties_to(instance, :ruby, instance.props)[:range]).to eq(range) end end end From 4b986acb06b30d2a0354613f9a1bcabd1e3f728b Mon Sep 17 00:00:00 2001 From: Chris Grigg Date: Mon, 4 May 2015 14:37:33 -0400 Subject: [PATCH 2/2] some basic specs for new property mgmt classes --- lib/neo4j/shared.rb | 4 ++ lib/neo4j/shared/declared_property.rb | 2 +- spec/e2e/property_management_spec.rb | 81 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 spec/e2e/property_management_spec.rb diff --git a/lib/neo4j/shared.rb b/lib/neo4j/shared.rb index 7d3eb9903..ba03213c5 100644 --- a/lib/neo4j/shared.rb +++ b/lib/neo4j/shared.rb @@ -34,5 +34,9 @@ def self.i18n_scope :neo4j end end + + def declared_property_manager + self.class.declared_property_manager + end end end diff --git a/lib/neo4j/shared/declared_property.rb b/lib/neo4j/shared/declared_property.rb index 958033dca..d0be52024 100644 --- a/lib/neo4j/shared/declared_property.rb +++ b/lib/neo4j/shared/declared_property.rb @@ -6,7 +6,7 @@ class IllegalPropertyError < StandardError; end ILLEGAL_PROPS = %w(from_node to_node start_node end_node) attr_reader :name, :name_string, :name_sym, :options, :magic_typecaster - def initialize(name, options) + def initialize(name, options = {}) fail IllegalPropertyError, "#{name} is an illegal property" if ILLEGAL_PROPS.include?(name.to_s) @name = @name_sym = name @name_string = name.to_s diff --git a/spec/e2e/property_management_spec.rb b/spec/e2e/property_management_spec.rb new file mode 100644 index 000000000..831685f30 --- /dev/null +++ b/spec/e2e/property_management_spec.rb @@ -0,0 +1,81 @@ +require 'spec_helper' + +describe 'declared property classes' do + describe Neo4j::Shared::DeclaredProperty do + before do + clazz = Class.new do + def primitive_type; end + end + + stub_const('MyTypeCaster', clazz) + end + + let(:clazz) { Neo4j::Shared::DeclaredProperty } + + describe Neo4j::Shared::DeclaredProperty do + context 'illegal property names' do + it 'raises an error' do + expect { clazz.new(:from_node) }.to raise_error { Neo4j::Shared::DeclaredProperty::IllegalPropertyError } + end + end + + describe 'options' do + let(:prop) { clazz.new(:my_prop, type: String, typecaster: MyTypeCaster, default: 'foo') } + + it 'controls method responses' do + expect(prop.type).to eq String + expect(prop.typecaster).to eq MyTypeCaster + expect(prop.default_value).to eq 'foo' + end + end + + describe 'magic properties' do + let(:created) { clazz.new(:created_at) } + let(:updated) { clazz.new(:updated_at) } + + it 'automatically sets type for created_at and updated_at if unset' do + expect(created.type).to be_nil + expect(updated.type).to be_nil + [created, updated].each(&:register) + + expect(created.type).to eq DateTime + expect(updated.type).to eq DateTime + end + end + end + end + + describe Neo4j::Shared::DeclaredPropertyManager do + before do + clazz = Class.new do + include Neo4j::ActiveNode + property :foo + property :bar, type: String, default: 'foo' + end + + stub_const('MyModel', clazz) + end + + let(:model) { MyModel } + let(:dpm) { MyModel.declared_property_manager } + + it 'is included on each class' do + expect(model.declared_property_manager).to be_a(Neo4j::Shared::DeclaredPropertyManager) + end + + it 'has a convenience method on each instance' do + inst = model.new + expect(inst.declared_property_manager.object_id).to eq model.declared_property_manager.object_id + end + + it 'contains information about each declared property' do + [:foo, :bar].each do |key| + expect(dpm.registered_properties[key]).to be_a(Neo4j::Shared::DeclaredProperty) + end + end + + it 'keeps a default hash of nil values for use in initial object wrapping' do + expect(dpm.attributes_nil_hash).to eq('foo' => nil, 'bar' => nil) + end + end +end