Preserve readonly flag only for readonly association #24099
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -1419,6 +1419,24 @@ def test_eager_load_multiple_associations_with_references | |||
| assert david.readonly_comments.first.readonly? | |||
| end | |||
|
|
|||
| test "eager-loading non-readonly association" do | |||
| # has-one | |||
There was a problem hiding this comment.
Fixed and squashed in k0kubun@7eaa6dd
k0kubun
force-pushed
the
preserve-readonly
branch
2 times, most recently
from
ffd4b4b
to
7eaa6dd
Compare
| join_scope = join_scope.instance_exec(nil, &node.reflection.scope) if node.reflection.scope | ||
| if join_scope.values[:readonly] | ||
| model.readonly! | ||
| end |
There was a problem hiding this comment.
This implementation is reaching into a lot of internals of other objects. Can you refactor this to something a bit more contained?
There was a problem hiding this comment.
How about this? k0kubun@198b285
k0kubun
force-pushed
the
preserve-readonly
branch
2 times, most recently
from
b258136
to
198b285
Compare
|
@sgrif Do you have time for a 2nd review? In my opinon, this (and #24093) should be added to the 5.0.0 milestone. |
|
We are well beyond the feature freeze for the 5.0 milestone. This bug isn't critical enough to mark as a release blocker. It will be resolved in 5.0l.1 |
|
@jeremy Do you want to add this PR to the 5.0.1 milestone, as per Sean’s comment above? |
|
Solves #25835 as well. |
| @@ -272,7 +272,9 @@ def construct(ar_parent, parent, row, rs, seen, model_cache, aliases) | |||
| construct(model, node, row, rs, seen, model_cache, aliases) | |||
| else | |||
| model = construct_model(ar_parent, node, row, model_cache, id, aliases) | |||
| model.readonly! | |||
| if node.reflection.scope_for(node.base_klass).values[:readonly] | |||
There was a problem hiding this comment.
values dup new Hash. It is better to use readonly_value.
https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/relation.rb#L678-L680
https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/relation/query_methods.rb#L73-L75
There was a problem hiding this comment.
Thanks to catch it. Fixed so in 411e459.
k0kubun
force-pushed
the
preserve-readonly
branch
from
198b285
to
411e459
Compare
|
Just FYI, to help someone else running into this in the meantime: our workaround was to |
Preserve readonly flag only for readonly association
Preserve readonly flag only for readonly association
|
Backported in 3cffae5 |
Summary
I fixed a bug in #18097. See #24093 for detail.
In this patch, I made eager-loaded association readonly only when it was specified as readonly.
Fixes #24093