-
Notifications
You must be signed in to change notification settings - Fork 276
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
Conversation
Interesting, this is failing on Neo4j 2.1.7? |
ping @cheerfulstoic and @dpisarewski |
@@ -53,7 +72,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 |
There was a problem hiding this comment.
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?!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@subvertallchris have you deleted your last comment? ^^ I meant to chime in earlier but got quite busy lately... so apparently you've debated about this already, but could you give the rational behind automatically saving Anyway, I'd say it's already a great improvement and I'd be happy to be able to use it :) |
Haha, you caught me! I didn't want to be too pushy... publicly. ;-) I poked Brian directly cause his internet connection is iffy at the moment. Believe it or not, ActiveRecord does really immediately save |
hmm ok, I thought we could do nothing but add the lesson to the collection and save it only when cascading saves from the |
It really depends on the state of all of the nodes. If
So there's that. As far as I can see, the basics mirror ActiveRecord, but it's possible there's a configuration option available that changes this or maybe I misinterpreted something along the way. |
Oh, and in this case:
That will immediately save the lesson and create the relationship, no |
oh, yeah, sorry I had mixed the two examples when reading back your post! |
@@ -16,7 +16,7 @@ Metrics/AbcSize: | |||
# Offense count: 2 | |||
# Configuration parameters: CountComments. | |||
Metrics/ClassLength: | |||
Max: 208 | |||
Max: 214 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tsk tsk... ;)
There was a problem hiding this comment.
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 notifications@github.com 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.
Seems pretty straightforward! Thanks for this! |
Very cool. Thanks for checking it out, guys. Even though it's somewhat simple, it feels like a big change to established behavior, so I appreciate having the extra sets of eyes on it. |
Lazy association creation
This is far from ideal but it will hopefully get the ball rolling in regard to working with associations and unpersisted nodes.
It makes a few key changes that are demonistrated in https://github.com/neo4jrb/neo4j/blob/lazy_association_creation/spec/e2e/unpersisted_association_spec.rb.
The basics are simple: when adding nodes to a has_many or has_one rel using
<<
or=
, it checks whether the nodes are persisted. If they are not, it will stash the nodes to associate in the unpersisted node's association_cache. Whensave
is called in the node, it checks for the presence of deferred associations and, if it finds any, it processes them aftersave
completes.It makes a few significant changes. In this case:
save
will be called on thelesson
immediately and the association will be processed. I know we debated this before but this is consistent with ActiveRecord.In this case:
...that final
save
will trigger a cascading save: firststudent
, thenlesson
, then the association is created. It will all occur within a transaction, so a failure of any piece of the operation will roll back the others. Iflesson
was an existing node andstudent
was unpersisted, it would wait forstudent.save
to create the relationship and it would avoid the extrasave
onlesson
.This could probably use a full more tests, so I'm going to come back to it tomorrow; in particular, I want to make sure that it works correctly when you give it an array or multiple nodes in separate method calls.
My biggest complaint here is that the whole process is rather inefficient. Each operation happens within a separate trip to the DB, so there's no real performance benefit when compared to manually managing transactions and the order of operations.
But there are benefits. The best thing this does is let you worry less about the order of your operations and the transaction handling is convenient. Performing multiple operations within a transaction is a bit faster than independent operations, so this will be safer and faster than what many people might be doing.
Aside from improving coverage a little, the next steps would be:
autosave
association option and a QueryProxybuild
method immediately come to mind.