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

Don't commit associations on assign; defer them until save. #529

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cheald
Copy link
Member

cheald commented Jul 5, 2013

This breaks existing functionality, but is probably correct. Since it's not backwards compatible, I wanted @jnunemaker to look it over.

The general problem here is described in #218, and is that assigning associations immediately saves the parent document and the association. This is both surprising and inconsistent with how it's done in other ORMs, and I'd argue that it needs to change.

This fix defers association saves/nullifies until the owner record is saved (but after it is validated!) - this means the deprecation of :dependent => :delete. The current implementation makes no sense at all, IMO, since :delete infers "I don't want this model to do its normal teardown". Additionally, this means that assigning an association to nil will not immediately delete the associated record, which is extremely dangerous behavior, IMO - a simple assignment should never destroy data in the database!

The basic fix here is to just defer association commits to before_save and eliminate delete operating on associated records. Travis is happy with it, but I did remove the specs pertaining to the eliminated behavior to get it there :)

Anyhow, please review and let me know what you think. If the current behavior is there for a reason that I'm not comprehending, I'd love to find a way to improve it.

@cheald

This comment has been minimized.

Copy link
Member

cheald commented Jul 5, 2013

Okay, so I'm tired and pushed this before looking at the many assocations. I'll need to fix those up to have similar behavior.

@cheald

This comment has been minimized.

Copy link
Member

cheald commented Jul 5, 2013

This would resolve #233, as well.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 8, 2013

Coverage Status

Coverage decreased (-0%) when pulling dddb653 on cheald:defer_association_save into c7856e4 on jnunemaker:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 9, 2013

Coverage Status

Coverage decreased (-0%) when pulling 47f3a52 on cheald:defer_association_save into f001de0 on jnunemaker:master.

@leifcr leifcr referenced this pull request Aug 20, 2013

Open

Rails4 compatability #493

@kaichen

This comment has been minimized.

Copy link

kaichen commented Nov 22, 2013

Merge it Merge it!!

This code works for me, please merge and release it in beta version let more people to test it.

@smtlaissezfaire

This comment has been minimized.

Copy link
Contributor

smtlaissezfaire commented Nov 17, 2014

Interesting - this only works on one associations and not on many associations (note that << and friends save the parent when assigning to new documents, but don't when assigning with =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment