Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy association creation #871

Merged
merged 14 commits into from
Jul 17, 2015
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Metrics/AbcSize:
# Offense count: 2
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 208
Max: 214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tsk tsk... ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done before I split things out into modules, it can be removed
now. ;-)

On Friday, July 17, 2015, Brian Underwood [email protected] wrote:

In .rubocop_todo.yml
#871 (comment):

@@ -16,7 +16,7 @@ Metrics/AbcSize:

Offense count: 2

Configuration parameters: CountComments.

Metrics/ClassLength:

  • Max: 208
  • Max: 214

Tsk tsk... ;)


Reply to this email directly or view it on GitHub
https://github.com/neo4jrb/neo4j/pull/871/files#r34912584.


# Offense count: 768
# Configuration parameters: AllowURI, URISchemes.
Expand Down
2 changes: 2 additions & 0 deletions lib/neo4j.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
require 'neo4j/active_node/dependent/query_proxy_methods'
require 'neo4j/active_node/dependent/association_methods'
require 'neo4j/active_node/query_methods'
require 'neo4j/active_node/query/query_proxy_unpersisted'
require 'neo4j/active_node/query/query_proxy_methods'
require 'neo4j/active_node/query/query_proxy_enumerable'
require 'neo4j/active_node/query/query_proxy_find_in_batches'
Expand All @@ -66,6 +67,7 @@
require 'neo4j/active_node/validations'
require 'neo4j/active_node/rels'
require 'neo4j/active_node/reflection'
require 'neo4j/active_node/unpersisted'
require 'neo4j/active_node/has_n'
require 'neo4j/active_node/has_n/association_cypher_methods'
require 'neo4j/active_node/has_n/association'
Expand Down
1 change: 1 addition & 0 deletions lib/neo4j/active_node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module ActiveNode
include Neo4j::ActiveNode::Query
include Neo4j::ActiveNode::Labels
include Neo4j::ActiveNode::Rels
include Neo4j::ActiveNode::Unpersisted
include Neo4j::ActiveNode::HasN
include Neo4j::ActiveNode::Scope
include Neo4j::ActiveNode::Dependent
Expand Down
25 changes: 9 additions & 16 deletions lib/neo4j/active_node/has_n.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,16 +157,6 @@ def previous_association_proxy_results_by_previous_id(association_proxy, associa
Hash[*query_proxy.pluck('ID(previous)', 'collect(next)').flatten(1)]
end

def handle_non_persisted_node(other_node)
return unless Neo4j::Config[:autosave_on_assignment]
other_node.try(:save)
save
end

def validate_persisted_for_association!
fail(Neo4j::ActiveNode::HasN::NonPersistedNodeError, 'Unable to create relationship with non-persisted nodes') unless self._persisted_obj
end

module ClassMethods
# :nocov:
# rubocop:disable Style/PredicateName
Expand Down Expand Up @@ -292,7 +282,7 @@ def has_one(direction, name, options = {}) # rubocop:disable Style/PredicateName

def define_has_many_methods(name)
define_method(name) do |node = nil, rel = nil, options = {}|
return [].freeze unless self._persisted_obj
# return [].freeze unless self._persisted_obj

if node.is_a?(Hash)
options = node
Expand Down Expand Up @@ -348,11 +338,14 @@ def define_has_one_getter(name)

def define_has_one_setter(name)
define_method("#{name}=") do |other_node|
handle_non_persisted_node(other_node)
validate_persisted_for_association!
association_proxy_cache.clear # TODO: Should probably just clear for this association...

Neo4j::Transaction.run { association_proxy(name).replace_with(other_node) }
if persisted?
other_node.save if other_node.respond_to?(:persisted?) && !other_node.persisted?
association_proxy_cache.clear # TODO: Should probably just clear for this association...
Neo4j::Transaction.run { association_proxy(name).replace_with(other_node) }
# handle_non_persisted_node(other_node)
else
association_proxy(name).defer_create(other_node, {}, :'=')
end
end
end

Expand Down
23 changes: 19 additions & 4 deletions lib/neo4j/active_node/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ def initialize(record)
# If any of the before_* callbacks return false the action is cancelled and save returns false.
def save(*)
update_magic_properties
association_proxy_cache.clear
create_or_update
cascade_save do
association_proxy_cache.clear
create_or_update
end
end

# Persist the object to the database. Validations and Callbacks are included
Expand Down Expand Up @@ -53,7 +55,7 @@ def create_model(*)
node = _create_node(properties)
init_on_load(node, node.props)
send_props(@relationship_props) if @relationship_props
@relationship_props = nil
@relationship_props = @deferred_nodes = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Rubocop really OK about this?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so. It's really sensitive about the length of things, so I tend to try pushing out before down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, it's just that I've had an error on something similar once, but actually it was on a parallel assignment.

true
end

Expand All @@ -66,12 +68,25 @@ def _create_node(*args)
session.create_node(props, labels)
end

private

# The pending associations are cleared during the save process, so it's necessary to
# build the processable hash before it begins. If there are nodes and associations that
# need to be created after the node is saved, a new transaction is started.
def cascade_save
deferred_nodes = pending_associations_with_nodes
Neo4j::Transaction.run(!deferred_nodes.blank?) do
result = yield
process_unpersisted_nodes!(deferred_nodes) if deferred_nodes
result
end
end

module ClassMethods
# Creates and saves a new node
# @param [Hash] props the properties the new node should have
def create(props = {})
association_props = extract_association_attributes!(props) || {}

new(props).tap do |obj|
yield obj if block_given?
obj.save
Expand Down
4 changes: 2 additions & 2 deletions lib/neo4j/active_node/query/query_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class QueryProxy
include Neo4j::ActiveNode::Query::QueryProxyMethods
include Neo4j::ActiveNode::Query::QueryProxyFindInBatches
include Neo4j::ActiveNode::Query::QueryProxyEagerLoading
include Neo4j::ActiveNode::Query::QueryProxyUnpersisted
include Neo4j::ActiveNode::Dependent::QueryProxyMethods

# The most recent node to start a QueryProxy chain.
Expand Down Expand Up @@ -159,8 +160,7 @@ def to_cypher_with_params(columns = [self.identity])

# To add a relationship for the node for the association on this QueryProxy
def <<(other_node)
create(other_node, {})

@start_object._persisted_obj ? create(other_node, {}) : defer_create(other_node, {}, :<<)
self
end

Expand Down
17 changes: 17 additions & 0 deletions lib/neo4j/active_node/query/query_proxy_unpersisted.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module Neo4j
module ActiveNode
module Query
module QueryProxyUnpersisted
def defer_create(other_nodes, _properties, operator)
key = [@association.name, [nil, nil, nil]].hash
@start_object.pending_associations[key] = [@association.name, operator]
if @start_object.association_proxy_cache[key]
@start_object.association_proxy_cache[key] << other_nodes
else
@start_object.association_proxy_cache[key] = [other_nodes]
end
end
end
end
end
end
49 changes: 49 additions & 0 deletions lib/neo4j/active_node/unpersisted.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module Neo4j
module ActiveNode
module Unpersisted
def pending_associations
@pending_associations ||= {}
end

def pending_associations?
!@pending_associations.blank?
end

private

# TODO: Change this method's name.
# Takes the pending_associations hash, which is in the format { cache_key => [:association_name, :association_operator]},
# and returns them as { association_name => [[nodes_for_persistence], :operator] }
def pending_associations_with_nodes
return unless pending_associations?
{}.tap do |deferred_nodes|
pending_associations.each_pair do |cache_key, (association_name, operator)|
nodes_for_creation = self.persisted? ? association_proxy_cache[cache_key].select { |n| !n.persisted? } : association_proxy_cache[cache_key]
deferred_nodes[association_name] = [nodes_for_creation, operator]
end
end
end

# @param [Hash] deferred_nodes A hash created by :pending_associations_with_nodes
def process_unpersisted_nodes!(deferred_nodes)
deferred_nodes.each_pair do |k, (v, o)|
save_and_associate_queue(k, v, o)
end
end


def save_and_associate_queue(association_name, node_queue, operator)
association_proc = proc { |node| save_and_associate_node(association_name, node, operator) }
node_queue.each do |element|
element.is_a?(Array) ? element.each { |node| association_proc.call(node) } : association_proc.call(element)
end
end

def save_and_associate_node(association_name, node, operator)
node.save if node.changed? || !node.persisted?
fail "Unable to defer node persistence, could not save #{node.inspect}" unless node.persisted?
operator == :<< ? send(association_name).send(operator, node) : send(:"#{association_name}=", node)
end
end
end
end
5 changes: 1 addition & 4 deletions spec/e2e/has_many_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,12 @@
it { is_expected.to include(:friends, :knows, :knows_me) }
end

# See unpersisted_association_spec.rb for more related tests
describe 'non-persisted node' do
let(:unsaved_node) { Person.new }
it 'returns an empty array' do
expect(unsaved_node.friends).to eq []
end

it 'has a frozen array' do
expect { unsaved_node.friends << friend1 }.to raise_error(RuntimeError)
end
end

describe 'unique: true' do
Expand Down
19 changes: 2 additions & 17 deletions spec/e2e/has_one_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,13 @@
end
end

# See unpersisted_association_spec.rb for additional tests related to this
context 'with non-persisted node' do
let(:unsaved_node) { HasOneB.new }

it 'returns a nil object' do
expect(unsaved_node.parent).to eq nil
end

it 'raises an error when trying to create a relationship' do
expect { unsaved_node.parent = HasOneA.create }.to raise_error(Neo4j::ActiveNode::HasN::NonPersistedNodeError)
end

context 'with enabled auto-saving' do
let_config(:autosave_on_assignment) { true }

it 'saves the node' do
expect { unsaved_node.parent = HasOneA.create }.to change(unsaved_node, :persisted?).from(false).to(true)
end

it 'saves the associated node' do
other_node = HasOneA.new
expect { unsaved_node.parent = other_node }.to change(other_node, :persisted?).from(false).to(true)
end
end
end

it 'find the nodes via the has_one accessor' do
Expand Down
Loading