Ah yeah I see what you are saying. Also I think that is reasonable to keep things like they are not only because existing code might expect that but because you can explicitly pass the reciprocal object to the associated one if you need it to be available.
I think your solution makes sense, it also passes our tests. Thanks for digging into this and for the explanations! On Thursday, December 13, 2018 at 3:35:39 PM UTC-8, Jeremy Evans wrote: > > On Thursday, December 13, 2018 at 1:34:00 PM UTC-8, Ben Alavi wrote: >> >> Actually... do you know why the reciprocal object isn't just replaced >> *before* calling the setter? >> >> This works for my example and passes specs: >> >> remove_reciprocal_object(opts, a) if a >> add_reciprocal_object(opts, o) if o >> # Allow calling private _setter method >> send(opts[:_setter_method], o) >> > > This would break symmetry with how the association caches are modified in > all other cases. Modifying the associations cache to set the new value > happens after the related add/remove code is run, and reciprocal > modifications happen after that. In the code posted, the associations > cache for the current object is modified after calling the _setter_method. > Other cases follow a similar pattern. add_associated_object runs the > add_method, then modifies the current association cache, then modifies the > reciprocal. remove_associated_object runs the remove_method, the modifies > the current associations cache, then modifies the reciprocal. Now, that > isn't a reason to reject this approach outright, but there would need to be > a compelling reason to make such a change. > > In the general case (when nested attributes is not used), at the point the > save method is called on the associated object (o), the associated object > in o (the reciprocal) wouldn't match the current database value, which > would result in different behavior than in most other cases where at the > point the save method is called, the association object matches the > database value. While no specs currently fail with the change described, I > expect it may cause issues in some applications that rely on the current > order of operations. > > Contrived Example: > > DB.create_table(:nodes) do > primary_key :id > Integer :parent_id > end > > class Node < Sequel::Model > many_to_one :parent, :class=>self > one_to_one :child, :key=>:parent_id, :class=>self > > # Check existing parent to ensure new value is greater > def parent_id=(v) > if parent > raise "bad parent" unless v > parent.id > end > super > end > end > > Node.import([:id], [[1], [2], [3]]) > > x = Node[1] > y = Node[2] > z = Node[3] > y.parent = x > y.parent = z > > x = Node[1] > y = Node[2] > z = Node[3] > x.child = y > z.child = y > > Thanks, > Jeremy > -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/sequel-talk. For more options, visit https://groups.google.com/d/optout.
