Rails Ruby on Rails

Comments and changes to this ticket

• 

  Michael Koziarski October 7th, 2008 @ 08:27 PM

  The write_attribute case isn't particularly interesting to me, if you're using that you're 'painting outside the lines'

  However the case which covers multiple belongs_to associations definitely is more interesting and something that should be addressed.

  Seems it should be possible to take that one generated foo_id= method and make it figure out the different associations which use that id and invalidate them all?

  Failing that we should revert the feature.

• You flagged this item as spam.
  

  Aliaksey Kandratsenka October 8th, 2008 @ 07:05 AM

  • Tag changed from “activerecord, associations, belongs_to, bug, patch” to “activerecord, associations, belongs_to, bug, patch”

  That's possible aproach. And I considered that. But I think that write_attribute is important. Otherwise it won't be able to cleanly override attribute setter by user.

• 

  Michael Koziarski October 10th, 2008 @ 04:06 PM

  OK, I'm going to revert this functionality till post 2.2

• 

  Pratik January 11th, 2009 @ 02:56 PM

  • Milestone cleared.
  • Assigned user set to “Michael Koziarski”
  • Title changed from “[PATCH] re-implemented support for updating a belongs to association from the foreign key” to “Re-implemented support for updating a belongs to association from the foreign key”
• 

  Michael Koziarski January 12th, 2009 @ 05:59 AM

  • State changed from “new” to “incomplete”

  I'd like to do this, but by adding a writer that invalidates all the cached associations.

• You flagged this item as spam.
  

  Aliaksey Kandratsenka January 12th, 2009 @ 07:12 AM

  I still think that doing this in attribute writer is not ok.

  When someone overrides attribute writer he/she uses 'write_attribute' and not 'super' to invoke original implementation. If only attribute writer has association validation logic, then we have no way to override foreign keys writers.

• 

  Michael Koziarski January 12th, 2009 @ 07:44 AM

  hmm, that's a compelling point.

  Can you give us an updated rebased patch with a commit message explaining the feature rather than comparing it to the previous implementation?

• You flagged this item as spam.
  

  Aliaksey Kandratsenka January 12th, 2009 @ 07:54 AM

  Ok. Will do that today.

• You flagged this item as spam.
  

  Aliaksey Kandratsenka January 13th, 2009 @ 09:08 AM

  Uploading. But I'm not happy with it right now. The problem is that there are many places where association is read (via #instance_variable_get), and all that places need to be modified to check for association stale-ness. I'm uploading it in as-is state, where not all such places are covered.

  Extra work needs to be done if this approach is taken. It seems that we need generic association reader method that in addition forcing reload will be able to skip reload. And we'll need to convert #instance_variable_get invokations to invokations of this new method.

  • 0001--re-implemented-support-for-updating-a-belongs-to-as.patch 8 KB
• 

  Michael Koziarski January 15th, 2009 @ 08:36 PM

  Could we perhaps push this logic into the proxy class itself? It knows the owner and the attribute that it matches?

  It'd make it a lot less intrusive than having to change everywhere?

• You flagged this item as spam.
  

  Aliaksey Kandratsenka January 16th, 2009 @ 06:43 AM

  Change detection is indeed placed in proxy in my patch. But the code which should run this detection and drop the proxy should not IMO belong to proxy. That's because we cannot use #reset and must simply drop reference on old proxy.

  I'll try to complete this work this weekend. I think there's no other way to support this feature.

• 

  Michael Koziarski January 16th, 2009 @ 07:28 AM

  What's the reason that we have to drop the reference to the old proxy rather than replacing the target internally? Seems more encapsulated that way?

• You flagged this item as spam.
  

  Aliaksey Kandratsenka January 16th, 2009 @ 07:42 AM

  Consider this:

  parent = record.parent

  old_id = parent.id

  record.parent_id = new_id

  assert_equal old_id, parent.id # will not fail

  record.parent # here parent discovers that foreign key change and turns to completely other object

  assert_equal old_id, parent.id # will fail

  The reason is that proxy is often used instead of it's target object and it's mostly transparent. Reseting proxy will look like mysterious change of object's identity.

  Original implementation had same problem due to use of #reset.

  BTW, because belong_to proxies are loaded eagerly (to be able to return nil, which cannot be emulated via #method_missing magic) we can try to return proxy's target instead of proxy itself. Actually, this way it looks much better and this approach should be tried.

• 

  Michael Koziarski January 26th, 2009 @ 03:41 AM

  Right, the eager loading for nil support is a bloody good point :)

  Give that a go and see what the patch comes out like, it sounds like it'll possibly be a little nicer.

• 

  Rohit Arondekar October 6th, 2010 @ 06:44 AM

  • State changed from “incomplete” to “stale”
  • Importance changed from “” to “”

  Marking ticket as stale. If this is still an issue please leave a comment with suggested changes, creating a patch with tests, rebasing an existing patch or just confirming the issue on a latest release or master/branches.