Fixup preview issues with trashbin/versions #34283

VicDeo · 2019-01-28T22:01:20Z

Description

uid is always known - pass it within the version_delete hook info
pass original_path that contains a path to the versioned file
and deleted_revision - versionId for the deleted version
pass an optional versionId to deleteAllPreviews when we need to remove previous for the specific version only
bonus 1: fix preview generation for files in deleted directories
bonus 2: cover some parts of files_versions with tests

Related Issue

Fixes [10.0.9]Trying to get owner from an erased file #32346
https://github.com/owncloud/enterprise/issues/2904

Motivation and Context

Version previews are not deleted properly
Previews don't show for files in deleted directories
Versions and trash expiration is flooding the log file with exceptions related to the previews

How Has This Been Tested?

Case 1

Create a text file "test.txt"
Edit the file three times in the text editor and save each time, will create three versions
Open the versions panel to have it generate previews
Run cron.php and observe the logs

Expected result

version previews for cleaned up versions are deleted
No errors

Actual result

version previews for cleaned up versions are NOT deleted

{"reqId":"viyF6aTvAF41QBIxIu9B","level":3,"time":"2018-10-15T19:37:42+00:00","remoteAddr":"","user":"--","app":"no app in context","method":"--","url":"--","message":"Exception: {\"Exception\":\"OCP\\\\Files\\\\NotFoundException\",\"Message\":\"\\\/test.txt.v1539632201 not found while trying to get owner\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/Filesystem.php(961): OC\\\\Files\\\\View->getOwner('\\\/test.txt.v1539...')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1241): OC\\\\Files\\\\Filesystem::getOwner('\\\/test.txt.v1539...')\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/legacy\\\/hook.php(105): OC\\\\Preview::prepare_delete(Array)\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Storage.php(765): OC_Hook::emit('\\\\\\\\OCP\\\\\\\\Versions', 'preDelete', Array)\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Command\\\/Expire.php(60): OCA\\\\Files_Versions\\\\Storage::expire('\\\/test.txt', 'admin')\\n#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Command\\\/CommandJob.php(34): OCA\\\\Files_Versions\\\\Command\\\\Expire->handle()\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/Job.php(57): OC\\\\Command\\\\CommandJob->run('O:33:\\\"OCA\\\\\\\\Files...')\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/QueuedJob.php(42): OC\\\\BackgroundJob\\\\Job->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#8 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/cron.php(120): OC\\\\BackgroundJob\\\\QueuedJob->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#9 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php\",\"Line\":1717}"}
{"reqId":"viyF6aTvAF41QBIxIu9B","level":3,"time":"2018-10-15T19:37:42+00:00","remoteAddr":"","user":"--","app":"no app in context","method":"--","url":"--","message":"Exception: {\"Exception\":\"OCP\\\\Files\\\\NotFoundException\",\"Message\":\"\\\/test.txt.v1539632201 not found while trying to get owner\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/Filesystem.php(961): OC\\\\Files\\\\View->getOwner('\\\/test.txt.v1539...')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1311): OC\\\\Files\\\\Filesystem::getOwner('\\\/test.txt.v1539...')\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1299): OC\\\\Preview::post_delete(Array, 'files\\\/')\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/legacy\\\/hook.php(105): OC\\\\Preview::post_delete_versions(Array)\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Storage.php(767): OC_Hook::emit('\\\\\\\\OCP\\\\\\\\Versions', 'delete', Array)\\n#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Command\\\/Expire.php(60): OCA\\\\Files_Versions\\\\Storage::expire('\\\/test.txt', 'admin')\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Command\\\/CommandJob.php(34): OCA\\\\Files_Versions\\\\Command\\\\Expire->handle()\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/Job.php(57): OC\\\\Command\\\\CommandJob->run('O:33:\\\"OCA\\\\\\\\Files...')\\n#8 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/QueuedJob.php(42): OC\\\\BackgroundJob\\\\Job->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#9 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/cron.php(120): OC\\\\BackgroundJob\\\\QueuedJob->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#10 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php\",\"Line\":1717}"}

Case 2

create a text files, edit it several times
open versions to create a previews for versions
edit file one more time

Expected result

Only file previews are deleted

Actual result

File previews and all versions previews are deleted

Case 3

delete a directory with a file and browse to the file in the trashbin

Expected result

File in deleted directory has a prievew

Actual result

No file preview

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-01-28T22:39:42Z

Codecov Report

Merging #34283 into master will increase coverage by 0.04%.
The diff coverage is 63.37%.

@@             Coverage Diff              @@
##             master   #34283      +/-   ##
============================================
+ Coverage     65.11%   65.15%   +0.04%     
- Complexity    18338    18352      +14     
============================================
  Files          1199     1200       +1     
  Lines         69634    69698      +64     
  Branches       1283     1283              
============================================
+ Hits          45340    45413      +73     
+ Misses        23920    23911       -9     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	`53.1% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.55% <63.37%> (+0.04%)`	`18352 <44> (+14)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_trashbin/ajax/preview.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/files_versions/lib/AppInfo/Application.php	`100% <100%> (ø)`	`3 <1> (+1)`	⬆️
apps/files_versions/lib/Storage.php	`72.67% <24.39%> (+1.28%)`	`95 <10> (-9)`	⬇️
lib/private/Preview.php	`80.12% <58.49%> (+1.38%)`	`178 <17> (+8)`	⬆️
apps/files_trashbin/lib/Trashbin.php	`77.7% <85.71%> (+0.29%)`	`146 <2> (ø)`	⬇️
apps/files_versions/lib/FileHelper.php	`92.72% <92.72%> (ø)`	`14 <14> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b875f06...fca5873. Read the comment docs.

PVince81

See comments.

I wonder if this is enough as there might be other code paths related to trashbin: try having version previews for files that were deleted in trashbin, the trigger trashbin expiration. This will like trigger https://github.com/owncloud/enterprise/issues/3067 and might trigger some other hook methods.

lib/private/Preview.php

PVince81 · 2019-02-01T07:03:25Z

I like how you refactored the static methods to be able to write tests!

PVince81 · 2019-02-01T07:04:18Z

codecov however seems less enthusiastic... and fails to load 😡

VicDeo · 2019-02-01T15:45:29Z

codecov however seems less enthusiastic... and fails to load

@PVince81
it annoys me the most because I need to see what else can be covered there instead of using 🔮 and reading the stars for that purpose

VicDeo · 2019-02-01T16:50:24Z

I wonder if this is enough as there might be other code paths related to trashbin: try having version previews for files that were deleted in trashbin, the trigger trashbin expiration. This will like trigger owncloud/enterprise#3067 and might trigger some other hook methods.

@PVince81
I tried to delete the test file after steps 1-3 from the PR description
Results:

directory data/D/thumbnails/2147496457/ was deleted so no preview of any kind exists anymore.
after browsing to trashbin previews for the file data/D/thumbnails/2147496457/ were recreated
previews for versions were NOT recreated after viewing the file in trashbin.

here is log (nothing fishy)

{"reqId":"952b0382-c5d9-4f17-904e-ebb3647cb83d","level":1,"time":"2019-02-01T16:34:35+00:00","remoteAddr":"127.0.0.1","user":"D","app":"admin_audit","method":"GET","url":"\/~deo\/oc-tmp\/remote.php\/dav\/files\/D\/bebe.txt","message":"Read \"\/D\/files\/bebe.txt\" by user <D>, displayName <Dryndyndyn>, owner: D [CLIENT_IP: 127.0.0.1] [USER_AGENT: Mozilla\/5.0 (Linux) mirall\/2.5.2 (build 268)]"}
{"reqId":"uZHeLxVvFDf2HZX1gS4U","level":0,"time":"2019-02-01T16:35:04+00:00","remoteAddr":"127.0.0.1","user":"D","app":"core","method":"GET","url":"\/~deo\/oc-tmp\/remote.php\/dav\/meta\/2147496457\/v\/1549038093?preview","message":"Generating preview for \"\/meta\/2147496457\/v\/1549038093\" with \"OC\\Preview\\TXT\""}
{"reqId":"S6IUMO4v2hUmT44uHYvy","level":0,"time":"2019-02-01T16:35:04+00:00","remoteAddr":"127.0.0.1","user":"D","app":"core","method":"GET","url":"\/~deo\/oc-tmp\/remote.php\/dav\/meta\/2147496457\/v\/1549038077?preview","message":"Generating preview for \"\/meta\/2147496457\/v\/1549038077\" with \"OC\\Preview\\TXT\""}
{"reqId":"qxEPyFCMFNDb25iyDUsk","level":0,"time":"2019-02-01T16:35:05+00:00","remoteAddr":"127.0.0.1","user":"D","app":"core","method":"GET","url":"\/~deo\/oc-tmp\/remote.php\/dav\/meta\/2147496457\/v\/1549038084?preview","message":"Generating preview for \"\/meta\/2147496457\/v\/1549038084\" with \"OC\\Preview\\TXT\""}
{"reqId":"VxXof4wusgbt9efZIhKO","level":1,"time":"2019-02-01T16:35:18+00:00","remoteAddr":"127.0.0.1","user":"D","app":"admin_audit","method":"DELETE","url":"\/~deo\/oc-tmp\/remote.php\/dav\/files\/D\/bebe.txt","message":"Delete \"\/D\/files\/bebe.txt\" by user <D>, displayName <Dryndyndyn>, owner: D [CLIENT_IP: 127.0.0.1] [USER_AGENT: Mozilla\/5.0 (X11; Linux x86_64) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/72.0.3626.81 Safari\/537.36]"}
{"reqId":"6RtK4Es9C3bSFanQm7sF","level":0,"time":"2019-02-01T16:40:48+00:00","remoteAddr":"127.0.0.1","user":"D","app":"core","method":"GET","url":"\/~deo\/oc-tmp\/index.php\/apps\/files_trashbin\/ajax\/preview.php?file=%2Fbebe.txt.d1549038918&c=1549038918000","message":"Generating preview for \"\/D\/files_trashbin\/files\/bebe.txt.d1549038918\" with \"OC\\Preview\\TXT\""}

