Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eager_load preserves readonly flag for associations #18097

Merged
merged 1 commit into from
Dec 29, 2014
Merged

eager_load preserves readonly flag for associations #18097

merged 1 commit into from
Dec 29, 2014

Conversation

k0kubun
Copy link
Contributor

@k0kubun k0kubun commented Dec 19, 2014

Closes #15853

I changed join_dependency to pass skipped test in 5706d29.

@robin850
Copy link
Member

Hi @k0kubun,

Thanks for your contribution! FWIW, running the provided gist in #15853 fails on 4.1.8 but passes on 4.2.0. As far as I can understand, this is already fixed for has_many/belongs_to associations. Your patch correctly fixes the mentioned test though. :-)

@k0kubun
Copy link
Contributor Author

k0kubun commented Dec 29, 2014

@robin850
Thank you for testing! I forgot to run the provided gist in #15853 because I thought 5706d29 added proper tests for the issue.

@k0kubun
Copy link
Contributor Author

k0kubun commented Dec 29, 2014

Because the provided gist in #15853 is testing belongs_to, I added a test for it.
It fails on master branch and my patch fixes it.

rafaelfranca added a commit that referenced this pull request Dec 29, 2014
@rafaelfranca
Merge pull request #18097 from k0kubun/readonly-eager_load
266ff70 
`eager_load` preserves readonly flag for associations
@rafaelfranca rafaelfranca merged commit 266ff70 into rails:master Dec 29, 2014
@robin850
Copy link
Member

Thank you @k0kubun !

@k0kubun k0kubun deleted the readonly-eager_load branch December 29, 2014 17:30
bquorning
bquorning reviewed Mar 7, 2016
View reviewed changes
activerecord/test/cases/associations/eager_test.rb
@@ -1340,6 +1339,10 @@ def test_deep_including_through_habtm
# has-many :through
david = Author.where(id: "1").eager_load(:readonly_comments).first!
assert david.readonly_comments.first.readonly?

# belongs_to
post = Post.where(id: "1").eager_load(:author).first!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for commenting on an old PR, but I just don’t understand this change. The association is defined on Post as belongs_to :author – no readonly option defined. So why does it need to be readonly after eager loading?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can't remember this problem well now, I think this behavior is not related to readonly option of belongs_to. At least joins was such a behavior and I unified them. I guess it is limited as readonly because the aim of eager loading is not writing but just reading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that joins doesn’t make the association readonly, while eager_load does. I’ve opened a bug #24093.

@bquorning bquorning mentioned this pull request Mar 7, 2016
Closed
@k0kubun k0kubun mentioned this pull request Mar 8, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Eager loaded models not marked as read only
4 participants
@k0kubun @robin850 @bquorning @rafaelfranca