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

[SergioBertolinSG]

Steps to reproduce

Having a OC server installed and two users, User1 and User2.

1. User1 creates a text file "hola.txt" and edits it several times. There are versions of that file.
2. User1 shares "hola.txt" with User2.
3. User2 opens the versions dropdown of the shared file and chooses to revert that file to a previous version.

Expected behaviour

File returns to previous version and keeps shared.

Actual behaviour

File returns to previous version but it stops being shared, so User2 stops seeing the file.

Server configuration

Operating system:
Ubuntu 14.04 LTS

Web server:
Apache

Database:
MySQL

PHP version:
5.5.9

ownCloud version: (see ownCloud admin page)
{"installed":true,"maintenance":false,"version":"8.0.3.0","versionstring":"8.0.3 RC1","edition":"Enterprise"}

Updated from an older ownCloud or fresh install:
Fresh

List of activated apps:
Default in enterprise version.

The content of config/config.php:

Are you using external storage, if yes which one: local/smb/sftp/...
no

Are you using encryption:
no

Logs

{"reqId":"6203f327a69889c45455b29fd2965357","remoteAddr":"10.40.40.45","app":"PHP","message":"Invalid argument supplied for foreach() at \/var\/www\/html\/enterprise_803RC1\/lib\/private\/files\/cache\/scanner.php#226","level":3,"time":"2015-04-08T11:53:12+00:00"}
{"reqId":"6203f327a69889c45455b29fd2965357","remoteAddr":"10.40.40.45","app":"PHP","message":"Undefined index:  at \/var\/www\/html\/enterprise_803RC1\/apps\/files_sharing\/lib\/cache.php#414","level":3,"time":"2015-04-08T11:53:12+00:00"}
{"reqId":"6203f327a69889c45455b29fd2965357","remoteAddr":"10.40.40.45","app":"PHP","message":"Call to a member function correctFolderSize() on a non-object at \/var\/www\/html\/enterprise_803RC1\/apps\/files_sharing\/lib\/cache.php#415","level":3,"time":"2015-04-08T11:53:12+00:00"}

Client configuration

browser
Chrome 41

Operating system
Mac OS X

[nickvergessen]

@PVince81 stable file id issue?

[PVince81]

Hmmmm, possibly.

@schiesbn does restoring a version creates a new file cache entry ?

If yes we need to fix the versions app to keep it.

@SergioBertolinSG did you check if that worked on OC 6 / OC 7 ? (to make sure whether it's a regression or not, I thought it used to work in the past)

[PVince81]

Ref: #13310 File ids must stay stable

[SergioBertolinSG]

In version 7 it is working fine, is a regression.

[PVince81]

Thanks.

@DeepDiver1975 setting to 8.0.3

[PVince81]

Also happens on 8.0.2 and also happens when reverting a file inside a shared folder (revert as recipient)

[PVince81]

I'll try and bisect this somehow.

[PVince81]

It goes as far as #14584 from @icewind1991 which fixed reverting files.
Before that commit it's not possible to revert versions because of the bug.

Not sure yet if that PR is causing the new issue or what the difference is with OC 7.

[PVince81]

@icewind1991 I debugged a bit and it indeed seems to be related to the PR.
Now instead of moving the cache entry to the new path, this code https://github.com/owncloud/core/pull/14584/files#diff-25d7979992311b77505964464556fceaR111 will first delete the cache entry, so we lose the file id.

What was wrong with the move() method ?

I'll try and see how it was done in OC 7.0.5.

[PVince81]

In OC7 there is no code to update the cache. Strange. And the post rename proxy isn't called, maybe because the source file is in "files_versions" and moved to "files".

[PVince81]

@icewind1991 can you explain why cache move() wasn't called in OC 7 but is now in OC 8 ?
I see that the cache's move() method hasn't changed between the versions.

I guess the proper fix is to make "move()" operation properly update the element and also keep the file id. But also there's the question which fileid to keep, the source one or target one ?
In the past (in OC 7) it was the target fileid that was kept. But in some situations like moving files (not overwriting) we'd rather keep the source one.

So... tricky stuff...

[PVince81]

Another alternative is to add extra code to the versions app to reuse the original file id and pass it to the rename method.

[PVince81]

or even more barbaric: do a stream copy 😛

[PVince81]

I'll prepare a unit test to reproduce the issue until a proper solution comes to mind...

[icewind1991]

The behaviour of the cache move is correct, when overwriting foo.txt with bar.txt the fileid of the new foo.txt should be the same as the one of bar.txt not the same as the old foo.txt.

A way to solve this might be to do the rename directly on the storage and update the scan for the file manually

[PVince81]

So overwriting a file through rename should reset the file id, that makes sense. It should reuse the source file id, I think it does that already.

What do you mean with "do the response directly on the storage" ?
Maybe you meant do the rename directly on the storage, assuming that it's a local file (I hope it works with object store)

[icewind1991]

Maybe you meant do the rename directly on the storage

Yes

[PVince81]

Hmmm what if the target file is on external storage ?

[PVince81]

Just tried it: for external storage it will use the stream copy approach, which doesn't need the cache update, so it keeps the file id.

Maybe if there was a way to force a stream copy...

[PVince81]

On the other hand if we want to be consistent with the rename logic you mentioned, even a stream copy should be made to affect the file id.

[icewind1991]

I think #15360 fixes that

[PVince81]

Ok cool.

I think for this versions bug I'll stream copy the version file into the target file, always.

[PVince81]

Fix is here: #15486

[MorrisJobke]

Changed milestone according to #15486 (comment)

[MorrisJobke]

Fixed in master with #15486. Backport for 8.0.4 in #16019