Comments and changes to this ticket

• 

  Pratik May 13th, 2008 @ 04:25 PM

  • Title changed from “[PATCH] belongs_to association not updated when assigning to foreign key” to “belongs_to association not updated when assigning to foreign key”
• 

  Pratik May 20th, 2008 @ 12:36 PM

  • Assigned user set to “Michael Koziarski”

  Not a big fan of this change. If you want the association to change, you should just assign the value using torchbox.city=. And this wouldn't work when you assign value to torchbox[:city_id].

  Koz, your take ?

• You flagged this item as spam.
  

  mattwestcott May 20th, 2008 @ 02:57 PM

  My motivation for this was being able to assign city_id in a mass assignment from a form post, and therefore avoid having to explicitly handle City objects in the controller. (This worked fine until I needed to add access control based on city, which required looking up the city before and after assignment...) Perhaps what I'm doing there (i.e. including foreign keys in mass-assignments) is frowned upon, and this lack-of-expected-functionality serves as a sort of syntactic vinegar - in which case, fair enough (but I'm not too sure that producing occasional subtle counter-intuitive behavour is a great way to signal "you're doing it wrong"...)

  Will investigate to see if there's a good clean way of handling []= as well (although arguably the cases where you're likely to use []= are the ones where you're seeking to avoid any 'magic' association behaviour).

• You flagged this item as spam.
  

  mattwestcott May 24th, 2008 @ 02:44 PM

  Following up on the possibility of overriding []= / write_attribute too - it looks like this isn't feasible. Various activerecord internals (such as the belongs_to_before_save_for_#{reflection.name} callback) assign the foreign key via []= in the process of manipulating the association, which makes it clear that there shouldn't be any side effects at that level; this persuades me that anything calling []= should be expected to do its own book-keeping.

  So yes, as it stands this patch will make torchbox.city_id=2 behave differently from torchbox[:city_id] = 2. I would have thought that the pattern of overriding setters is common enough that this wouldn't cause anyone any surprises, but others may disagree.

• 

  Pratik July 9th, 2008 @ 01:07 PM

  • State changed from “new” to “wontfix”
  • Tag set to “activerecord, association, belongs_to, foreign-key, patch”

  I'm going to close this as 'wontfix' for now. But I think you should try to spark the discussion in core mailing list, to see if there's any interest.

  Thanks.

• 

  Jon Leighton September 6th, 2008 @ 05:57 PM

  Just to chime in...

  It seems Matt found the response to this ticket quite exasperating, and so didn't want the additional effort of having to try to drum up support on the core list. I'm not surprised.

  In all due respect Pratik, I don't see any real reason why you have rejected it. There is a very clear use for this: being able to assign the city from an incoming form submission, where it is impossible to assign directly to city=. I don't understand why this would be controversial, as city_id is really just an internal representation of the underlying city - I cannot for the life of me think of an example of a time where a user would consider it the expected or most useful behaviour to have a city_id which is different from the id of the object returned by city.

  Furthermore, once the record has been saved, if I were to reload the record, or find it from the database afresh, the city will have changed (obviously, because the city_id has changed in the db). How does that fit with "the principle of least surprise"?

  I'm not trying to spark an argument here, but I can see it must be very frustrating for Matt to have what seems to be a pretty obvious fix rejected by ultimately one person in a community of thousands.

• 

  Michael Koziarski September 6th, 2008 @ 08:25 PM

  I'm sorry he found the wontfix disapponting, the patch itself could be improved but the bug is genunely frustrating. Instead of turning this into a question of Pratik's opinion why not email the list yourself? Or figure out what problems were identified and fix those.

  We're all people here, let's focus on improvng things rather than making and taking things personally .

• 

  Jon Leighton September 6th, 2008 @ 08:33 PM

  Firstly, just to clarify, I am not taking this personally, but I am agreeing with Matt that "the process" hasn't worked well here.

  I'm slightly confused with your comment, because on the one hand you say "the bug is genuinely frustrating", but on the other hand you invite me to email the core list myself. What would such an email say? If it's genuinely frustrating surely the bug could be re-opened and any issues with the actual patch could be identified and resolved?

  I'm not trying to be difficult, and would genuinely like to do whatever is needed to find a more amicable resolution to this ticket :)

• 

  Michael Koziarski September 6th, 2008 @ 08:50 PM

  • State changed from “wontfix” to “stale”

  The patch doesn't apply cleanly, and isn't in the right format (see the advice).

  Couldn't it use BelongsToAssociation#reset instead of setting to nil?

  Couldn't it use write_attribute instead of []=?

  It's things like this that need to be addressed more than debating the merits of the bug itself.

  Finally, mailing the core list isn't about trying to drum up a posse to come post comments on this ticket, but rather about exploring potential solutions with other contributors.

• 

  Jon Leighton September 6th, 2008 @ 08:54 PM

  Thanks for your comments. I will implement those suggestions and upload a new patch, and then solicit discussion of the concept on the core list.

• 

  Jon Leighton September 7th, 2008 @ 02:48 PM

  Attached is an improved patch which implements your suggestions. In order for BelongsToAssociation#reset to be used, it was necessary to check whether association.loaded? in the association accessor method; I've added a test for that additional behaviour.

  I will send a message to the Rails core list, although it might be a little while because I am a new member to that group and so need to have my first post moderated.

  Cheers

  • belongs_to_update_association_from_foreign_key.diff 3.7 KB
• 

  Pratik September 7th, 2008 @ 08:17 PM

  FWIW, I should have used 'hold'/'stale' status instead of 'invalid'. I was just doing LH gardening ( Notice 45 days of inactivity and me saying 'for now' ). Please don't take it personally. LH ticket statuses don't mean a whole lot.

• 

  Repository September 13th, 2008 @ 10:09 AM

  • State changed from “stale” to “committed”

  (from [fcf31cb7521ba7de0aae972ac2ddfc80e3e7dfce]) Support for updating a belongs to association from the foreign key (without saving and reloading the record)

  Signed-off-by: Michael Koziarski michael@koziarski.com [#142 state:committed] http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  John Hume December 11th, 2008 @ 06:35 PM

  I see that commit http://github.com/rails/rails/co... was rolled back in commit http://github.com/rails/rails/co... with a comment about "situations it doesn't cater for" and not holding up 2.2.
  

  Was there any discussion on the mailing list of what those situations are? We're suffering from Rails validating with stale objects and are planning to drop in this patch.

• 

  Michael Koziarski December 12th, 2008 @ 03:00 PM

  • State changed from “committed” to “stale”

  I can't find the mailing list thread, perhaps it was on IRC.

  Off hand the main one I can remember was multiple associations with the same foreign key, the previous patch didn't reset all the different values.

• 

  Jon Leighton December 12th, 2008 @ 03:09 PM

  Is there still interest in this patch of that issue is resolved?

• 

  Michael Koziarski December 12th, 2008 @ 03:11 PM

  Yep, if that issue is resolved and the patch is still not too 'complex' I'd be happy to apply it again and try to dig up any of the other issues that were raised.

• 

  Jon Leighton December 20th, 2008 @ 11:00 PM

  Updated patch to resolve the above issue.

  • belongs_to_update_association_from_foreign_key.diff 6.8 KB
• 

  Frederick Cheung December 20th, 2008 @ 11:02 PM

  • State changed from “stale” to “open”

  Did you mean to have two places where you defined a #{reflection.primary_key_name}=" method ?

• 

  Jon Leighton December 21st, 2008 @ 12:38 AM

  Sort of... there are two commit (the one I did before and a new one on top of that) but they should be separate files, which I will attach now. Thanks.

  • 0001-Support-for-updating-a-belongs-to-association-from-t.patch 3.7 KB
  • 0002-Assigning-to-the-foreign-key-of-a-belongs-to-associa.patch 3.1 KB
• 

  Frederick Cheung December 21st, 2008 @ 01:45 AM

  D'oh me being thick. One random thought that might be too much hassle to worry about: What about has_many/has_one through a belongs_to ?

• 

  Jon Leighton December 21st, 2008 @ 10:41 AM

  I had a think and a tinker about that, but as far as I can tell, there is no way for a belongs_to association to ask "what associations go through me?". We could look through all associated models and query their associations to see if they go through the one in question, but that feels like a step to far really.

  I guess the "proper" way to do it would be to set up some sort of Observer implementation, where associations sign up to observe the key(s) they depend on, and take action when they receive a message that the key(s) have changed. But again, that seems like overkill.

• You flagged this item as spam.
  

  John Hume January 5th, 2009 @ 06:12 PM

  Has there been any discussion of fixing this issue with a stale-check when reading the association rather than trying to clear cached objects when the foreign key is set?

  I'm thinking of doing a check for BelongsToAssociation and an inconsistent state where we currently have if association.nil? || force_reload

  Obviously there's potential performance impact, but it's pretty tiny, and that would be much simpler than the foo_id= implementation in the latest patch.

• 

  Frederick Cheung January 5th, 2009 @ 08:12 PM

  could we not achieve reseting of has_one throughs etc. by first accumulating those belongs_to that need to be reset (which the patch is basically already doing) and then something like

  
   reflections.select {|r| changing_belongs_tos.include?(r.options[:through])}
  

• 

  Pratik March 7th, 2009 @ 12:28 PM

  • State changed from “open” to “incomplete”

  Please put in "open" once we have a patch ready to go.

  Thanks!

• You flagged this item as spam.
  

  Nolan Eakins March 23rd, 2009 @ 08:40 PM

  So what's the verdict on this? Three months not long enough to apply the diffs linked from some of the comments? Looks like there are test cases and all. What exactly is the hold up?

• You flagged this item as spam.
  

  phs March 26th, 2009 @ 06:19 PM

  I've just been bitten by my model not catching an invalid "validates_presence_of :belongs_to_association" after setting the association id from a form.

  While my resources are generally structured so that associations are built up through the associated_collection.build methods, there are a few cases (like this one) I can't get away from.

  For others like me who just want to keep their foreign key integrity in the meantime, I worked around it by dropping this in the model:

  before_validation :clear_association_cache

• You flagged this item as spam.
  

  phs March 26th, 2009 @ 06:29 PM

  I spoke too soon. If you try "before_validation :clear_association_cache", beware strange side effects in your before_save hooks. Say, for example, if you try to save the associated model.

• You flagged this item as spam.
  

  Sean Kirby April 30th, 2009 @ 06:45 PM

  It would be nice if this could be fixed for belongs_to associations and break off handling other types of associations in another ticket. No reason to delay rolling out the patch imho.

• You flagged this item as spam.
  

  Steven Hartland July 24th, 2009 @ 07:19 PM

  There is actually a more serious side effect to this issue, consider the following
  

  created_state_id = 1

  published_state_id = 2

  post = Post.new( "my post", "stuff..." )
  post.state = created
  post.save
  logger.info "Post saved: #{post.state_id}"
  .... logger.info "Before update: #{post.state_id}"
  post.state_id = published_state_id
  logger.info "After update: #{post.state_id}"
  post.save
  logger.info "After save: #{post.state_id}"

  You would expect that after the second save post's state_id would be 2 yes? Well its not the output from the above would be:
  

  Post saved: 1 Before update: 1 After update: 2 After save: 1

  The problem is that the assignment to "state" flags the association, in BelongsToAssociation, as updated but its never cleared so after that point its impossible to change the value of state_id via direct assignment.

  Quite a serious issue really as information is lost.

• You flagged this item as spam.
  

  Steven Hartland July 24th, 2009 @ 07:19 PM

  Ok that totally messed up the formatting :(

• You flagged this item as spam.
  

  Steven Hartland July 24th, 2009 @ 07:23 PM

  Lets try this for formatting...
  There is actually a more serious side effect to this issue, consider the following
  @@ // created_state_id = 1 // published_state_id = 2 post = Post.new( "my post", "stuff..." )
  post.state = created
  post.save
  logger.info "Post saved: #{post.state_id}"
  .... logger.info "Before update: #{post.state_id}"
  post.state_id = published_state_id
  logger.info "After update: #{post.state_id}"
  post.save
  logger.info "After save: #{post.state_id}"
  

  
  You would expect that after the second save post's state_id would be 2 yes? Well its not the output from the above would be:
  

  Post saved: 1
  Before update: 1
  After update: 2
  After save: 1
  

  
  The problem is that the assignment to "state" flags the association, in BelongsToAssociation, as updated but its never cleared, even after save, so after that point its impossible to change the value of state_id via direct assignment.
  
  
  
  
  
  
  
  Quite a serious issue really as information is lost.
  

• You flagged this item as spam.
  

  Steven Hartland July 24th, 2009 @ 07:27 PM

  Grrrr, where's the option to edit / delete your own posts :(

  Last try, sorry about the mess above.

  There is actually a more serious side effect to this issue, consider the following
  

  # created_state_id = 1
  # published_state_id = 2
  post = Post.new( "my post", "stuff..." )
  post.state = created
  post.save
  logger.info "Post saved: #{post.state_id}"
  ....
  logger.info "Before update: #{post.state_id}"
  post.state_id = published_state_id
  logger.info "After update: #{post.state_id}"
  post.save
  logger.info "After save: #{post.state_id}"
  

  You would expect that after the second save post's state_id would be 2 yes? Well its not the output from the above would be:
  

  > Post saved: 1
  > Before update: 1
  > After update: 2
  > After save: 1
  

  The problem is that the assignment to "state" flags the association, in BelongsToAssociation, as updated but its never cleared so after that point its impossible to change the value of state_id via direct assignment.

  Quite a serious issue really as information is lost.

• You flagged this item as spam.
  

  Martin September 9th, 2010 @ 10:34 AM

  Now I spent 2 days (and some hours of my colleagues) with this issue.
  If anyone else finds this:

  Our solution in the current project is a pragmatic one:
  Everytime, you use an object setter (object.association = other_obj), you should call object.reload after save.
  And we are thinking about a generic validation "If associated_id exists in changes, than associated.updated? must return false"

  Both things seem to be workarounds...

• You flagged this item as spam.
  

  cschiefelbein October 10th, 2010 @ 03:32 AM

  I've been bitten by this too, and tried Jon Leighton's patches which worked for me until I tried it with a BelongsToPloymorphicAssociation. The fix for that is quite simple. I'd be happy to submit a patch if there is still interest in fixing this ticket.

  Martin, Steven Hartland: it's not clear to me whether your comments are regarding the original bug or the patched version. Should a new patch incorporate any additional tests to satisfy the issues you raise?

• You flagged this item as spam.
  

  Trung LE October 31st, 2010 @ 05:42 AM

  someone please ban wangxindan and fstory, i hate spammer!!

• 

  Jeff Kreeftmeijer November 8th, 2010 @ 08:23 AM

  • Tag cleared.
  • Importance changed from “” to “Low”

  Automatic cleanup of spam.

• 

  Jon Leighton December 23rd, 2010 @ 01:45 PM

  • State changed from “incomplete” to “duplicate”

  If anyone is interested, this has been re-reported at #2989 and is being taken forward there.