This project is archived and is in readonly mode.

#1178 ✓stale
Aliaksey Kandratsenka

Re-implemented support for updating a belongs to association from the foreign key

Reported by Aliaksey Kandratsenka | October 6th, 2008 @ 06:13 PM

I've encountered a bug in this new feature. While fixing it I realized that there're a whole bunch of bugs.

The proposed patch has tests for all found bugs and a complete reimplementation of this feature which fixes those bugs.

Comments and changes to this ticket

  • Michael Koziarski

    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.

  • Aliaksey Kandratsenka

    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

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

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

  • Pratik

    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

    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.

  • Aliaksey Kandratsenka

    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

    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?

  • Aliaksey Kandratsenka
  • Aliaksey Kandratsenka

    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.

  • Michael Koziarski

    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?

  • Aliaksey Kandratsenka

    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

    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?

  • Aliaksey Kandratsenka

    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

    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

    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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages