enhance: serialize user update methods - #1042
eternal-flame-AD wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1042 +/- ##
==========================================
+ Coverage 75.77% 75.79% +0.02%
==========================================
Files 66 66
Lines 3620 3644 +24
==========================================
+ Hits 2743 2762 +19
- Misses 666 669 +3
- Partials 211 213 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ddfa0cc to
08f571c
Compare
c031b29 to
e787139
Compare
d486c1b to
6e4f37c
Compare
| ctx.AbortWithError(400, errCannotDeleteLastAdmin) | ||
| return errCannotDeleteLastAdmin | ||
| } | ||
| return a.UserChangeNotifier.fireUserDeleted(id) |
There was a problem hiding this comment.
This should probably be moved outside of the transaction and be only invoked when the deleted succeeded. Otherwise the transaction could fail, and we've already deleted all plugin related resources.
There was a problem hiding this comment.
Hmmm, this looks about right to me? If we failed to clean up all resources the transaction should be rolled back so they can try again.
There is the possibility of a partial cleanup I guess but I think it's more intuitive to keep the user record itself intact instead of completely deleting the user record despite leaving residual dependencies not deleted.
There was a problem hiding this comment.
Sorry missed the notification; I mean it more the other way around, the deletion from the plugin manager succeeds, but the committing of the transaction fails.
Then the users isn't deleted, but the plugin state is already cleaned up.
There was a problem hiding this comment.
I understand this leakage as I described above. I think this is an acceptable tradeoff for now compared to revamping the entire user notify callback design. We can go back to this if this becomes a real issue, I think the easiiest solution to this issue is simply add an soft deletion/account lockout feature rather than trying to delete everything in one atomic action (which is practically impossible when we also have application images, etc).
From my point of view, a user clicking "delete user" and "confirm" will understand the practicality of such actions (plugin state will be deleted). If the user cannot be deleted for whatever reason after all they can just try again.
There was a problem hiding this comment.
Okay, that it can stay this way.
I just tried to delete this on a running instance with a enabled plugin. It seems like the request is just timing out. It could be a connection pool lock. THe transactions owns the connection but inside fireUserDeleted/plugin.Manager#RemoveUser another database call is done, which doesn't use the txdb.
Afterwards the server doesn't accept requests anymore.
6e4f37c to
3421a40
Compare
3421a40 to
6445b84
Compare
2 participants
Serializes user update actions to prevent race conditions leading to unexpected results.
I removed the 'Test_UpdateUserByID_EmptyPassword_Expect400' test as it seemed to be a mistake - it should return 200, it returned 400 in the test because there wasn't a second admin.