fix moving folders out of a cache jail #5424

icewind1991 · 2017-06-15T12:01:41Z

@plrunner @mrieser @bat553 please verify if this fixes the problem for you

Signed-off-by: Robin Appelman <robin@icewind.nl>

mention-bot · 2017-06-15T12:01:43Z

@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @d--j and @rullzer to be potential reviewers.

warnerbryce · 2017-06-15T12:42:08Z

Hello @icewind1991

How can i test this fix i don't have this path in my cloud : tests/lib/Files/Cache/Wrapper/CacheJailTest.php

I have the CacheJail.php in lib/private/Files/Cache/Wrapper/

Does this CacheJailTest.php is necessary ?

codecov · 2017-06-15T12:43:03Z

Codecov Report

Merging #5424 into master will decrease coverage by 22.9%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #5424       +/-   ##
=============================================
- Coverage     54.15%   31.24%   -22.91%     
- Complexity    22349    22362       +13     
=============================================
  Files          1379     1380        +1     
  Lines         85557    85020      -537     
  Branches       1329     1329               
=============================================
- Hits          46331    26564    -19767     
- Misses        39226    58456    +19230

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Cache/Wrapper/CacheJail.php	`0% <0%> (-87.81%)`	`38 <1> (+1)`
version.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Repair/NC13/RepairInvalidPaths.php	`0% <0%> (ø)`	`13 <13> (?)`
lib/private/Repair.php	`27.84% <0%> (-0.36%)`	`19 <0> (ø)`
lib/private/Files/Mount/CacheMountProvider.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
apps/user_ldap/lib/Mapping/UserMapping.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
apps/files_sharing/lib/External/MountProvider.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
lib/private/Files/Mount/LocalHomeMountProvider.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
lib/private/Accounts/Hooks.php	`0% <0%> (-100%)`	`14% <0%> (ø)`
...ivate/Files/Cache/Wrapper/CachePermissionsMask.php	`0% <0%> (-100%)`	`3% <0%> (ø)`
... and 370 more

LukasReschke · 2017-06-15T13:10:20Z

Does this CacheJailTest.php is necessary ?

This is only the unit test, it is not packaged within the final releases. So you don't have to replace that file, it's only for our automated tests relevant :)

LukasReschke · 2017-06-16T09:20:41Z

@icewind1991 Can we have a repair step here? If possible a live repair step to not block the upgrade and in multiple smaller steps.

after further testing seems to not work

LukasReschke

This change works for me locally, but we need to have a way to repair older installations. But there's another desktop client bug … let me try to reproduce this and file to the client repo 🙈

icewind1991 · 2017-06-16T14:59:23Z

note that this issue is completely reproducible without the client by moving files from the web ui

bat553 · 2017-06-20T12:35:26Z

I have not yet been able to apply the fix but I wanted to report that this bug also happened when we create a shared folder.

icewind1991 · 2017-06-20T14:33:28Z

@LukasReschke added repair step

LukasReschke · 2017-06-20T16:09:01Z

@nickvergessen Can you review the repair step? THX!

nickvergessen · 2017-06-20T17:19:00Z

@icewind1991 please write a unit test for the repair step.
Should be fairly easy?!

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 · 2017-06-21T13:52:54Z

@nickvergessen done

MorrisJobke · 2017-06-21T21:51:36Z

I updated this here to do:

move the repair step into a NC13 folder
add a version number check to only run the repair step once

MorrisJobke

Tested and works 👍 (also the repair step properly cleans it up)

LukasReschke · 2017-06-22T06:45:55Z

lib/private/Repair/NC13/RepairInvalidPaths.php

+
+	public function run(IOutput $output) {
+		$versionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0');
+		if (version_compare($versionFromBeforeUpdate, '13.0.0.1', '<')) {


Can we adjust this to either:

Use an appconfig value

Also check for 12.0.1 as we want to backport this one :)

Use an appconfig value

This needs to be cleaned up later.

Also check for 12.0.1 as we want to backport this one :)

Okay - if this is 12.0.1 then I will add it as well.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Fixed

LukasReschke · 2017-06-24T09:03:58Z

tests/lib/Repair/RepairInvalidPathsTest.php

+
+use OC\Files\Cache\Cache;
+use OC\Files\Storage\Temporary;
+use OC\Repair\RepairInvalidPaths;


Test not passing, you moved that file 😉

JiriMON · 2017-06-29T08:40:11Z

Seems to work ok for moving folders/files out of a cache jail, but still seeing some occurrences of duplicate files as #3502. Tested on 12.0.0.29 - manually backported from changed files.

Signed-off-by: Robin Appelman <robin@icewind.nl>

jospoortvliet · 2017-07-01T09:50:30Z

Note that it results in lost data 😢 😱
I updated a file on my laptop but it doesn't sync to my desktop, even though both clients claim to be up to date.

icewind1991 · 2017-07-03T12:25:27Z

@LukasReschke please re-review

ArtificialOwl · 2017-07-06T15:59:36Z

Tried to reproduce #3502 with linux client (2.3.2beta, build 13) and this branch. No can do. Works fine for me.

MorrisJobke · 2017-07-06T16:31:13Z

