Propagate etags across shared storages #14764

icewind1991 · 2015-03-09T15:26:02Z

Solution involves three parts

Watch for filesystem changes made inside a shared mount, when detected, trigger the change propagator for the owner of the storage
Listen to the change propagator, when it updates the etag of a shared item, mark the shares as "dirty"
When setting up the filesystem, trigger the change propagator for each "dirty" mountpoint

cc @PVince81 @DeepDiver1975

icewind1991 · 2015-03-09T15:26:54Z

@PVince81 I think I have all distinct cases covered by the unit tests, please check if you can think of a situation not covered

MorrisJobke · 2015-03-09T15:43:03Z

@icewind1991 This was a quick merge conflict ... rebase required ;)

DeepDiver1975 · 2015-03-09T21:49:43Z

apps/files_sharing/lib/propagator.php

+		if ($time === null) {
+			$time = time();
+		}
+		$this->config->setAppValue('files_sharing', $share['id'], $time);


Will this ever be removed again? Looks like we are going to pollute the appconfig table

I could move the timestamp to the share table (requires schema change) or use an unshare hook to cleanup

I remember we have a similar mechanism for external storages. Maybe that would be a candidate for a future new table for mount points ? So I'm not sure we should move it to the share table now.

Here is the files_external version: https://github.com/owncloud/core/blob/master/apps/files_external/lib/etagpropagator.php#L64

Maybe using oc_share isn't that bad an idea. At least it prevents requiring ALL share ids and makes it possible to only query the ones that have the dirty flag set.

Schema change: yes, if for 8.1.
I have the feeling that this whole fix isn't backportable anyway. 😦

We already have a list of all shares from setting up the mounts

PVince81 · 2015-03-10T14:54:06Z

Had a quick look and it confirms what I thought: the fix for this will big and not backportable.
I still wonder how we managed to make it work in OC 6. Maybe it was because all you had to do is update the etag of the "Shared" folder for all users (if we ever did...)

DeepDiver1975 · 2015-03-10T15:09:46Z

Maybe it was because all you had to do is update the etag of the "Shared" folder for all users (if we ever did...)

yes - every user had a pseudo etag for the shared folder in his preferences. This approach no longer works.

PVince81 · 2015-03-12T11:26:13Z

apps/files_sharing/lib/propagator.php

@@ -0,0 +1,138 @@
+<?php
+/**
+ * Copyright (c) 2014 Robin Appelman <[email protected]>


Man, you live in the past!

PVince81 · 2015-03-12T11:35:01Z

@icewind1991 did you do a blackfire compare before and after this branch to find potential perf issues ? Performance is my first worry, second one is backportability.

PVince81 · 2015-03-12T11:38:01Z

apps/files_sharing/lib/propagator.php

+		$shares = Share::getAllSharesForOwner($owner);
+		$propagator->listen('\OC\Files', 'propagate', function ($path, $entry) use ($shares) {
+			foreach ($shares as $share) {
+				if ((int)$share['file_source'] === $entry['fileid']) {


Hmmm wait, why do we need all shares ?
We only need all shares that have file_source = fileid, probably doable with a custom SQL statement.

Maybe getUsersItemShare() could get you all recipients https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L506

And use getUsersSharingFile https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L102 for reshares, etc that will need update as well.

PVince81 · 2015-03-12T12:14:34Z

icewind1991 · 2015-03-12T15:02:40Z

Did some refactoring, splitting up classes and getting rid of static stuff

DeepDiver1975 · 2015-03-16T13:31:49Z

apps/files_sharing/lib/propagation/propagator.php

+	 * @param string $owner
+	 */
+	public function attachToPropagator(ChangePropagator $propagator, $owner) {
+		$shares = Share::getAllSharesForOwner($owner);


We really need to query all shares - even if the closure below is never called?

changed it to query on demand

PVince81 · 2015-04-27T12:45:26Z

Happens here:

0  OC_Share_Backend_File::getSource() /srv/www/htdocs/owncloud/apps/files_sharing/lib/share/file.php:228
1  OC\Files\Cache\Shared_Cache->getSourceCache() /srv/www/htdocs/owncloud/apps/files_sharing/lib/cache.php:63
2  OC\Files\Cache\Shared_Cache->get() /srv/www/htdocs/owncloud/apps/files_sharing/lib/cache.php:98
3  OC\Files\Cache\ChangePropagator->propagateChanges() /srv/www/htdocs/owncloud/lib/private/files/cache/changepropagator.php:78
4  OCA\Files_Sharing\Propagation\ChangeWatcher->propagateForOwner() /srv/www/htdocs/owncloud/apps/files_sharing/lib/propagation/changewatcher.php:73
5  OCA\Files_Sharing\Propagation\ChangeWatcher->writeHook() /srv/www/htdocs/owncloud/apps/files_sharing/lib/propagation/changewatcher.php:40
6  call_user_func:{/srv/www/htdocs/owncloud/lib/private/hook.php:103}() /srv/www/htdocs/owncloud/lib/private/hook.php:103
7  OC_Hook::emit() /srv/www/htdocs/owncloud/lib/private/hook.php:103
8  OC\Files\View->emit_file_hooks_post() /srv/www/htdocs/owncloud/lib/private/files/view.php:513
9  OC\Files\View->file_put_contents() /srv/www/htdocs/owncloud/lib/private/files/view.php:543
10 OC\Files\View->fromTmpFile() /srv/www/htdocs/owncloud/lib/private/files/view.php:840
11 OC\Files\Filesystem::fromTmpFile() /srv/www/htdocs/owncloud/lib/private/files/filesystem.php:700
12 require_once()  /srv/www/htdocs/owncloud/apps/files/ajax/upload.php:191
13 __lambda_func() /srv/www/htdocs/owncloud/lib/private/route/route.php%28153%29%20:%20runtime-created%20function:1
14 call_user_func:{/srv/www/htdocs/owncloud/lib/private/route/router.php:272}() /srv/www/htdocs/owncloud/lib/private/route/router.php:272
15 OC\Route\Router->match() /srv/www/htdocs/owncloud/lib/private/route/router.php:272
16 OC::handleRequest() /srv/www/htdocs/owncloud/lib/base.php:853
17 {main}          /srv/www/htdocs/owncloud/index.php:40

PVince81 · 2015-04-27T12:55:16Z

@icewind1991 it looks like this is also triggered by the unit test "testReshareRecipientWritesToReshare" which matches the case. The test doesn't fail because it's only logged as DEBUG. Maybe the consequence is harmless or the situation is expected.
We should still add some checks to avoid logging this (or even going this code path) when expected.

icewind1991 · 2015-04-27T13:04:32Z

Ok, can reproduce it now, since it's just a debug message and doesnt seem to affect the propagation can we merge this PR and raise a new issue for the message

PVince81 · 2015-04-27T13:07:59Z

Ok. But I noticed that the unit test don't check the etag of "folder", I want to double check because I'm not certain that it is harmless.

PVince81 · 2015-04-27T13:09:20Z

@MorrisJobke review ?

PVince81 · 2015-04-27T13:13:27Z

I checked with a manual test, the etag of "folder" changes properly. So not sure about the consequences. During my quick debug session I had the feeling it would not propagate to owner (which is the code path where the warning appears) but somehow it still manages to propagate there, possibly through other means.

👍 for now, will raise the next ticket(s) as soon as this is merged

scrutinizer-notifier · 2015-04-27T13:18:39Z

A new inspection was created.

icewind1991 · 2015-04-27T15:00:01Z

Tests passed on jenkins pr

PVince81 · 2015-04-28T08:26:39Z

@nickvergessen @LukasReschke @DeepDiver1975 @MorrisJobke please review.

Test plan is here: #14764 (comment) (and also covered by the unit tests)

MorrisJobke · 2015-04-28T08:38:59Z

I will do the review and merge once I'm fine with this.

MorrisJobke · 2015-04-28T08:58:45Z

I tested most of the cases above and all succeed. I also couldn't find any errors in the logs.

Awesome work @PVince81 @icewind1991 🚀 🚢 🇮🇹 👍

Propagate etags across shared storages

PVince81 · 2015-04-28T08:59:40Z

I will take care or raising tickets for the remaining issues.

PVince81 · 2015-04-28T09:00:52Z

We should also look into providing a simple backportable solution for older versions, but for that we need to first indentify which cases were exactly broken. Maybe using the "etagpropagation" unit test from this PR could help.

PVince81 · 2015-04-28T09:01:02Z

@karlitschek ^

MorrisJobke · 2015-04-28T09:03:10Z

To share the test setup with you all - Outside the ❄️ is falling and I'm in the 🚌 and my 💻 looks like this:

PVince81 · 2015-04-28T10:36:17Z

Raised remaining tickets:

new warnings in log: Log entry file source not found in log when uploading to inner reshares #15909
lazy propagate for all users except current one to reduce cache updates: Lazy propagate etags to all users except current one #15910
missing etag propagations when unsharing: Etag change on unshare misses the first parent folder etag #15813

LukasReschke · 2015-04-28T12:31:55Z

💣 💣 💣

Broke sharing: #15913

💣 💣 💣

Propagate etags across shared storages

icewind1991 force-pushed the shared-etag-propagate branch from 60df28b to 174eda0 Compare March 9, 2015 16:04

DeepDiver1975 added this to the 8.1-current milestone Mar 9, 2015

DeepDiver1975 added the 3 - To Review label Mar 9, 2015

DeepDiver1975 reviewed Mar 9, 2015
View reviewed changes

PVince81 reviewed Mar 12, 2015
View reviewed changes

icewind1991 force-pushed the shared-etag-propagate branch from 174eda0 to e8e9948 Compare March 12, 2015 14:59

DeepDiver1975 reviewed Mar 16, 2015
View reviewed changes

PVince81 mentioned this pull request Mar 17, 2015

Etag doesn't propagate across shared storage boundaries #14726

Closed

dont pass floats as timestamp to the changepropagator

5304afb

MorrisJobke added a commit that referenced this pull request Apr 28, 2015

Merge pull request #14764 from owncloud/shared-etag-propagate

de8c15e

Propagate etags across shared storages

MorrisJobke merged commit de8c15e into master Apr 28, 2015

MorrisJobke deleted the shared-etag-propagate branch April 28, 2015 08:58

This was referenced Apr 28, 2015

Log entry file source not found in log when uploading to inner reshares #15909

Closed

Lazy propagate etags to all users except current one #15910

Closed

PVince81 mentioned this pull request Apr 28, 2015

Document etag propagation for developers owncloud-archive/documentation#1089

Closed

LukasReschke mentioned this pull request Apr 28, 2015

Public sharing broken on master #15913

Closed

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

Merge pull request owncloud#14764 from owncloud/shared-etag-propagate

d294fd6

Propagate etags across shared storages

PVince81 mentioned this pull request Aug 3, 2015

Client won't sync when files are put in a shared folder #12713

Closed

PVince81 mentioned this pull request Nov 16, 2015

Integration tests for etag propagation #20521

Closed

jvillafanez mentioned this pull request Nov 18, 2015

Sharing etag propagation tests owncloud/smashbox#136

Merged

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

Propagate etags across shared storages #14764

Propagate etags across shared storages #14764

Conversation

icewind1991 commented Mar 9, 2015

icewind1991 commented Mar 9, 2015

MorrisJobke commented Mar 9, 2015

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

Choose a reason for hiding this comment

PVince81 commented Mar 10, 2015

DeepDiver1975 commented Mar 10, 2015

Choose a reason for hiding this comment

PVince81 commented Mar 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 12, 2015

Test plan

Owner O shares "test" with recipients R1 and R2

R2 reshares "test" with R3

R2 reshares subdir "test/sub" with R4

Groups

icewind1991 commented Mar 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 27, 2015

PVince81 commented Apr 27, 2015

icewind1991 commented Apr 27, 2015

PVince81 commented Apr 27, 2015

PVince81 commented Apr 27, 2015

PVince81 commented Apr 27, 2015

scrutinizer-notifier commented Apr 27, 2015

icewind1991 commented Apr 27, 2015

PVince81 commented Apr 28, 2015

MorrisJobke commented Apr 28, 2015

MorrisJobke commented Apr 28, 2015

PVince81 commented Apr 28, 2015

PVince81 commented Apr 28, 2015

PVince81 commented Apr 28, 2015

MorrisJobke commented Apr 28, 2015

PVince81 commented Apr 28, 2015

LukasReschke commented Apr 28, 2015