This project is archived and is in readonly mode.

#5442 ✓resolved
oleg dashevskii

Model.has_many_through_association.find(id) returns a read-only record

Reported by oleg dashevskii | August 24th, 2010 @ 02:49 PM | in 3.1

I have the following models:

class Provider < AR::Base
  has_many :dish_types
  has_many :dishes, :through => :dish_types
end

class DishType < AR::Base
  has_many :dishes
  belongs_to :provider
end

class Dish < AR::Base
  belongs_to :dish_type
end

Now load dish with #find:

ree-1.8.7-2010.02 > Provider.first.dishes.find(1).readonly?
 => true 
ree-1.8.7-2010.02 > Provider.first.dish_types.find(1).readonly?
 => false 
ree-1.8.7-2010.02 > DishType.first.dishes.find(555).readonly?
 => false

Simple has_many associations work as they should, but HMT association returns a read-only record.

Comments and changes to this ticket

  • Paul Rosania

    Paul Rosania August 24th, 2010 @ 08:05 PM

    Confirmed under Rails 3 RC2.

  • oleg dashevskii
  • Neeraj Singh

    Neeraj Singh August 25th, 2010 @ 04:17 PM

    • Importance changed from “” to “Low”

    This is not a bug. It is an undocumented feature. :-)

    Look at following test cases which is already there in ActiveRecord.

      def test_habtm_find_readonly
        dev = Developer.find(1)
        assert !dev.projects.empty?
        assert dev.projects.all?(&:readonly?)
        assert dev.projects.find(:all).all?(&:readonly?)
        assert dev.projects.readonly(true).all?(&:readonly?)
      end
    
      def test_has_many_find_readonly
        post = Post.find(1)
        assert !post.comments.empty?
        assert !post.comments.any?(&:readonly?)
        assert !post.comments.find(:all).any?(&:readonly?)
        assert post.comments.readonly(true).all?(&:readonly?)
      end
    

    As you can see by default with habtm all the retrieved records are marked as readonly.

    Although it is not there in the test but you can pass readonly(false) and you will get non readonly records.

    Check this out

    class Post < ActiveRecord::Base
      has_many :taggings
      has_many :tags, :through => :taggings
    
      def self.lab
        tmp = Post.first.tags.readonly(true).find(1).readonly?
        puts tmp #=> true
      end
    end
    
    
    class Post < ActiveRecord::Base
      has_many :taggings
      has_many :tags, :through => :taggings
    
      def self.lab
        tmp = Post.first.tags.readonly(false).find(1).readonly?
        puts tmp #=> false
      end
    end
    
  • Neeraj Singh

    Neeraj Singh August 25th, 2010 @ 04:18 PM

    I should have tested with habtm rather than has_many through. But the ActiveRecord test is with habtm.

  • Paul Rosania

    Paul Rosania August 25th, 2010 @ 04:42 PM

    Are there safety consequences to using #readonly(false) during retrieval?

    Why is it defaulted to readonly in this case? Results retrieved through HMT have been readonly? #=> false for a long time (since first beta?). (I believe this has been the case since HMT was introduced.) Is readonly(true) necessary for some reason now?

  • Neeraj Singh

    Neeraj Singh August 25th, 2010 @ 04:45 PM

    Good questions. And I do not have answer.

    With final release of rails 3 so close I believe we should not change API much. I would try to dig deeper into this issue later.

  • Paul Rosania

    Paul Rosania August 25th, 2010 @ 05:02 PM

    Thanks for the quick reply! I took one more peek at the Rails source, and I actually think that the current behavior (readonly? #=> true) contradicts this test, introduced in Rails 1.15:

    activerecord/test/cases/readonly_test.rb:69

    def test_has_many_with_through_is_not_implicitly_marked_readonly
      assert people = Post.find(1).people
      assert !people.any?(&:readonly?)
    end
    

    See commit http://github.com/rails/rails/commit/3f049b0b6b5a338786c3dfafb31edf...

    I spent some time tracing execution yesterday, but I haven't been able to identify the change between RC and RC2 that causes the regression.

  • oleg dashevskii

    oleg dashevskii August 25th, 2010 @ 05:21 PM

    Paul, this test is noop, since :people fixture isn't loaded and empty array passes the test. I've changed it (see the patch) and it still passes! So the problem is only with find(id) call, which returns a readonly record for some reason.

    Returning back to my IRB session:

    ree-1.8.7-2010.02 > Provider.first.dishes.find(1).readonly?
     => true 
    ree-1.8.7-2010.02 > Provider.first.dishes.map(&:readonly?).uniq
     => [false]
    

    This is clearly a bug.

  • Paul Rosania

    Paul Rosania August 25th, 2010 @ 05:28 PM

    I just noticed the same thing. To clarify, readonly_test.rb currently specifies the following:

    • Model.habtm.find(1).readonly? #=> true
    • Model.has_many.find(1).readonly? #=> false

    There is no test of has_many :through behavior, which has changed from readonly #=> false to readonly #=> true between RC and RC2.

    This change is likely an unintended side-effect of changes to the Relation class since RC, but I have not been able to isolate it.

  • oleg dashevskii

    oleg dashevskii August 26th, 2010 @ 05:30 AM

    • Title changed from “Model.has_many_through_association.find(id) returns a read-only record” to “[PATCH] Model.has_many_through_association.find(id) returns a read-only record”

    This is a rather obscure bug. I'm glad to say I finally traced it down. ActiveRecord::QueryMethods#build_select wasn't resetting @implicit_readonly since it was passed empty selects argument. The RC version was using selects.present? check which returned true even for empty array. And the selects were empty because of the finder options slicing.

    The fix is attached.

  • oleg dashevskii

    oleg dashevskii August 26th, 2010 @ 05:31 AM

    • Tag changed from activerecord rails3, find_one, has_many_through_association, readonly to activerecord rails3, find_one, has_many_through_association, patch, readonly
  • Justin Smestad

    Justin Smestad August 28th, 2010 @ 08:31 AM

    Running into this bug as well. Quite annoying. +1 for this being fixed in 3.0.0 final.

  • Santiago Pastorino

    Santiago Pastorino August 28th, 2010 @ 05:31 PM

    • Milestone cleared.
    • State changed from “new” to “open”
  • Jeremy Kemper
  • Ryan Bigg

    Ryan Bigg October 11th, 2010 @ 12:12 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer October 12th, 2010 @ 06:57 AM

    • Title changed from “[PATCH] Model.has_many_through_association.find(id) returns a read-only record” to “Model.has_many_through_association.find(id) returns a read-only record”
    • Tag set to has_many_through, patch

    Using the "patch" tag instead of prefixing the ticket title with "[PATCH]" to make sure patched tickets end up in the open patches bin. :)

  • Espen Antonsen

    Espen Antonsen October 15th, 2010 @ 01:36 PM

    I assume this is not fixed in 3.0.1? Can reproduce on 3.0.1 in my project.

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:02 PM

    • Milestone set to 3.0.2
  • Santiago Pastorino
  • Jason King
  • Jason King

    Jason King November 17th, 2010 @ 06:34 PM

    You can work around it declaratively too:

    has_many :foos, :through => :bars, :readonly => false
    
  • Jon Leighton

    Jon Leighton December 21st, 2010 @ 12:48 PM

    • Assigned user set to “Aaron Patterson”

    Hi,

    This patch didn't apply cleanly for me initially. I merged in the tests and ran them, and they passed on master and on 3-0-stable. I've attached an updated patch containing these tests which I think should be applied to guard against regressions.

    I'm not sure whether the change that was proposed in AssociationCollection#find should still be applied, but if so presumably that's a different issue as these tests not pass. I've left it out for now.

    Thanks

  • Jon Leighton

    Jon Leighton December 23rd, 2010 @ 04:39 PM

    • Milestone set to 3.1

    Contrary to my earlier assertion, this is not fixed in 3-0-stable. It is, however, fixed in master.

    I've looked into fixing it in 3-0-stable but my personal opinion is that it's too complicated and therefore not worth it. The root of the problem is that 3-0-stable generates strings for the INNER JOINs it performs, which then causes the @implicit_readonly flag to get set when building the Arel query. In master, the joins are built as Arel objects instead, so this flag is not set.

    I've also attached a new patch against master which cleans up the tests a little, and also fixes the third one (which was not actually testing what it said it was testing).

  • Jon Leighton

    Jon Leighton December 23rd, 2010 @ 11:55 PM

    • State changed from “open” to “resolved”
  • Geoffrey Hichborn

    Geoffrey Hichborn April 12th, 2011 @ 10:50 PM

    Can you confirm that the suggested patch for 3.0-stable doesn't fix the issue/produces unexpected behavior?

  • Justin Smestad

    Justin Smestad April 20th, 2011 @ 08:36 PM

    This is marked as resolved however there is no commit referenced and the last comment from Jon said this could not be fixed till 3.1. Is this fixed or not?

  • Jason King

    Jason King April 20th, 2011 @ 09:20 PM

    Actually, the last comment from Jon says that (a) it's already fixed in master (ie. 3.1), and (b) it's too complicated to fix it in 3.0

  • klkk

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>

Referenced by

Pages