Tried to reproduce #3502 with linux client (2.3.2beta, build 13) and this branch. No can do. Works fine for me.

Seeing as 👍 -> merging

MorrisJobke · 2017-07-06T16:31:30Z

@icewind1991 please create the backport PR ;)

fix moving folders out of a cache jail

fa81759

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 added the 3. to review Waiting for reviews label Jun 15, 2017

icewind1991 added this to the Nextcloud 13 milestone Jun 15, 2017

icewind1991 requested review from nickvergessen and MorrisJobke June 15, 2017 12:01

icewind1991 mentioned this pull request Jun 15, 2017

Duplicated files and folders when moving from one shared folder to another (can reproduce it) #3502

Closed

nickvergessen previously approved these changes Jun 16, 2017

View reviewed changes

LukasReschke previously requested changes Jun 16, 2017

View reviewed changes

icewind1991 force-pushed the moveFromCache-from-shared branch from 0b64df0 to 8a77b8d Compare June 20, 2017 14:33

MorrisJobke added the backport-request label Jun 20, 2017

LukasReschke requested a review from nickvergessen June 20, 2017 16:09

Add repair step for invalid paths

d3c20ee

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 force-pushed the moveFromCache-from-shared branch from 8a77b8d to d3c20ee Compare June 21, 2017 13:52

MorrisJobke approved these changes Jun 21, 2017

View reviewed changes

LukasReschke reviewed Jun 22, 2017

View reviewed changes

Run repair step only once

846e62c

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke force-pushed the moveFromCache-from-shared branch from c0af593 to 846e62c Compare June 22, 2017 20:44

LukasReschke requested changes Jun 24, 2017

View reviewed changes

adjust to moved repair step

601362e

Signed-off-by: Robin Appelman <robin@icewind.nl>

MorrisJobke mentioned this pull request Jul 4, 2017

File move fails silently (but makes copy of files) #5022

Closed

MorrisJobke requested review from rullzer and schiessle July 6, 2017 12:21

MorrisJobke merged commit b4a221f into master Jul 6, 2017

MorrisJobke deleted the moveFromCache-from-shared branch July 6, 2017 16:31

icewind1991 mentioned this pull request Jul 8, 2017

[12] fix moving folders out of a cache jail #5655

Merged

MorrisJobke removed the backport-request label Jul 10, 2017

MorrisJobke mentioned this pull request Jul 13, 2017

Fixed repair step #5715

Merged

MorrisJobke mentioned this pull request Sep 15, 2017

Desktop Sync Client - Moving files deletes and creates them #4188

Closed

icewind1991 mentioned this pull request Sep 20, 2017

[11] fix moving folders out of a cache jail #6588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix moving folders out of a cache jail #5424

fix moving folders out of a cache jail #5424

icewind1991 commented Jun 15, 2017

mention-bot commented Jun 15, 2017

warnerbryce commented Jun 15, 2017

codecov bot commented Jun 15, 2017 •

edited

Loading

LukasReschke commented Jun 15, 2017

LukasReschke commented Jun 16, 2017

LukasReschke left a comment •

edited

Loading

icewind1991 commented Jun 16, 2017

bat553 commented Jun 20, 2017

icewind1991 commented Jun 20, 2017

LukasReschke commented Jun 20, 2017

nickvergessen commented Jun 20, 2017

icewind1991 commented Jun 21, 2017

MorrisJobke commented Jun 21, 2017

MorrisJobke left a comment

LukasReschke Jun 22, 2017

MorrisJobke Jun 22, 2017

LukasReschke Jun 24, 2017

JiriMON commented Jun 29, 2017

jospoortvliet commented Jul 1, 2017

icewind1991 commented Jul 3, 2017

ArtificialOwl commented Jul 6, 2017

MorrisJobke commented Jul 6, 2017

MorrisJobke commented Jul 6, 2017

fix moving folders out of a cache jail #5424

fix moving folders out of a cache jail #5424

Conversation

icewind1991 commented Jun 15, 2017

mention-bot commented Jun 15, 2017

warnerbryce commented Jun 15, 2017

codecov bot commented Jun 15, 2017 • edited Loading

Codecov Report

LukasReschke commented Jun 15, 2017

LukasReschke commented Jun 16, 2017

LukasReschke left a comment • edited Loading

Choose a reason for hiding this comment

icewind1991 commented Jun 16, 2017

bat553 commented Jun 20, 2017

icewind1991 commented Jun 20, 2017

LukasReschke commented Jun 20, 2017

nickvergessen commented Jun 20, 2017

icewind1991 commented Jun 21, 2017

MorrisJobke commented Jun 21, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

LukasReschke Jun 22, 2017

Choose a reason for hiding this comment

MorrisJobke Jun 22, 2017

Choose a reason for hiding this comment

LukasReschke Jun 24, 2017

Choose a reason for hiding this comment

JiriMON commented Jun 29, 2017

jospoortvliet commented Jul 1, 2017

icewind1991 commented Jul 3, 2017

ArtificialOwl commented Jul 6, 2017

MorrisJobke commented Jul 6, 2017

MorrisJobke commented Jul 6, 2017

codecov bot commented Jun 15, 2017 •

edited

Loading

LukasReschke left a comment •

edited

Loading