So there is nothing to fix for the trashbin IMO unless such agressive deletion of preview is unexpected

VicDeo · 2019-02-01T19:06:56Z

The fix is incomplete, preDelete hook should also have the same changes.
The same applies to trashbin as for trashbin '/files' is added after userid producing weird path like
/D/files/files_trashbin/files/bebe.txt.d1549039401 as a result

PVince81 · 2019-02-04T14:46:33Z

So there is nothing to fix for the trashbin IMO unless such agressive deletion of preview is unexpected

I seem to remember that the code was deleting more than just the expected previews, which is a bit brutal. Can you recheck the exact path that is getting deleted from the code ?

VicDeo · 2019-02-07T11:30:48Z

@PVince81

the code was deleting more than just the expected previews, which is a bit brutal.

it's due \OC\Preview::post_write hands off the job to \OC\Preview::post_delete which removes all previews (including version previews) by default.

…nst non-existing route

VicDeo · 2019-02-15T15:14:34Z

@PVince81 description updated to describe 3 bugs that are already fixed in this PR.
Only trashbin expiration is left to fix.

VicDeo · 2019-02-15T18:28:30Z

I'll list my expectations here. This is how the things work within this PR:

file preview is created when the file is opened in a file list as /user/thumbnails/fileId/x-y.png
version preview is created when the version is listed in the versions tab as /user/thumbnails/fileId/versionId/x-y.png
All file previews are deleted when file is updated: all /user/thumbnails/fileId/*.png are deleted
All version previews are kept when file is updated /user/thumbnails/fileId/versionId/ still exists
File preview is deleted when the file is moved to trashbin: entire /user/thumbnails/fileId/ directory is deleted
Versions previews are deleted when the file is moved to trashbin: entire /user/thumbnails/fileId/ directory is deleted
After browsing to the file in the trashbin preview is recreated in /user/thumbnails/fileId/
After browsing to the file in the version previews are not recreated in /user/thumbnails/fileId/
After a file with a preview is deleted from trashbin - preview is deleted: entire /user/thumbnails/fileId/ directory is deleted
After a file with a preview is restored from trashbin - previews are kept

VicDeo · 2019-02-18T08:45:37Z

@PVince81 please rereview

apps/files_versions/lib/Storage.php

PVince81

some minor questions

apps/files_versions/lib/Storage.php

apps/files_versions/tests/FileHelperTest.php

lib/private/Preview.php

PVince81

👍

PVince81 · 2019-02-18T14:18:13Z

@VicDeo please backport

VicDeo · 2019-02-18T16:08:31Z

stable10: #34533

VicDeo added the 2 - Developing label Jan 28, 2019

VicDeo added this to the development milestone Jan 28, 2019

VicDeo self-assigned this Jan 28, 2019

VicDeo force-pushed the fix-32346 branch from d54a64e to 58a5ba8 Compare January 28, 2019 22:38

VicDeo force-pushed the fix-32346 branch from 8415ea9 to 08fdf75 Compare January 29, 2019 15:33

owncloud deleted a comment from codecov bot Jan 30, 2019

VicDeo force-pushed the fix-32346 branch 5 times, most recently from b7228de to 4cd2eda Compare January 31, 2019 21:49

VicDeo added 3 - To Review and removed 2 - Developing labels Jan 31, 2019

VicDeo force-pushed the fix-32346 branch 3 times, most recently from 1bc87b2 to eb05df4 Compare January 31, 2019 23:05

PVince81 reviewed Feb 1, 2019

View reviewed changes

lib/private/Preview.php Outdated Show resolved Hide resolved

lib/private/Preview.php Outdated Show resolved Hide resolved

VicDeo force-pushed the fix-32346 branch 4 times, most recently from 80a8195 to 806866d Compare February 1, 2019 20:46

PVince81 mentioned this pull request Feb 5, 2019

Attempt to fix version preview deletion #33188

Closed

13 tasks

VicDeo added 3 commits February 15, 2019 15:42

Remove all previews for the version on deletion

496ed1b

Increase coverage for files_versions storage.

bca23cb

Kill unused third parameter that was intended to generate a link agai…

6106d29

…nst non-existing route

VicDeo force-pushed the fix-32346 branch 2 times, most recently from 8749a66 to 47c0735 Compare February 15, 2019 15:05

VicDeo changed the title ~~Remove all previews for the version on deletion~~ Fixup preview issues Feb 15, 2019

VicDeo changed the title ~~Fixup preview issues~~ Fixup preview issues with trashbin/versions Feb 15, 2019

VicDeo force-pushed the fix-32346 branch from 10d09d8 to 7c0ddbb Compare February 15, 2019 18:44

VicDeo added 4 commits February 15, 2019 22:54

Pass a deleted revision and original path as extra hook params

ccfef4d

Keep previews for versions on post_write hook, remove duplicated method

2962aee

Fix trashbin preview generation for files in non-root directory

d1cacb7

Properly resolve all paths starting from /files_trashbin into fileId

c06b2f8

VicDeo force-pushed the fix-32346 branch from 7c0ddbb to c06b2f8 Compare February 15, 2019 19:54

VicDeo added 3 - To Review backport-request and removed 2 - Developing labels Feb 15, 2019

PVince81 reviewed Feb 18, 2019

View reviewed changes

apps/files_versions/lib/Storage.php Outdated Show resolved Hide resolved

PVince81 reviewed Feb 18, 2019

View reviewed changes

apps/files_versions/lib/Storage.php Show resolved Hide resolved

apps/files_versions/tests/FileHelperTest.php Show resolved Hide resolved

lib/private/Preview.php Show resolved Hide resolved

Document test method, fix identation

fca5873

PVince81 approved these changes Feb 18, 2019

View reviewed changes

PVince81 merged commit 2ec2839 into master Feb 18, 2019

delete-merged-branch bot deleted the fix-32346 branch February 18, 2019 14:18

VicDeo mentioned this pull request Feb 18, 2019

[stable10] Fixup preview issues with trashbin/versions #34533

Merged

9 tasks

VicDeo removed the backport-request label Feb 18, 2019

lock bot locked as resolved and limited conversation to collaborators Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup preview issues with trashbin/versions #34283

Fixup preview issues with trashbin/versions #34283

VicDeo commented Jan 28, 2019 •

edited

Loading

codecov bot commented Jan 28, 2019 •

edited

Loading

PVince81 left a comment

PVince81 commented Feb 1, 2019

PVince81 commented Feb 1, 2019

VicDeo commented Feb 1, 2019 •

edited

Loading

VicDeo commented Feb 1, 2019

VicDeo commented Feb 1, 2019 •

edited

Loading

PVince81 commented Feb 4, 2019

VicDeo commented Feb 7, 2019 •

edited

Loading

VicDeo commented Feb 15, 2019

VicDeo commented Feb 15, 2019

VicDeo commented Feb 18, 2019

PVince81 left a comment

PVince81 left a comment

PVince81 commented Feb 18, 2019

VicDeo commented Feb 18, 2019

Fixup preview issues with trashbin/versions #34283

Fixup preview issues with trashbin/versions #34283

Conversation

VicDeo commented Jan 28, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Case 1

Expected result

Actual result

Case 2

Expected result

Actual result

Case 3

Expected result

Actual result

Types of changes

Checklist:

Open tasks:

codecov bot commented Jan 28, 2019 • edited Loading

Codecov Report

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Feb 1, 2019

PVince81 commented Feb 1, 2019

VicDeo commented Feb 1, 2019 • edited Loading

VicDeo commented Feb 1, 2019

VicDeo commented Feb 1, 2019 • edited Loading

PVince81 commented Feb 4, 2019

VicDeo commented Feb 7, 2019 • edited Loading

VicDeo commented Feb 15, 2019

VicDeo commented Feb 15, 2019

VicDeo commented Feb 18, 2019

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Feb 18, 2019

VicDeo commented Feb 18, 2019

VicDeo commented Jan 28, 2019 •

edited

Loading

codecov bot commented Jan 28, 2019 •

edited

Loading

VicDeo commented Feb 1, 2019 •

edited

Loading

VicDeo commented Feb 1, 2019 •

edited

Loading

VicDeo commented Feb 7, 2019 •

edited

Loading