Fix file cache updater #5513

schiessle · 2013-10-24T10:40:18Z

This fixes bug #5098
For a detailed explanation how to reproduce it look at: owncloud/client#1056 (comment)

@icewind1991 please have a look. correctFolder() always assumed that we are operating on a folder owned by the current user which of course is wrong in case of a shared file. That's why I changed resolvePath() to resolve the path to the owners path and updated the correctFolder() method to loop though the correct folder tree.

Also affects master, will port it once the fix is merged for stable5.

…oesn't work for Shared files correctFolder() correct folder for the owner of the file. The Shared folder for the other users gets updated by the files_sharing app

bantu · 2013-10-24T11:28:24Z

Please use constants instead of magic numbers (-1).

schiessle · 2013-10-24T11:31:12Z

There is no constant. That's how it is done in other places of the filecache implementation, too.

bantu · 2013-10-24T11:32:52Z

Oh, this is stable5. Can we at least make that a constant in master?

schiessle · 2013-10-24T11:51:12Z

Oh, this is stable5. Can we at least make that a constant in master?

I don't want to make larger changes to the file cache without discussing it with @icewind1991.
Let's focus on the actual bug fix, also because we are not that far away from OC6. At this stage of OC6 I would prefer to only change stuff which really needs to be changed (aka bug fixes)

ghost · 2013-10-24T12:11:49Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1766/

schiessle · 2013-10-24T13:12:21Z

Any idea why this tests are failing... I don't see how this is connected to my changes.

For example this error is obvious: https://ci.owncloud.org/job/pull-request-analyser/1766/testReport/junit/%28root%29/Test_Files_Cache_Scanner/testFile/ Here https://github.com/owncloud/core/blob/stable5/lib/files/cache/cache.php#L111 we cast some of the results to int but not 'parent' so I'm not surprised that the test which compares the result against an integer fails. But why does it only fails for my pull request. Did I missed something?

schiessle · 2013-10-24T14:13:47Z

If I cast the parent to integer on my local system the test still fails, than with the message:

1) Test\Files\Cache\Scanner::testFile
Failed asserting that -1 is not equal to -1.

Strange. 😨

ghost · 2013-10-24T16:12:01Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1772/

schiessle · 2013-10-25T08:03:03Z

failed test might be related to #3568

icewind1991 · 2013-10-27T12:23:15Z

👎 the changes to correctFolder prevent etag and mtime changes from propagating across storages

DeepDiver1975 · 2013-10-27T22:04:59Z

lib/files/cache/updater.php

