Association new.save vs. create
Reported by Tony Pitale | June 7th, 2009 @ 01:55 AM
Demonstration:
Many to Many through associations do not create the join records required when doing #new and then #save. However, #create works as expected.
Comments and changes to this ticket
-
Dan Kubb (dkubb) June 7th, 2009 @ 04:18 AM
- State changed from “unconfirmed” to “accepted”
- Assigned user set to “Dan Kubb (dkubb)”
-
Tony Pitale June 9th, 2009 @ 02:02 PM
Is there anything I can be doing on this? I tried looking through the code and the specs and I think it'll take me a little to familiarize myself with it. Any pointers on where to look, or how this might be spec'd and I can take a crack at it.
-
Dan Kubb (dkubb) June 10th, 2009 @ 01:04 PM
Tony: I'm not 100% sure yet. Normally when you create a Resource through a ManyToMany::Collection, it'll create the intermediary records automatically. In this case the collection isn't doing the creation at all, but rather the Resource.
For this to work I think we're going to have to add some logic to the Resource so that just prior to saving, it tells the Collection it is part of that it wants to save, allowing the Collection to do whatever it needs to set the storage state properly, like create intermediaries, etc. In most cases, like a normal DM::Collection, or a OneToMany::Collection this "callback" will not do anything, only the ManyToMany::Collection.
This is the first thing that comes to mind when I think about this problem, but I haven't had a chance yet to test it out.
Before doing this though, it would be nice to have a pending spec in place that reproduces the problem, so that when we start to work on it we can make sure it begins passing properly.
-
Tony Pitale June 10th, 2009 @ 01:43 PM
Perhaps this is why methods like "build" exist and should continue to exist as part of the ManyToMany::Collection?
I can write the spec (probably), just not sure where it should go.
-
Dan Kubb (dkubb) June 10th, 2009 @ 01:49 PM
ManyToMany::Collection#build should be inherited from OneToMany::Collection, which inherits it from DM::Collection.
For now, perhaps fork dm-core/next, and add it into the spec/public/shared/collection_shared_spec.rb since all Collection objects will need the same basic callback mechanism. The behavior to be specced will be to save a Resource that is part of a Collection, reloading the Collection, and then making sure the new Resource is included.
Don't worry too much about optimum location yet. When it's complete and I merge it into the mainline, we can work to rearrange it then. For now I'm just focus on getting the spec and functionality working.
-
Tony Pitale June 10th, 2009 @ 02:11 PM
This would work if instead of adding the new resource to itself within Collection#new, the reverse was true. Not sure that it's possible or easy. In the example I gave above, the retailer has the item in it's collection (unsaved). If the retailer was in the item's collection then, when it is saved, the intermediary would be saved as well.
-
Dan Kubb (dkubb) June 11th, 2009 @ 12:17 AM
Tony: It might not be totally clear, but there is actually a bidirectional relationship between the Resource and a Collection. So not only does the Collection contain the Resource (as you would expect), but the Resource has a link to it's container through Resource#collection.
Inside Resource#save, it should do something like collection.save_resource(resource), to notify the Collection of it's intention to save the Resource. ManyToMany::Collection#save_resource could then save the intermediary at this time.
Ideally I would like this mechanism to be used to trigger the intermediary creation inside ManyToMany::Collection#save as well so there's only one logic path to create intermediaries rather than two.
-
Tony Pitale June 11th, 2009 @ 08:39 AM
Dan: that is good to know. I'll see what I can do, after RubyNation is over.
-
Dan Kubb (dkubb) June 16th, 2009 @ 03:05 AM
- Milestone set to “0.10.0”
-
Dan Kubb (dkubb) June 23rd, 2009 @ 02:30 AM
- Tag changed from “0.10.0, associations, join, regression” to “associations, join”
Removing regression tag because dm-core 0.9 didn't behave this way either
-
Tony Pitale June 23rd, 2009 @ 08:41 AM
Yeah, I think 0.9 had #build, and #build is now deprecated recommending #new, which doesn't behave the same way.
-
Dan Kubb (dkubb) June 23rd, 2009 @ 12:00 PM
@Tony: Not that it matters too much, but in 0.9, there was a new() and build() command, with both having the exact same outcome, except an assertion being thrown by new() if the parent/source isn't saved.
During refactoring for 0.10 I deleted new() (since it wasn't necessary for the parent/source to be saved just to initialize the object), renamed build() to new(), and added a deprecation warning for anyone trying to use build() directly. Once spec coverage was really good, I eventually found some patterns in new(), that weren't association specific, so I pushed that down into Collection and removed the now redundant new() call in 1:m and m:m collections.
I take regressions really seriously because if people are relying on behavior I accidentally removed, it could cause alot of problems for them later on. The behavior you're describing is how I think things should work, but they don't, and I there's no evidence they ever did. I traced the 0.9 code from ManyToMany down through OneToMany, and Collection, and can't see any evidence that we had bidirectional associations working. If we did, even accidentally, I would be surprised because it would require quite a non-trivial change to the architecture and design of most of Collection and other parts of dm-core that I wrote and maintain.
Just to confirm I even tried to run the script above with 0.9.11, and it explodes. Would you mind providing a script that demonstrates it working in 0.9.11? I'm I'm wrong, and the behavior was as you described, I'd like to trace through it and figure out how it worked and then try to port that to 0.10.
-
Tony Pitale June 23rd, 2009 @ 12:29 PM
No, no. You're right. It never worked. I guess I just assumed that build would handle that stuff before. I only discovered this on a new project and I'm not sure I ever got it working before I switched to 0.10.
-
Dan Kubb (dkubb) June 29th, 2009 @ 09:37 PM
@Tony: I just hit this same problem in an app.
In a create() method within my controller, I was initializing the new resource, and then calling resource.save() on it and then redirecting if successful (a typical pattern in Merb/Rails apps). The intermediary wasn't created as expected, while the target resource was.
I'm going to try to bump this up in priority, because the work-around in my app isn't as elegant as it should be. I have one vector in mind to attempt to fix this, and will add a comment on whether or not it turns out to be successful.
Attached is an example script with a simplified domain model to test against when looking into this fix.
-
Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM
- Milestone changed from “0.10.0” to “0.10.2”
[project:id#20609 not-tagged:"0.10.0" milestone:id#51895 bulk edit command]
-
Dan Kubb (dkubb) February 1st, 2010 @ 04:33 PM
- Milestone changed from “0.10.2” to “1.0.0”
-
-
-
Dan Kubb (dkubb) February 2nd, 2010 @ 02:47 AM
- State changed from “accepted” to “confirmed”
Please Sign in or create a free account to add a new ticket.
With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Dan Kubb (dkubb)
Tony Pitale