#5442 Model.has_many_through_association.find(id) returns a read-only record - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5442 ✓resolved

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

You flagged this item as spam.
Paul Rosania August 24th, 2010 @ 08:05 PM
Confirmed under Rails 3 RC2.
You flagged this item as spam.
oleg dashevskii August 25th, 2010 @ 10:11 AM
I've added a test proving this bug.
- 0001-test-for-5442.patch 1.4 KB

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 August 25th, 2010 @ 04:18 PM
I should have tested with habtm rather than has_many through. But the ActiveRecord test is with habtm.
You flagged this item as spam.
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 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.
You flagged this item as spam.
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.
You flagged this item as spam.
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.
You flagged this item as spam.
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.
You flagged this item as spam.
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.
- 0001-fix-unwanted-implicit-readonly-in-modelhas_many_thr.patch 2.8 KB
You flagged this item as spam.
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”
You flagged this item as spam.
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 August 28th, 2010 @ 05:31 PM
- Milestone cleared.
- State changed from “new” to “open”
Jeremy Kemper August 30th, 2010 @ 04:10 AM
- Milestone cleared.
[bulk edit]
Ryan Bigg October 11th, 2010 @ 12:12 PM
- Tag cleared.
Automatic cleanup of spam.
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. :)
You flagged this item as spam.
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 October 15th, 2010 @ 11:02 PM
- Milestone set to “3.0.2”
[bulk edit]
Santiago Pastorino November 15th, 2010 @ 09:15 PM
- Milestone cleared.
[bulk edit]
You flagged this item as spam.
Jason King November 17th, 2010 @ 06:31 PM
Still exists in 3.0.3
You flagged this item as spam.
Jason King November 17th, 2010 @ 06:34 PM
You can work around it declaratively too:
```
has_many :foos, :through => :bars, :readonly => false
```
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
- 5442_has_many_through_readonly.diff 1.7 KB
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).
- 5442_has_many_through_readonly.diff 1.6 KB
Jon Leighton December 23rd, 2010 @ 11:55 PM
- State changed from “open” to “resolved”
You flagged this item as spam.
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?
You flagged this item as spam.
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?
You flagged this item as spam.
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
You flagged this item as spam.
klkk May 23rd, 2011 @ 03:14 AM
www.louisvuittonline.com