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

Lazy association creation #871

merged 14 commits into from
Jul 17, 2015

Conversation

subvertallchris
Copy link
Contributor

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. When save is called in the node, it checks for the presence of deferred associations and, if it finds any, it processes them after save completes.

It makes a few significant changes. In this case:

student = Student.create
lesson = Lesson.new
student.lessons << lesson

save will be called on the lesson immediately and the association will be processed. I know we debated this before but this is consistent with ActiveRecord.

In this case:

student = Student.new
less = Lesson.new
student.lessons << lesson
student.save

...that final save will trigger a cascading save: first student, then lesson, 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. If lesson was an existing node and student was unpersisted, it would wait for student.save to create the relationship and it would avoid the extra save on lesson.

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:

  • Investigate whether the process should reuse any ActiveRecord method names and signatures
  • Add methods that rely on this process to improve compatibility with third-party gems. An autosave association option and a QueryProxy build method immediately come to mind.

@subvertallchris
Copy link
Contributor Author

Interesting, this is failing on Neo4j 2.1.7?

@subvertallchris
Copy link
Contributor Author

ping @cheerfulstoic and @dpisarewski
Dieter, this changes your work from a few months ago that let a user automatically save unpersisted nodes when associating. It changes to the default, essentially, and removes the option to disable it.

@@ -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
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.

@olance
Copy link
Contributor

olance commented Jul 16, 2015

@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 lesson if it's added to another persisted object's collection?
Does ActiveRecord really do that too?

Anyway, I'd say it's already a great improvement and I'd be happy to be able to use it :)

@subvertallchris
Copy link
Contributor Author

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 lesson. I've always been in the "raise-an-error-and-make-them-save" camp but our previous debates showed me as the minority. General consensus was that if you try to do that, you expect to be dealing with a persisted or persistable node, so calling save first is reasonable.

@olance
Copy link
Contributor

olance commented Jul 17, 2015

hmm ok, I thought we could do nothing but add the lesson to the collection and save it only when cascading saves from the student.save, otherwise I don't really see the point of having to call it!
Calling save automatically on lesson but not saving the relationship and requiring to call save on student to do so, makes little sense to me ^^

@subvertallchris
Copy link
Contributor Author

It really depends on the state of all of the nodes. If student is unpersisted, nothing happens until you call save. If student is persisted, adding to its lessons will call save on the lesson... but all is not lost. You can always work in reverse.

lesson = Lesson.new
student = Student.first
lesson.students << student
# save has not been called anywhere

lesson.save
# now a save will trigger on student and the rel will be created

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.

@subvertallchris
Copy link
Contributor Author

Oh, and in this case:

student = Student.first
lesson = Lesson.new
student.lessons << lesson

That will immediately save the lesson and create the relationship, no save required.

@olance
Copy link
Contributor

olance commented Jul 17, 2015

oh, yeah, sorry I had mixed the two examples when reading back your post!
So I guess that's cool :)

@@ -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 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.

@cheerfulstoic
Copy link
Contributor

Seems pretty straightforward! Thanks for this!

@subvertallchris
Copy link
Contributor Author

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.

subvertallchris added a commit that referenced this pull request Jul 17, 2015
@subvertallchris subvertallchris merged commit be01fd1 into master Jul 17, 2015
@subvertallchris subvertallchris deleted the lazy_association_creation branch August 7, 2015 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants