Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

active_record does not follow active_model specs for #to_key

#5249

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" 

Reported by gucki · July 30th, 2010 @ 10:00 AM

State: resolved
Milestone: 3.0.2
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: Low

Activity

  1. Repository
  2. Repository
  3. Santiago Pastorino
    Santiago Pastorino
    • 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

    August 14th, 2010 @ 02:33 AM

  4. gucki
    gucki

    Hi Santiago, thanks for getting on this. However I still think the patch should be the other way round.

    1. 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.

    2. 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.

    3. 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.

    4. 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).

    August 14th, 2010 @ 08:54 AM

  5. Repository
  6. Santiago Pastorino
    Santiago Pastorino
    • 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.

    August 15th, 2010 @ 03:41 PM

  7. gucki
    gucki

    Santiago, it seems you only changed the doc of #to_key, not the implementation. Or am I missing something? :)

    August 25th, 2010 @ 02:40 PM

  8. Santiago Pastorino
    Santiago Pastorino

    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.

    August 25th, 2010 @ 04:50 PM

  9. Jeremy Kemper