-				$id = $cache->getId($internalPath);
-				if ($id !== -1) {
-					$cache->update($id, array('mtime' => $time, 'etag' => $storage->getETag($internalPath)));
-					self::correctFolder($parent, $time);


We need to keep this line - this is responsible for recursive etag change

The recursive call was replaced with a while loop.

ok - thanks - I totally overlooked this.
Once more: sorry for the noise 😉

…ethod, this method always expect a path relativ to the current user

schiessle · 2013-10-28T11:05:15Z

@icewind1991 the last commits should address your concerns.

The test will probably still fail because operations like getOwner() and getFileInfo() doesn't work correctly in the test environment. To make it work we would probably have to change the tests in a way that it uses a real user and operates on a real file view. But this would be quite large changes to the tests. But I don't see a better solution. Opinions?

ghost · 2013-10-28T11:34:48Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1798/

icewind1991 · 2013-10-28T12:18:10Z

lib/files/cache/updater.php

+				//$id = $cache->getParentId($internalPath);
+				$internalPath = dirname($internalPath);
+				// check storage for parent in case we change the storage in this step
+				list($storage, $internalPath) = $view->resolvePath($internalPath);


This will go wrong, you need to call dirname on $path and use that in the resolvePath call.

Internal path is relative to the storage and thus will never go outside the storage with dirname

I'm not sure. We need to update the whole tree from the original path (internalPath). Imagine following situation:

user1 has /folder1/folder2/folder3 and shares folder3 with user2. For user2 this will look like /Shared/folder3 and that's also the path we will receive here, for example if user2 uploads a file to the shared folder. dirname() from $path would return Shared but we want to iterate over /folder1/folder2/folder3 to update the original path. That's why we need to iterate over the internalPath which is the correct /folder1/folder2/folder3

I also tried it both with nested Shares and with nested folder from the same user. It works fine in real world. The only problem are the tests.

Internal path is relative to the storage and thus will never go outside the storage with dirname

I think I see your point... But how do we solve it correctly? As shown with the example above we need to iterate over the "real path" to update the owners path correctly.

I think the crucial point is that I need to get somehow the complete path from the owner of a file/folder independent for any storage which might be mounted in between. Does our file system provides any method to get such a path? I couldn't find one until now.

user1 has /folder1/folder2/folder3 and shares folder3 with user2. For user2 this will look like /Shared/folder3 and that's also the path we will receive here, for example if user2 uploads a file to the shared folder. dirname() from $path would return Shared but we want to iterate over /folder1/folder2/folder3 to update the original path. That's why we need to iterate over the internalPath which is the correct /folder1/folder2/folder3

Internal path won't be /folder1/folder2/folder3 though for a shared file, it would be folder3 because it's relative to the shared storage

During my tests this PR's resolvePath() returned /folder1/folder2/folder3, keep in mind that I also did some small changes to resolvePath() which you can see above.

…to be updated check the storage for every iteration to make sure that we update the correct cache entry

schiessle · 2013-10-29T08:59:53Z

OK, now I think I took all possible situations into account.

Storage changes are addressed by calling resolvePath() for every iteration (tested)
nested Shared folders are working because at the beginning we make sure that we operate on the owners path and iterate over it. (tested)

ghost · 2013-10-29T09:30:13Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1819/

…sn't use a completely initialized oc filesystem

ghost · 2013-10-29T11:38:41Z

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1822/

schiessle · 2013-10-29T11:47:56Z

Yea, finally tests are passing as well. 😄

@icewind1991 Can you please have a final look? Thanks!

icewind1991 · 2013-10-29T12:33:40Z

👍 looks good now

schiessle · 2013-10-29T12:55:25Z

Great! Can I have a second reviewer? Maybe @DeepDiver1975 or @PVince81 ? Thanks!

karlitschek · 2013-10-29T13:17:14Z

looks good 👍

PVince81 · 2013-10-29T13:27:05Z

lib/files/cache/updater.php

+	 * @return array with the oweners uid and the owners path
+	 */
+	public static function getUidAndFilename($filename) {
+		$uid = \OC\Files\Filesystem::getOwner($filename);


Would it make sense to move this to the FileSystem or View class instead of the Updater ? This way it can be used in other places where the user might be needed ?

Probably not. Here we need the path relative to data/user in most other cases you would probably need the path relative to data/user/files

Fix file cache updater

Fix file cache updater (backport to master of #5513)

Bjoern Schiessle added 2 commits October 24, 2013 12:26

return -1 if file is empty otherwise we run into infinite loops

80ec546

resolvePath() needs to resolve the path for the owner, otherwise it d…

ecdba3d

…oesn't work for Shared files correctFolder() correct folder for the owner of the file. The Shared folder for the other users gets updated by the files_sharing app

Bjoern Schiessle added 2 commits October 24, 2013 16:14

remove debug code

e897523

changed the logic, this should fix the tests

765ab9d

DeepDiver1975 reviewed Oct 27, 2013
View reviewed changes

Bjoern Schiessle added 3 commits October 28, 2013 11:37

check storage for every iteration

f6e04c9

Share Updater doesn't need to call the cache updaters correctFolder m…

bf65728

…ethod, this method always expect a path relativ to the current user

remove debug output, add comment

c8bcdce

icewind1991 reviewed Oct 28, 2013
View reviewed changes

make sure that we operate on the owners Path because this path needs …

1de813f

…to be updated check the storage for every iteration to make sure that we update the correct cache entry

tests should work if I explicitely log-in as a user

5cf8c84

Bjoern Schiessle added 2 commits October 29, 2013 11:45

make sure that hooks are only executed when the app is enabled

392ed5c

disable encryption app for file cache updater test since the test doe…

b6249ea

…sn't use a completely initialized oc filesystem

PVince81 reviewed Oct 29, 2013
View reviewed changes

make getUidAndFilename() private

91a11e2

schiessle added a commit that referenced this pull request Oct 29, 2013

Merge pull request #5513 from owncloud/fix_file_cache_updater

072b827

Fix file cache updater

schiessle merged commit 072b827 into stable5 Oct 29, 2013

schiessle mentioned this pull request Oct 29, 2013

Fix file cache updater (backport to master of #5513) #5603

Merged

schiessle deleted the fix_file_cache_updater branch October 29, 2013 15:00

PVince81 pushed a commit that referenced this pull request Oct 30, 2013

Merge pull request #5603 from owncloud/fix_file_cache_updater_master

de342a5

Fix file cache updater (backport to master of #5513)

schiessle mentioned this pull request Oct 30, 2013

ETag change of folder 'Shared' is not propagated to the root folder #5543

Closed

schiessle pushed a commit that referenced this pull request Oct 30, 2013

backport of #5513

fcfac51

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix file cache updater #5513

Fix file cache updater #5513

schiessle commented Oct 24, 2013

bantu commented Oct 24, 2013

schiessle commented Oct 24, 2013

bantu commented Oct 24, 2013

schiessle commented Oct 24, 2013

ghost commented Oct 24, 2013

schiessle commented Oct 24, 2013

schiessle commented Oct 24, 2013

ghost commented Oct 24, 2013

schiessle commented Oct 25, 2013

icewind1991 commented Oct 27, 2013

DeepDiver1975 Oct 27, 2013

schiessle Oct 28, 2013

DeepDiver1975 Oct 28, 2013

schiessle commented Oct 28, 2013

ghost commented Oct 28, 2013

icewind1991 Oct 28, 2013

schiessle Oct 28, 2013

schiessle Oct 28, 2013

icewind1991 Oct 28, 2013

schiessle Oct 28, 2013

schiessle commented Oct 29, 2013

ghost commented Oct 29, 2013

ghost commented Oct 29, 2013

schiessle commented Oct 29, 2013

icewind1991 commented Oct 29, 2013

schiessle commented Oct 29, 2013

karlitschek commented Oct 29, 2013

PVince81 Oct 29, 2013

schiessle Oct 29, 2013

Fix file cache updater #5513

Fix file cache updater #5513

Conversation

schiessle commented Oct 24, 2013

bantu commented Oct 24, 2013

schiessle commented Oct 24, 2013

bantu commented Oct 24, 2013

schiessle commented Oct 24, 2013

ghost commented Oct 24, 2013

schiessle commented Oct 24, 2013

schiessle commented Oct 24, 2013

ghost commented Oct 24, 2013

schiessle commented Oct 25, 2013

icewind1991 commented Oct 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Oct 28, 2013

ghost commented Oct 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Oct 29, 2013

ghost commented Oct 29, 2013

ghost commented Oct 29, 2013

schiessle commented Oct 29, 2013

icewind1991 commented Oct 29, 2013

schiessle commented Oct 29, 2013

karlitschek commented Oct 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment