Keep fileid when restoring version, using stream copy #15486

PVince81 · 2015-04-08T16:29:12Z

Always stream copy when restoring version

This is a workaround to make it possible to keep the file id when restoring a file. Please note that a stream copy would happen anyway when restoring cross-storage.

Please review @icewind1991 @SergioBertolinSG @nickvergessen @MorrisJobke @DeepDiver1975

PVince81 · 2015-04-08T16:29:44Z

Note that this is broken on OC 8.0.3 as well, we might want a backport @karlitschek (even if it's for 8.0.4, depending on severity)

PVince81 · 2015-04-08T16:30:01Z

Fixes #15468

karlitschek · 2015-04-08T17:44:38Z

looks good 👍 Backport is fine but only for 8.0.4

SergioBertolinSG · 2015-04-09T07:27:41Z

I've tested #15468 in this branch and it works.
But, if the user2 restores the oldest version, it is restored and a new version (additional) is created.

PVince81 · 2015-04-09T08:18:16Z

Yes, this is expected, in case you want to re-switch back to the version you had before.
Can you compare with OC 7 ?

SergioBertolinSG · 2015-04-09T08:30:02Z

In OC 7 after restoring oldest version there is no creation of a new version.
If you have a file with 3 versions, after restoring the oldest i think you should have still 3 versions. OC 7 behaves this way.
Here after recipient restoring of the oldest you have 4.

PVince81 · 2015-04-09T08:31:25Z

Hmmm strange, thanks for the check.
Maybe it's the stream copy that is firing hooks and automatically creating a new version. Not good.

In OC 7 that would probably happen too if the version is restored to external storage... as a side effect

PVince81 · 2015-04-09T08:32:04Z

As this is not going into 8.0.3 I'll have a look later during the bugfix phase.

PVince81 · 2015-04-09T08:32:31Z

BUG: do not create an additional version when restoring version (possibly due to streamcopy), might need a different approach...

nickvergessen · 2015-04-09T08:35:31Z

What happens if we don't have enough space to duplicate the file?

PVince81 · 2015-04-09T08:38:26Z

The same as on OC 7: stuff will likely break...

Before restoring the version, there is existing code that will backup the current file (as an extra version) through copying, and then proceed to restore the selected version.
This should be addressed (and confirmed) as a separate issue.

PVince81 · 2015-04-16T11:11:59Z

I changed the implementation: now it's doing the rename/copy on storage level to avoid touching the cache, which will also keep the file id.
The unit tests was also extended to test with single storage or cross storage.
I manually tested restoring versions on single storage and on external storage, the fileid / favorite info / share info was kept.

Please review/retest @icewind1991 @SergioBertolinSG @schiesbn @nickvergessen

PVince81 · 2015-04-16T11:14:07Z

apps/files_versions/lib/storage.php

+
+		if ($result !== false) {
+			$storage1->unlink($internalPath1);
+			$storage1->getCache()->remove($internalPath1);


It turns out it also works without this line. I'll remove it once Jenkins is done (to avoid confusing it)
Maybe getVersions() is doing a rescan internally.

PVince81 · 2015-04-24T14:27:54Z

Please review (sorry, forgot to set the label to "to review")

PVince81 · 2015-04-28T13:22:15Z

@SergioBertolinSG can you retest this ?

SergioBertolinSG · 2015-04-29T07:17:30Z

@PVince81 I have tested #15468 in versions-rollbackkeepfileid, here you can restore a shared file to previous version correctly without losing the share.
But there is still the problem about a version of the file deleted when restored the last version, only in not shared files. Do you remember if there is a different issue about this or this was introduced here?.

PVince81 · 2015-04-29T09:42:02Z

@SergioBertolinSG I did fix the problem with the extra version. Did you "git pull" this branch ?
Might be safer to just delete the branch with "git branch -D versions-rollbackkeepfileid", then "git fetch" and then check out the branch again.

SergioBertolinSG · 2015-04-29T09:49:50Z

I tested it in a new install, checking out the branch for first time. I have to say it doesn't happen always, but many times.

PVince81 · 2015-04-29T10:18:33Z

Are you able to identify the exact cases in which it happens ?
Did it involve external storages ?

SergioBertolinSG · 2015-04-29T10:39:49Z

No not external storages.
Just create five versions of a file and restore the last of the dropdown. (it has just erased me 3 of the 5 versions)

PVince81 · 2015-04-29T10:41:40Z

Erasing? That's the expiration code in action. The expiration is only triggered when you do an action on the file. You were talking about additional versions ?

PVince81 · 2015-04-29T11:09:21Z

Maybe one way to test it is to look at the "data/$user/files_versions" folder and look at the timestamps "filename.txt.v123456" (the number is the timestamp).
When restoring a version, for example "456789", it should:

Make a backup of "filename.txt" by creating a version for it with its current timestamp
Copy "filename.txt.v456789" and replace the original "filename.txt".

So at this point, no new version is created.
If other versions unrelated to "456789" or the backup disappear from the list, it's fine. (it's the expiration logic)

SergioBertolinSG · 2015-04-29T11:51:39Z

@PVince81 Yes, that's the behaviour. After restoring last version of the dropdown having five versions of a file, in the "data/$user/files_versions" folder there is the backup of the filename.txt, and another version which was the third one. The rest were erased.

So this is the expected behaviour of versions?
Even if your quota used is close to 0%?

icewind1991 · 2015-04-29T11:55:45Z

apps/files_versions/lib/storage.php

+		$target = $storage2->fopen($internalPath2, 'w');
+		// FIXME: might need to use part file to avoid concurrent writes
+		// (this would be an issue anyway when renaming/restoring cross-storage)
+		list(, $result) = \OC_Helper::streamCopy($source, $target);


use moveFromStorage now that's in master?

Are you saying I could also get rid of the $storage1 === $storage2 case that way ?
I'll have a try later.

scrutinizer-notifier · 2015-04-30T14:24:19Z

A new inspection was created.

PVince81 · 2015-04-30T14:24:50Z

Rebased onto master.

@icewind1991 I replaced streamCopy with moveFromStorage.
Unit tests still work, restoring version still keeps shares in local storage or external storage.

Please rereview @SergioBertolinSG @icewind1991 @schiesbn

SergioBertolinSG · 2015-04-30T15:00:19Z

@PVince81 Tested #15468 with last changes. It is working fine 👍
I think we can discuss that deletion of versions in another issue/irc.

ghost · 2015-05-01T00:00:55Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12106/
🚀 Test PASSed.🚀

MorrisJobke · 2015-05-03T11:12:52Z

Tested and works. File ID keeps stable 👍

Keep fileid when restoring version, using stream copy

MorrisJobke · 2015-05-03T11:16:36Z

Backport in #16019

…fileid Keep fileid when restoring version, using stream copy

PVince81 added the 3 - To Review label Apr 8, 2015

PVince81 added this to the 8.1-current milestone Apr 8, 2015

PVince81 mentioned this pull request Apr 8, 2015

Files stop being shared after a recipient restores them to previous versions #15468

Closed

MorrisJobke added the 8.0-next label Apr 8, 2015

PVince81 added 1 - To develop and removed 3 - To Review labels Apr 9, 2015

PVince81 force-pushed the versions-rollbackkeepfileid branch from b0af09f to 7b53672 Compare April 16, 2015 11:10

nickvergessen removed the 8.0-next label Apr 16, 2015

PVince81 reviewed Apr 16, 2015
View reviewed changes

PVince81 added 3 - To Review and removed 1 - To develop labels Apr 24, 2015

PVince81 modified the milestones: 8.0.4-next-maintenance, 8.1-current Apr 28, 2015

PVince81 added the 8.0-next label Apr 28, 2015

icewind1991 reviewed Apr 29, 2015
View reviewed changes

Vincent Petry added 2 commits April 30, 2015 16:15

Fix version rollback to keep fileid

2228faa

Use moveFromStorage instead of streamCopy when restoring version

972e189

PVince81 force-pushed the versions-rollbackkeepfileid branch from ac9c889 to 972e189 Compare April 30, 2015 14:24

MorrisJobke added a commit that referenced this pull request May 3, 2015

Merge pull request #15486 from owncloud/versions-rollbackkeepfileid

7a3b74a

Keep fileid when restoring version, using stream copy

MorrisJobke merged commit 7a3b74a into master May 3, 2015

MorrisJobke deleted the versions-rollbackkeepfileid branch May 3, 2015 11:12

This was referenced May 3, 2015

[stable8] Keep fileid when restoring version, using stream copy #16018

Closed

[stable8] Keep fileid when restoring version, using stream copy #16019

Merged

MorrisJobke removed the 8.0-current label May 20, 2015

mmattel pushed a commit to mmattel/core that referenced this pull request May 22, 2015

Merge pull request owncloud#15486 from owncloud/versions-rollbackkeep…

5224201

…fileid Keep fileid when restoring version, using stream copy

MorrisJobke added the 8.0.6-current label Jul 16, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep fileid when restoring version, using stream copy #15486

Keep fileid when restoring version, using stream copy #15486

PVince81 commented Apr 8, 2015

PVince81 commented Apr 8, 2015

PVince81 commented Apr 8, 2015

karlitschek commented Apr 8, 2015

SergioBertolinSG commented Apr 9, 2015

PVince81 commented Apr 9, 2015

SergioBertolinSG commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 9, 2015

nickvergessen commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 16, 2015

PVince81 Apr 16, 2015

PVince81 commented Apr 24, 2015

PVince81 commented Apr 28, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

icewind1991 Apr 29, 2015

PVince81 Apr 29, 2015

scrutinizer-notifier commented Apr 30, 2015

PVince81 commented Apr 30, 2015

SergioBertolinSG commented Apr 30, 2015

ghost commented May 1, 2015

MorrisJobke commented May 3, 2015

MorrisJobke commented May 3, 2015

Keep fileid when restoring version, using stream copy #15486

Keep fileid when restoring version, using stream copy #15486

Conversation

PVince81 commented Apr 8, 2015

PVince81 commented Apr 8, 2015

PVince81 commented Apr 8, 2015

karlitschek commented Apr 8, 2015

SergioBertolinSG commented Apr 9, 2015

PVince81 commented Apr 9, 2015

SergioBertolinSG commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 9, 2015

nickvergessen commented Apr 9, 2015

PVince81 commented Apr 9, 2015

PVince81 commented Apr 16, 2015

PVince81 Apr 16, 2015

Choose a reason for hiding this comment

PVince81 commented Apr 24, 2015

PVince81 commented Apr 28, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

PVince81 commented Apr 29, 2015

PVince81 commented Apr 29, 2015

SergioBertolinSG commented Apr 29, 2015

icewind1991 Apr 29, 2015

Choose a reason for hiding this comment

PVince81 Apr 29, 2015

Choose a reason for hiding this comment

scrutinizer-notifier commented Apr 30, 2015

PVince81 commented Apr 30, 2015

SergioBertolinSG commented Apr 30, 2015

ghost commented May 1, 2015

MorrisJobke commented May 3, 2015

MorrisJobke commented May 3, 2015