This project is archived and is in readonly mode.
active_record does not follow active_model specs for #to_key
Reported by gucki | July 30th, 2010 @ 10:00 AM | in 3.0.2
I hit this while playing with sequel together rails rails 3.0rc. Please see #to_key here:
http://github.com/rails/rails/blob/master/activemodel/lib/active_mo...
http://github.com/rails/rails/blob/master/activerecord/lib/active_r...
AR is using #new_record? while AM is using #persisted? to check wether to return an id.
Consider the following code:
@news = News.create @news.destroy dom_id(@news)
As sequel conforms to the active_model specs the following
returns "new_news".
ActiveRecord does not follow the AM specs and so returns
"new_4545".
I think the implementation in AM is wrong and AR is correct. Otherwise a lot of old rails apps won't work anymore, because you destroy the @news in den controller and call afterwards in a template something like $('##{dom_id @news}').hide() which won't work if you check for persisted? because persisted? is false (in AR and Sequel) after the object has been destroyed.
Example using AR:
ruby-1.9.2-head > a = News.create => # ruby-1.9.2-head > a.new_record? => false ruby-1.9.2-head > a.persisted? => true ruby-1.9.2-head > a.destroyed? => false ruby-1.9.2-head > ApplicationController.new.dom_id a => "news_2" ruby-1.9.2-head > a.destroy => # ruby-1.9.2-head > a.new_record? => false ruby-1.9.2-head > a.persisted? => false ruby-1.9.2-head > a.destroyed? => true ruby-1.9.2-head > ApplicationController.new.dom_id a => "news_2"
Using Sequel (or any other ORM which conforms to AM):
ruby-1.9.2-head > a = News.create => #87363, :user_id=>nil, :recipient_id=>0, :ref_id=>nil, :ref_type=>"Messagein", :created_at=>nil, :notification=>0}> ruby-1.9.2-head > a.new? => false ruby-1.9.2-head > a.persisted? => true ruby-1.9.2-head > ApplicationController.new.dom_id a => "news_87363" ruby-1.9.2-head > a.destroy => #87363, :user_id=>nil, :recipient_id=>0, :ref_id=>nil, :ref_type=>"Messagein", :created_at=>nil, :notification=>0}> ruby-1.9.2-head > a.new? => false ruby-1.9.2-head > a.persisted? => false ruby-1.9.2-head > ApplicationController.new.dom_id a => "new_news"
Comments and changes to this ticket
-
Repository August 14th, 2010 @ 02:28 AM
(from [1590377886820e00b1a786616518a32f3b61ec0f]) Makes AR use AMo to_key implementation
[#5249] http://github.com/rails/rails/commit/1590377886820e00b1a786616518a3...
-
Repository August 14th, 2010 @ 02:28 AM
(from [ccd4364a13d7a3af9eec7672e08d0682765f5b2f]) Makes AR use AMo to_key implementation
[#5249] http://github.com/rails/rails/commit/ccd4364a13d7a3af9eec7672e08d06...
-
Santiago Pastorino August 14th, 2010 @ 02:33 AM
- Milestone set to 3.x
- State changed from new to invalid
- Assigned user set to Yehuda Katz (wycats)
- Importance changed from to Low
gucki this make me realize that it makes sense if AR uses AMo implementation. So i've pushed that.
Also if you look at the test i've changed it's ok too, a model doesn't have primary_key after being deleted.Regarding to your ticket i think the way you're trying to hide the dom element is not the best ... take a look at this example made by Yehuda ...
$("li.item a").click(function() { var li = $(this).closest("li"); $.ajax({ method: "DELETE", type: "json", url: "/items/" + this.data("id"), success: function(json) { if(json.success) { li.remove(); } } }); });
I'm closing this issue and assigning to Yehuda just in case he want to add something else
-
gucki August 14th, 2010 @ 08:54 AM
Hi Santiago, thanks for getting on this. However I still think the patch should be the other way round.
-
When an object is destroyed only the record in the db is deleted. The object itself has the same data as before, it's just old/ outdated data in some way (ex the fk is outdated) and destroyed? returns true.
-
I don't the see anything wrong with using this old, outdated data. It's really usefully in some cases. I'm not sure if it's a good practice/ idea to replace a simple link_to("delete", @item, :method => :delete) with a rjs containing only $('##dom_id(@item).hide') with such a huge JS block as shown above.
-
It breaks A LOT of exisiting rails applications. I just googled for "rails ajax tutorial detroy" and found lot's of examples using the technique above within seconds. Some of them would still work but only because they hardcode the id like "note_#{@note.id}" and are not using dom_id, but I guess that's not really the point.
-
What are the drawbacks when you would change it the ways I suggested, so current AR behavior into AM?
Btw: I'm not sure why you close the ticket with "invalid". The ticket is a valid convern/ bug report, you even created a patch. Shouldn't it be marked resolved instead (well, in fact open).
-
-
Repository August 14th, 2010 @ 12:23 PM
(from [36a84a4f15f29b41c7cac2f8de410055006a8a8d]) Makes AR use AMo to_key implementation
[#5249] http://github.com/rails/rails/commit/36a84a4f15f29b41c7cac2f8de4100...
-
Santiago Pastorino August 15th, 2010 @ 03:41 PM
- Milestone cleared.
- State changed from invalid to resolved
- Assigned user changed from Yehuda Katz (wycats) to Santiago Pastorino
gucki AMo semantics were changed, ORMs should implement to_key according to this new semantics http://github.com/rails/rails/commit/d0cf212cb5a02db1b3df85e1a337ea...
Also i've reverted the AR change. -
gucki August 25th, 2010 @ 02:40 PM
Santiago, it seems you only changed the doc of #to_key, not the implementation. Or am I missing something? :)
-
Santiago Pastorino August 25th, 2010 @ 04:50 PM
Yes only the docs were changed. ORMs which conforms to AMo should implement to_key following the rules given in the docs.
Also the implementation of to_key as José said in the docs should be ...
respond_to?(:id) && id ? [id] : nil
But Ruby 1.8.7 always respond_to id so nothing better can be done.
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>
People watching this ticket
Referenced by
- 5249 active_record does not follow active_model specs for #to_key [#5249] http://github.com/rails/rails/commit/15903778868...
- 5249 active_record does not follow active_model specs for #to_key [#5249] http://github.com/rails/rails/commit/ccd4364a13d...
- 5249 active_record does not follow active_model specs for #to_key [#5249] http://github.com/rails/rails/commit/36a84a4f15f...