Optimize resolveGroupShare #27434

mrow4a · 2017-03-20T16:32:54Z

This optimizes the case in which you do PROPFIND to some folder, in which you had group share, and that share was moved to another folder (generaly people reorganise stuff from root folder to their required folders)

Unit tests
Verify integration tests

Below is what happens when you move file testgrshare.txt which has some group shares receiver/send to another folder and then propfind the "old" root folder:

mrow4a · 2017-03-20T17:12:15Z

@DeepDiver1975 @SergioBertolinSG @PVince81 Do we have integration for this scenario of moving group share to other folder, group share folder, fed share whatever?

Unit test cover the changes already.

PVince81 · 2017-03-21T07:42:56Z

lib/private/Share20/DefaultShareProvider.php

-	 * Resolve a group share to a user specific share
-	 * Thus if the user moved their group share make sure this is properly reflected here.
+	 * Resolve a group shares to a user specific share.
+	 * Returns in the array both the updated share if one was found and for not found in DB passing predicate, the original shares.


mind clarifying what you mean with "updated share" and "original share" ?

Do you mean that "original share" is the actual group share and "updated share" is the user-specific special entry ?

Hmm, ok, I should say resolved :>

an the line is too long - 80 chars please

SergioBertolinSG · 2017-03-21T07:44:27Z

@mrow4a There are quite a few int. tests about sharing, perhaps the test you are looking for is this one https://github.com/owncloud/core/blob/master/tests/integration/features/sharing-v1.feature#L522 .

If you miss some specific case, please tell me / open a ticket in QA repo.

DeepDiver1975 · 2017-03-21T10:07:26Z

lib/private/Share20/DefaultShareProvider.php

-	 * Resolve a group share to a user specific share
-	 * Thus if the user moved their group share make sure this is properly reflected here.
+	 * Resolve a group shares to a user specific share.
+	 * Returns in the array both the updated share if one was found and for not found in DB passing predicate, the original shares.


an the line is too long - 80 chars please

DeepDiver1975 · 2017-03-21T10:08:06Z

lib/private/Share20/DefaultShareProvider.php

 		$qb = $this->dbConn->getQueryBuilder();

-		$stmt = $qb->select('*')
+		$shareParentIds = array_keys($parentIdToShareMap);


chunk to 1000 max?

PVince81 · 2017-03-21T15:58:36Z

lib/private/Share20/DefaultShareProvider.php

-			->setMaxResults(1)
-			->execute();
+		$shareParentIds = [];
+		$parentIdToShareMap = [];


Rename to shareIdToShareMap or simply sharesMap ? Because it seems that's what it is.
It can of course be used to resolve parents but that's only one possibility
The "parent" bit confused me.

PVince81 · 2017-03-21T15:59:15Z

lib/private/Share20/DefaultShareProvider.php

-			))
-			->setMaxResults(1)
-			->execute();
+		$shareParentIds = [];


I have trouble understanding the chunking part, it seems this array stores chunks so call it something with "chunk" in its name. See comment below to possibly avoid saying "parent".

PVince81 · 2017-03-21T16:00:15Z

lib/private/Share20/DefaultShareProvider.php

+			} else {
+				$shareNo++;
+			}
+			$shareParentIds[$chunkId][] = $parentId;


Can you explain the chunking part with code comments ?

It seems it's basically just building a chunked array of share ids ?
The "parent" part in the name makes it more confusing, I suggest removing it.

$parentId is just $shareId.

yep, it just optimised for this case array_chunk, avoiding unnecessary loops.

PVince81 · 2017-03-21T16:04:18Z

@mrow4a code looks good. Took me a while due to the variable names confusing me.
Please adjust.

jvillafanez · 2017-03-21T17:28:30Z

lib/private/Share20/DefaultShareProvider.php

+		$shareIdToShareMap = [];
+		$chunkId = 0;
+		$shareNo = 0;
+		foreach($shares as $share) {


Move this to another function so we can unittest it?

TO unit tests it is to have >100 shares

https://github.com/owncloud/core/pull/27434/files#diff-88730505cd190b0b2f8b43ded358feb9R1144 and did it already

This part of code involves chunking the shares list, which isn't related to the DB access that the rest of the function perform. That another reason to extract this piece out.

I have no idea what you want to achieve here, I need to contruct $shareIdToShareMap and $chunkedShareIds, and this is what the loop is doing. I need that to avoid unnecesairly loops over shares

I need to contruct $shareIdToShareMap and $chunkedShareIds

My point is that another private function could take care of the chunking in order to make this function to focus on fetching the data from the DB. It's fine if this function calls the "chunking function" and then fetch the data.

This function would need to return 2 arrays...

Ok, will try it and see how it looks like. I get your point.

@jvillafanez Addressed this in the commit, wrote function chunkSharesToMaps()

jvillafanez · 2017-03-21T17:34:16Z

lib/private/Share20/DefaultShareProvider.php

-
-		return $share;
+		
+		$resolvedShares = array_values($shareIdToShareMap);


is the array_values needed?

Hmm good question, how array merge will handle it in upper layer. This is basicaly converting from map to classical array, which is used in upper layer

$shareId = $data['parent']; I guess this won't be unqiue, will it? But if it won't be unique, the check above (if (isset($shareIdToShareMap[$shareId]))) could be problematic.... I need some light there.

Parent is basicaly unique share ID. It is needed to track received shares moved over user folders, as in the picture in the first post. Below are DB entries to enlighten you.

but there could be several entries with the same parent.... that's what worries me.

I am discussing this with @cdamken lets see what comes out

Ok we tried things and seems impossible

If consider the parent will be unique, I think it's better to make the consideration all the way through, so if the assumption is wrong 💥 and 🚑 , otherwise it will be a pain to debug what's going on.

So, the key of the $shareIdToShareMap map will be unique and the array_merge (without the array_values) shouldn't be a problem. The only question is if those keys are used in any other place, if not we could ignore the array_values

I think we need array_values... the other array in upper layer is not a map

ok, fair enough. Maybe something to check in the future. Anyway 👍

jvillafanez · 2017-03-21T17:35:55Z

lib/private/Share20/DefaultShareProvider.php

+				// If we pass to resolveGroupShares map with one element, we expect to receive exactly one element, otherwise it is error
+				$share = $resolvedShares[0];
+			} else {
+				throw new \Exception("ResolveGroupShares() returned wrong result");


Any other better exception that we could throw here? I'm not fan of throwing plain exceptions

We already throw this exception in this class, and this error is basicaly WTF error which should not happen -> if developer later touches this and breaks, he will know immedietaly :> Other alternative is just to throw IndexError as default, but it will probably confuse

Will use @throws ProviderException

DeepDiver1975 · 2017-03-21T12:20:12Z

lib/private/Share20/DefaultShareProvider.php

-			$share = $this->resolveGroupShare($share, $recipientId);
+			$resolvedShares = $this->resolveGroupShares([$share], $recipientId);
+			if (count($resolvedShares) === 1){
+				// If we pass to resolveGroupShares map with one element, we expect to receive exactly one element, otherwise it is error


80 chars 😉

mrow4a · 2017-03-22T17:08:43Z

lib/private/Share20/DefaultShareProvider.php

+				if (!isset($shareParents[$shareParent])) {
+					$shareParents[] = $shareParent;
+				}else {
+					throw new ProviderException('Parent of share should be unique');


@jvillafanez I think this should be what you wanted to achieve

jvillafanez · 2017-03-22T17:22:24Z

lib/private/Share20/DefaultShareProvider.php

+				// Ensure uniquenes of parents
+				if (!isset($shareParents[$shareParent])) {
+					$shareParents[] = $shareParent;
+				}else {


sneak a whitespace here?

jvillafanez · 2017-03-22T17:25:21Z

The code is fine for me, just #27434 (review) , but unless there is any other change I don't think it's worthy.
👍 for the code.

mrow4a · 2017-03-23T08:37:55Z

I think I addressed the reviewed code @DeepDiver1975 any ideas?

PVince81 · 2017-03-23T19:36:00Z

👍 from me as well, code is way more readable now.

PVince81 · 2017-04-24T11:44:53Z

Not sure if we should backport this, it's a huge change and a bit risky. Needs proper retesting.

Thoughts ?

@cdamken @DeepDiver1975

cdamken · 2017-04-24T15:36:51Z

Long story short:

The patch was sent to the customer because it looks like the fixes the issue.
The customer have 9.1.4
The patch was applied.
The problem was fixed by the customer.

I'm not sure if cherry pick will work, but anyhow the solution works for master and 9.1.4 too.

tomneedham · 2017-05-23T15:02:55Z

Agreed to not backport due to the risk. Advise to update to 10 or can provide this patch to other customers on 9.1.4 if they really want - with the risk that comes with it.

cdamken · 2017-05-23T17:17:34Z

Ok, I asked to upgrade to 10.

lock · 2019-08-03T08:02:05Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mrow4a mentioned this pull request Mar 20, 2017

Optimize join for sharedWith by moving logic to DB #27436

Merged

2 tasks

PVince81 added the 3 - To Review label Mar 21, 2017

PVince81 added this to the 10.0 milestone Mar 21, 2017

PVince81 reviewed Mar 21, 2017

View reviewed changes

DeepDiver1975 requested changes Mar 21, 2017

View reviewed changes

mrow4a force-pushed the optimize_resolvegroupshare branch 2 times, most recently from bb6291e to 63b95a4 Compare March 21, 2017 12:52

PVince81 reviewed Mar 21, 2017

View reviewed changes

mrow4a force-pushed the optimize_resolvegroupshare branch from 63b95a4 to 1b4520c Compare March 21, 2017 16:53

jvillafanez reviewed Mar 21, 2017

View reviewed changes

DeepDiver1975 requested changes Mar 21, 2017

View reviewed changes

mrow4a force-pushed the optimize_resolvegroupshare branch 4 times, most recently from 3297113 to a14d271 Compare March 22, 2017 17:07

mrow4a commented Mar 22, 2017

View reviewed changes

mrow4a force-pushed the optimize_resolvegroupshare branch from a14d271 to d195262 Compare March 22, 2017 17:11

jvillafanez reviewed Mar 22, 2017

View reviewed changes

Optimize resolveGroupShare

c64f0e4

mrow4a force-pushed the optimize_resolvegroupshare branch from d195262 to c64f0e4 Compare March 22, 2017 17:38

PVince81 merged commit ac842f6 into master Mar 23, 2017

PVince81 deleted the optimize_resolvegroupshare branch March 23, 2017 19:36

PVince81 added the backport-request label Apr 24, 2017

cdamken added the blue-ticket label Apr 24, 2017

cdamken removed the backport-request label May 23, 2017

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


		return $share;

		$resolvedShares = array_values($shareIdToShareMap);

Optimize resolveGroupShare #27434

Optimize resolveGroupShare #27434

Conversation

mrow4a commented Mar 20, 2017 • edited Loading

mrow4a commented Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBertolinSG commented Mar 21, 2017

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

Choose a reason for hiding this comment

PVince81 commented Mar 21, 2017

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

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

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

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

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

jvillafanez commented Mar 22, 2017

mrow4a commented Mar 23, 2017

PVince81 commented Mar 23, 2017 • edited Loading

PVince81 commented Apr 24, 2017

cdamken commented Apr 24, 2017

tomneedham commented May 23, 2017

cdamken commented May 23, 2017

lock bot commented Aug 3, 2019

mrow4a commented Mar 20, 2017 •

edited

Loading

mrow4a commented Mar 20, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

PVince81 commented Mar 23, 2017 •

edited

Loading