run update_resource inside a transaction to avoid autosaving relationships through assign_attributes when the record is invalid #7437
Conversation
c4bc6c4 to
50a9275
Compare
50a9275 to
ab27f84
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7437 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 141 141
Lines 4064 4076 +12
=======================================
+ Hits 4028 4040 +12
Misses 36 36 ☔ View full report in Codecov by Sentry. |
|
@deivid-rodriguez @javierjulio is this planned to be merged at some point? it solves an important issue |
|
I also just encountered that issue, this fix is important and should be merged. |
|
Is this PR going to be merged, or is there another fix for this issue already in the codebase? |
|
Hello, sorry for the radio silence here. I'll try to find some time to re-evaluate this in the following weeks! |
Are you experiencing this problem? I personally don't know if it has been fixed independently, but if you're experiencing the issue, I think you're very well equipped to verify it! |
@deivid-rodriguez I manage to replicate the original issue in v4 branch adding some models to the template app. This confirms the errors hasn't been fixed. I want to write a feature test about the change. I don't have time now but should have it in the following days. |
c3cbefd to
ca0c7a5
Compare
1f00911 to
a12c203
Compare
27365d4 to
325f093
Compare
…ships through assign_attributes when the record is invalid
|
@deivid-rodriguez I found time to write the spec. I rebased the PR. The first commit shows the failure and the second one, a cherry-pick of the original code, fixes it. About the spec. It involved a new model so there are a lot of new files just for this. It may be the wrong call but I considered it cleaner than trying to find a way to mimic the issue scenario with existing models (for example: using Now we have |
|
@mgrunberg than 10000 k you. Did you consider or try doing a controller or unit test instead for this? Cucumber adds significant overhead that makes it hard to follow. Then again I don't know how easy it is to do in a controller or unit test, it would just be ideal if we could I think. |
@javierjulio I didn't consider a controller test. No strong argument here. I simply went for a feature test. It should be possible to add a spec in Just one question, are you fine with the introduction of the new modals? |
|
@mgrunberg no worries, take your time. Thank you. Yes the models, migrations, etc. are ok I just prefer to avoid Cucumber if we can. Although feel free to leave the feature spec as is since it's already written. I just think a unit or controller test would be easier to understand and review if we can. Thank you again. |
|
@javierjulio I wrote the controller spec. Let me know what you think |
Co-authored-by: Javier Julio <JJfutbol@gmail.com>
A copy of #6202, to not let it be forgotten.
Fixes #5430
When submitting a form with invalid attributes, some associations are autosaved via assign_attributes before calling save_resource. As a consequence, when updating a resource, the user sees a validation error and is not aware that the changes to the associations got persisted.
This PR runs the code inside the update_resource method in an ActiveRecord::Transaction so that everything is rolled back when the model is not valid.