Fixes #31849 - delete thumbnails on write hooks #31853
Conversation
|
@DeepDiver1975 did you find out why it worked in v10.0.8 ? Are we not triggering the same hooks with the new versions code ? |
I completly rewired the hook processing and missed this case. Hail to stupid hooks and the lack of tests ..... |
| return; | ||
| $user = isset($args['user']) ? $args['user'] : \OC_User::getUser(); | ||
| if ($user === false) { | ||
| $user = Filesystem::getOwner($path); |
There was a problem hiding this comment.
what user do we receive here in the following scenario:
- admin sets up system-wide ext storage "/sftp" with sharing enabled
- user1 shares folder "sftp/test" with public link
- log out
- anonymous user opens link and upload+overwrites file
There is a slight risk that you'll get null here as the storage has no owner since it's system-wide. In other code paths we somehow hackily resolve this to the sharer.
There was a problem hiding this comment.
let me return in case $user is null and we will see how far we get
There was a problem hiding this comment.
Let me know when you are finished editing the "real code" and I will quickly re-run the 3 scenarios in the issue report.
After that, you can write unit tests all you like, and it won't make a difference to the run-time behavior.
There was a problem hiding this comment.
There is no need in checking if Filesystem::getOwner is returning a valid user - it will always be a user (maybe not the correct on - but this is a different story).
An exception will be thrown if there is no owner - so we will stop execution of the hook listener.
no further changes from my pov - let's merge this and I take care about the backport
|
I saw that owner code, and it made me try sharing and editing.
so we have 2 different thumbnails stored somewhere. |
|
@phil-davis are these test scenarios also a regression ? |
|
Hmmm - in 10.0.8 the scenario is "reversed". When "user2" edits the file in the received share, the thumbnail for "user2" is updated. But the thumbnail for "user1" (the owner) stays out-of-date. In 10.0.9RC1 nether user sees an updated thumbnail. So you can choose which set of symptoms you "like" - |
|
I guess there is some underlying issue that is allowing the possibility for a file to have a thumbnail generated and stored multiple times, under different users. If that is so, then I guess it is a whole other can-of-worms that has been there "a long time". |
Codecov Report
@@ Coverage Diff @@
## master #31853 +/- ##
============================================
- Coverage 63.27% 63.27% -0.01%
- Complexity 18476 18479 +3
============================================
Files 1161 1161
Lines 69371 69377 +6
Branches 1261 1261
============================================
Hits 43895 43895
- Misses 25106 25112 +6
Partials 370 370
Continue to review full report at Codecov.
|
|
As discussed in chat, there are issues with file sharing and keeping thumbnails in-sync when sharer and sharee(s) edit a file. Multiple thumbnails are getting generated under various conditions. That has been the case "for some time". The particular set of symptoms evident with this PR might be a little different to the symptoms when running 10.0.8, but the underlying general problem is common. I will raise an issue for that, which needs to be addressed separately, not as a "quick fix for an RC". |
|
Backport |
Description
Thumbnails only got deleted on delete - not for write events.
On write thumbnails need to be deleted so that a future GET will recreate them as needed.
Related Issue
Fixes #31849
How Has This Been Tested?
Types of changes
Checklist: