Listen to federated share accept event #131

sharidas · 2019-07-31T05:55:47Z

Listen to the federated share accept event
and add the accepted share to the background
job.

Signed-off-by: Sujith H sharidasan@owncloud.com

sharidas · 2019-07-31T06:01:06Z

Description

Listen to the accept event triggered for the federated share. Prior to this change the events were not listened hence no background jobs for the same were triggered. Hence the share data was not updated in the search. With this change the event is listened and the share user is added to the background job to update the content.

By the time the event is listened the view of the user is not udpated with the accepted share. It happens when the share is accepted and the dav request is triggered. Once the dav request is tirggered we get sufficient time to execute the background job.

Issue

Fixes Search elastic does not return results for fed.shared files #126

Test done

Create a user user1 in the server1
From server2 send a federated share say file1.txt to user1 of server1.
When user1 of server1 accepts the federeated share the background job is added. Its been verified by looking into the jobs table.
Run the occ system:cron. Verified the contents of file1.txt of user1 (of server1) are displayed in the search.

codecov · 2019-07-31T06:42:54Z

Codecov Report

Merging #131 into release-1.0.0 will increase coverage by 0.26%.
The diff coverage is 22%.

@@                 Coverage Diff                 @@
##             release-1.0.0     #131      +/-   ##
===================================================
+ Coverage            19.32%   19.59%   +0.26%     
+ Complexity             273      271       -2     
===================================================
  Files                   24       24              
  Lines                 1190     1230      +40     
===================================================
+ Hits                   230      241      +11     
- Misses                 960      989      +29

Impacted Files	Coverage Δ	Complexity Δ
lib/Hooks/Files.php	`7.35% <0%> (-2%)`	`19 <16> (-2)`
lib/Application.php	`88.39% <100%> (+1.26%)`	`22 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddfe8c2...41f726b. Read the comment docs.

davitol · 2019-07-31T07:27:38Z

Tested and works 👍

micbar · 2019-07-31T07:54:10Z

lib/Hooks/Files.php

+	 * @param AcceptShare $acceptShare
+	 */
+	public static function federatedShareUpdate(AcceptShare $acceptShare) {
+		$share = $acceptShare->getShare();


Why don't we use the status mapper here?
Please use the same logic like in the contentChanged method.

While looking into the issue, my first thought was to proceed using logic like contentChanged method. But then I found an issue with accessing nodes like for example \OC::$server->getUserFolder($share['user'])->get($share['name']). And I found that the node was not there. And then I found that a PROPFIND request is made for the user, after the share is accepted and then the nodes are available. Hence I thought we can add the background job for the user, instead of using the logic of contentChanged. Let me know if this makes sense? @micbar

The propfind request is triggered from https://github.com/owncloud/core/blob/master/core/js/files/client.js#L481

So we depend on the webUI making the propfind before the cronjob runs? This is ugly.
@VicDeo Any Ideas?

By design federated shares are neither scanned nor updated before user browses inside due to performance reasons.
That's why we added owncloud/core#34933 to sync them manually by cron.

I have updated the PR with trashbin and version restore.

jvillafanez · 2019-07-31T15:33:02Z

For the trashbin and version hooks, I'd create different functions. There is also common functionality that can be reused in the contentChange function.

From what I see, the contentChange can be seen as "getNode" + "processNode". This "processNode" is the common part that can be moved to a private function and be reused, while the "getNode" is specific for each event.
Basically, I'd create a different hook for each event in order to get the related node, and then call this "processNode" function, which will be the same.

What we get with these changes is a more streamlined flow. I expect less conditions to check, which leads to less unittests to do in order to cover all scenarios.

So we depend on the webUI making the propfind before the cronjob runs? This is ugly.

This is something we can't guarantee, so we musn't rely on that.

sharidas · 2019-08-01T12:57:59Z

Updated the PR.
The initial observation with PROPFIND helping list the node, was not correct. The \OC_Util::setupFS() was doing the trick. So I have updated the code to have a tearDown and setupFS call for the user which receives the share.

lib/Hooks/Files.php

jvillafanez · 2019-08-01T13:25:09Z

lib/Hooks/Files.php

+		 * update to have the federated share added.
+		 */
+		\OC_Util::tearDownFS();
+		\OC_Util::setupFS($share['user']);


Doble-check FS operations after this hook is run are executed using the previous user, not the one shared. We might need to restore the previous user.

Its run against the recipient of the fed share, and its not done against the sender of the share.

I think it's better to include a comment.

Normally, you'd use \OC_Util::setupFS() to setup the FS for the user in the session, and \OC_Util::setupFS($user) if you need to explicitly setup the FS for a different user. At this point in the code, we might assume that we have the FS for the session user available, so it's fine to tearDown the FS.

Now, if we use \OC_Util::setupFS($share['user']), I assume that $share['user'] might not be the user in the session but a different user. If this is the case, I think we should restore the previous user.
We have to assume this callback won't be last to access to the FS. If after this callback is executed there are more calls to the FS, these calls will be run against the $share['user']'s FS, which as said, it could be a different user.

jvillafanez · 2019-08-01T13:28:48Z

lib/Hooks/Files.php

-			return;
-		}
-
+	public static function processNode($node, $userIdReceiver = null) {


Why the $userIdReceiver can be null? or, why the $userIdReceiver is needed?

Updated the method. Now it accepts userId. There is a check which I have made for @ in the code.

Listen to the federated share accept event, trashbin restore event and file version restore event and process the event to add it to the background job. Signed-off-by: Sujith H <sharidasan@owncloud.com>

jvillafanez · 2019-08-02T07:45:01Z

lib/Hooks/Files.php

+		 * update to have the federated share added.
+		 */
+		\OC_Util::tearDownFS();
+		\OC_Util::setupFS($share['user']);


I think it's better to include a comment.

Normally, you'd use \OC_Util::setupFS() to setup the FS for the user in the session, and \OC_Util::setupFS($user) if you need to explicitly setup the FS for a different user. At this point in the code, we might assume that we have the FS for the session user available, so it's fine to tearDown the FS.

Now, if we use \OC_Util::setupFS($share['user']), I assume that $share['user'] might not be the user in the session but a different user. If this is the case, I think we should restore the previous user.
We have to assume this callback won't be last to access to the FS. If after this callback is executed there are more calls to the FS, these calls will be run against the $share['user']'s FS, which as said, it could be a different user.

lib/Hooks/Files.php

sharidas · 2019-08-02T08:18:09Z

Regarding #131 (comment) I have updated the setupFS to execute on the currently logged in user. Because the recipient of the remote share is the currently logged in user.

jvillafanez

code looks good

sharidas · 2019-08-02T12:42:43Z

Not sure why codecov is responding.

sharidas requested a review from micbar July 31, 2019 06:01

sharidas self-assigned this Jul 31, 2019

sharidas added the 3 - To review label Jul 31, 2019

sharidas added this to the development milestone Jul 31, 2019

sharidas mentioned this pull request Jul 31, 2019

Search elastic does not return results for fed.shared files #126

Open

sharidas requested a review from VicDeo July 31, 2019 06:46

davitol approved these changes Jul 31, 2019

View reviewed changes

micbar suggested changes Jul 31, 2019

View reviewed changes

sharidas force-pushed the listen-fedshare-acceptevent branch 3 times, most recently from f7152d5 to 8f61a4f Compare July 31, 2019 14:15

sharidas force-pushed the listen-fedshare-acceptevent branch 2 times, most recently from 2108cab to eed7b79 Compare August 1, 2019 12:37

jvillafanez reviewed Aug 1, 2019

View reviewed changes

sharidas force-pushed the listen-fedshare-acceptevent branch 2 times, most recently from 35180f4 to 5051a76 Compare August 1, 2019 16:40

micbar force-pushed the listen-fedshare-acceptevent branch from 5051a76 to 5eb28e4 Compare August 1, 2019 19:27

Listen to federated share, trashbin and version restore event

cb6d267

Listen to the federated share accept event, trashbin restore event and file version restore event and process the event to add it to the background job. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the listen-fedshare-acceptevent branch from 5eb28e4 to 16a88cc Compare August 2, 2019 07:01

jvillafanez reviewed Aug 2, 2019

View reviewed changes

Remove stable10 test jobs

41f726b

sharidas force-pushed the listen-fedshare-acceptevent branch from 16a88cc to 41f726b Compare August 2, 2019 08:15

jvillafanez approved these changes Aug 2, 2019

View reviewed changes

micbar approved these changes Aug 2, 2019

View reviewed changes

sharidas merged commit d6ef1d2 into release-1.0.0 Aug 6, 2019

delete-merged-branch bot deleted the listen-fedshare-acceptevent branch August 6, 2019 09:20

micbar mentioned this pull request Aug 26, 2019

Release v1.0.0 #120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Listen to federated share accept event #131

Listen to federated share accept event #131

sharidas commented Jul 31, 2019

sharidas commented Jul 31, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

davitol commented Jul 31, 2019

micbar Jul 31, 2019

sharidas Jul 31, 2019

sharidas Jul 31, 2019

micbar Jul 31, 2019

VicDeo Jul 31, 2019 •

edited

Loading

sharidas Jul 31, 2019

jvillafanez commented Jul 31, 2019

sharidas commented Aug 1, 2019

jvillafanez Aug 1, 2019

sharidas Aug 1, 2019

jvillafanez Aug 2, 2019

jvillafanez Aug 1, 2019

sharidas Aug 1, 2019

jvillafanez Aug 2, 2019

sharidas commented Aug 2, 2019

jvillafanez left a comment

sharidas commented Aug 2, 2019

Listen to federated share accept event #131

Listen to federated share accept event #131

Conversation

sharidas commented Jul 31, 2019

sharidas commented Jul 31, 2019

Description

Issue

Test done

codecov bot commented Jul 31, 2019 • edited Loading

Codecov Report

davitol commented Jul 31, 2019

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

VicDeo Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Jul 31, 2019

sharidas commented Aug 1, 2019

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

sharidas commented Aug 2, 2019

jvillafanez left a comment

Choose a reason for hiding this comment

sharidas commented Aug 2, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

VicDeo Jul 31, 2019 •

edited

Loading