Skip to content

Group shares with same source and target #25113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PVince81 merged 7 commits into from
Jul 19, 2016
Merged

Group shares with same source and target #25113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aa42b7b
Group shares with same source and target
rullzer Jun 15, 2016
e5af146
Add integration tests for merging received shares
Jul 11, 2016
c97bb28
Improved share grouping readability + fixed test
Jul 11, 2016
cb683dd
Add repair step for unmerged shares (WIP)
Jul 12, 2016
fb0f186
Added more tests for sharing's MountProvider
Jul 13, 2016
c4db9ad
Group incoming shares for resharing in JS
Jul 13, 2016
d0244f5
Adjust repair version check for unmerged shares
Jul 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 76 additions & 3 deletions apps/files_sharing/lib/MountProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,20 @@ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID();
});

$mounts = [];
foreach ($shares as $share) {
$superShares = $this->buildSuperShares($shares);

$mounts = [];
foreach ($superShares as $share) {
try {
$mounts[] = new SharedMount(
'\OC\Files\Storage\Shared',
$mounts,
[
'user' => $user->getUID(),
'newShare' => $share,
// parent share
'superShare' => $share[0],
// children/component of the superShare
'groupedShares' => $share[1],
],
$storageFactory
);
Expand All @@ -95,4 +99,73 @@ public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
// array_filter removes the null values from the array
return array_filter($mounts);
}

/**
* Groups shares by path (nodeId) and target path
*
* @param \OCP\Share\IShare[] $shares
* @return \OCP\Share\IShare[][] array of grouped shares, each element in the
* array is a group which itself is an array of shares
*/
private function groupShares(array $shares) {
$tmp = [];

foreach ($shares as $share) {
if (!isset($tmp[$share->getNodeId()])) {
$tmp[$share->getNodeId()] = [];
}
$tmp[$share->getNodeId()][$share->getTarget()][] = $share;
Copy link
Contributor

@PVince81 PVince81 Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rullzer Shouldn't nodeId and target be combined into a single key ? That is, if you're grouping by these too, unless this is doing something slightly different.

Once I understand this I'll update the PHPDoc.

Copy link
Contributor

@PVince81 PVince81 Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it would likely yield the same results anyway.

Whenever a single folder is has two shares, one with group and one with user, as long as the target is the same they need to stay combined. Once the user renamed the target, they need to stay split. That makes sense.

Copy link
Contributor Author

@rullzer rullzer Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is the 'old' behaviour anyways.

I'm not convinced that is always the right approach. Ideally I just see 1 entry per share. Else you also get duplicated data if you sync etc.

But lets make it behave like in the old days first.

}

$result = [];
foreach ($tmp as $tmp2) {
foreach ($tmp2 as $item) {
$result[] = $item;
}
}

return $result;
}

/**
* Build super shares (virtual share) by grouping them by node id and target,
* then for each group compute the super share and return it along with the matching
* grouped shares. The most permissive permissions are used based on the permissions
* of all shares within the group.
*
* @param \OCP\Share\IShare[] $allShares
* @return array Tuple of [superShare, groupedShares]
*/
private function buildSuperShares(array $allShares) {
$result = [];

$groupedShares = $this->groupShares($allShares);

/** @var \OCP\Share\IShare[] $shares */
foreach ($groupedShares as $shares) {
if (count($shares) === 0) {
continue;
}

$superShare = $this->shareManager->newShare();

// compute super share based on first entry of the group
$superShare->setId($shares[0]->getId())
->setShareOwner($shares[0]->getShareOwner())
->setNodeId($shares[0]->getNodeId())
->setTarget($shares[0]->getTarget());

// use most permissive permissions
$permissions = 0;
foreach ($shares as $share) {
$permissions |= $share->getPermissions();
}

$superShare->setPermissions($permissions);

$result[] = [$superShare, $shares];
}

return $result;
}
}
24 changes: 17 additions & 7 deletions apps/files_sharing/lib/SharedMount.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ class SharedMount extends MountPoint implements MoveableMount {
private $user;

/** @var \OCP\Share\IShare */
private $share;
private $superShare;

/** @var \OCP\Share\IShare[] */
private $groupedShares;

/**
* @param string $storage
Expand All @@ -61,10 +64,13 @@ class SharedMount extends MountPoint implements MoveableMount {
public function __construct($storage, array $mountpoints, $arguments = null, $loader = null) {
$this->user = $arguments['user'];
$this->recipientView = new View('/' . $this->user . '/files');
$this->share = $arguments['newShare'];
$newMountPoint = $this->verifyMountPoint($this->share, $mountpoints);

$this->superShare = $arguments['superShare'];
$this->groupedShares = $arguments['groupedShares'];
Copy link
Contributor

@PVince81 PVince81 Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose another enhanced approach would be to define a new class CompositeShare that implements IShare and contains all the children (groupedShares) then when we call setTarget() on the main one it internally sets the target on all sub-shares.

This could have the advantage of being reusable in other contexts where grouping is required.

Copy link
Contributor Author

@rullzer rullzer Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that!
very elegant and abstract nicely away


$newMountPoint = $this->verifyMountPoint($this->superShare, $mountpoints);
$absMountPoint = '/' . $this->user . '/files' . $newMountPoint;
$arguments['ownerView'] = new View('/' . $this->share->getShareOwner() . '/files');
$arguments['ownerView'] = new View('/' . $this->superShare->getShareOwner() . '/files');
parent::__construct($storage, $absMountPoint, $arguments, $loader);
}

Expand Down Expand Up @@ -106,7 +112,11 @@ private function verifyMountPoint(\OCP\Share\IShare $share, array $mountpoints)
*/
private function updateFileTarget($newPath, &$share) {
$share->setTarget($newPath);
\OC::$server->getShareManager()->moveShare($share, $this->user);

foreach ($this->groupedShares as $share) {
Copy link
Contributor

@PVince81 PVince81 Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conflict with the "share" variable from the method argument.

Also I'm confused why we are relying on the internal groupedShares here and ignoring the $share attribute, the function doesn't fulfil its contract

Copy link
Contributor Author

@rullzer rullzer Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fair enough.

Yeah it needs to be cleaned up more. Basically the groupedShares are always there now. The 'regual' share is just a super set of all shares. I did it this way so we have less code paths. So we even if you have just 1 incomming share then you will have a 'super' share with the same stuff. And groupedShares with just 1 entry.

I prefer 1 code path.

$share->setTarget($newPath);
\OC::$server->getShareManager()->moveShare($share, $this->user);
}
}


Expand Down Expand Up @@ -212,7 +222,7 @@ public function removeMount() {
* @return \OCP\Share\IShare
*/
public function getShare() {
return $this->share;
return $this->superShare;
}

/**
Expand All @@ -221,6 +231,6 @@ public function getShare() {
* @return int
*/
public function getStorageRootId() {
return $this->share->getNodeId();
return $this->getShare()->getNodeId();
}
}
77 changes: 34 additions & 43 deletions apps/files_sharing/lib/sharedstorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@
* Convert target path to source path and pass the function call to the correct storage provider
*/
class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage {

private $share; // the shared resource

/** @var \OCP\Share\IShare */
private $newShare;
private $superShare;

/** @var \OCP\Share\IShare[] */
private $groupedShares;

/**
* @var \OC\Files\View
Expand Down Expand Up @@ -76,11 +76,14 @@ class Shared extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage {
public function __construct($arguments) {
$this->ownerView = $arguments['ownerView'];
$this->logger = \OC::$server->getLogger();
$this->newShare = $arguments['newShare'];

$this->superShare = $arguments['superShare'];
$this->groupedShares = $arguments['groupedShares'];

$this->user = $arguments['user'];

Filesystem::initMountPoints($this->newShare->getShareOwner());
$sourcePath = $this->ownerView->getPath($this->newShare->getNodeId());
Filesystem::initMountPoints($this->superShare->getShareOwner());
$sourcePath = $this->ownerView->getPath($this->superShare->getNodeId());
list($storage, $internalPath) = $this->ownerView->resolvePath($sourcePath);

parent::__construct([
Expand All @@ -95,15 +98,22 @@ private function init() {
}
$this->initialized = true;
try {
Filesystem::initMountPoints($this->newShare->getShareOwner());
$sourcePath = $this->ownerView->getPath($this->newShare->getNodeId());
Filesystem::initMountPoints($this->superShare->getShareOwner());
$sourcePath = $this->ownerView->getPath($this->superShare->getNodeId());
list($this->sourceStorage, $sourceInternalPath) = $this->ownerView->resolvePath($sourcePath);
$this->sourceRootInfo = $this->sourceStorage->getCache()->get($sourceInternalPath);
} catch (\Exception $e) {
$this->logger->logException($e);
}
}

/**
* @return string
*/
public function getShareId() {
return $this->superShare->getId();
}

private function isValid() {
$this->init();
return $this->sourceRootInfo && ($this->sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE;
Expand All @@ -118,15 +128,6 @@ public function getId() {
return 'shared::' . $this->getMountPoint();
}

/**
* get file cache of the shared item source
*
* @return int
*/
public function getSourceId() {
return $this->newShare->getNodeId();
}

/**
* Get the permissions granted for a shared file
*
Expand All @@ -137,7 +138,7 @@ public function getPermissions($target = '') {
if (!$this->isValid()) {
return 0;
}
$permissions = $this->newShare->getPermissions();
$permissions = $this->superShare->getPermissions();
// part files and the mount point always have delete permissions
if ($target === '' || pathinfo($target, PATHINFO_EXTENSION) === 'part') {
$permissions |= \OCP\Constants::PERMISSION_DELETE;
Expand Down Expand Up @@ -259,30 +260,18 @@ public function rename($path1, $path2) {
* @return string
*/
public function getMountPoint() {
return $this->newShare->getTarget();
return $this->superShare->getTarget();
}

/**
* @param string $path
*/
public function setMountPoint($path) {
$this->newShare->setTarget($path);
}

/**
* @return int
*/
public function getShareType() {
return $this->newShare->getShareType();
}
$this->superShare->setTarget($path);

/**
* get share ID
*
* @return integer unique share ID
*/
public function getShareId() {
return $this->newShare->getId();
foreach ($this->groupedShares as $share) {
$share->setTarget($path);
}
}

/**
Expand All @@ -291,14 +280,14 @@ public function getShareId() {
* @return string
*/
public function getSharedFrom() {
return $this->newShare->getShareOwner();
return $this->superShare->getShareOwner();
}

/**
* @return \OCP\Share\IShare
*/
public function getShare() {
return $this->newShare;
return $this->superShare;
}

/**
Expand All @@ -307,7 +296,7 @@ public function getShare() {
* @return string
*/
public function getItemType() {
return $this->newShare->getNodeType();
return $this->superShare->getNodeType();
}

public function getCache($path = '', $storage = null) {
Expand Down Expand Up @@ -336,7 +325,7 @@ public function getPropagator($storage = null) {
}

public function getOwner($path) {
return $this->newShare->getShareOwner();
return $this->superShare->getShareOwner();
}

/**
Expand All @@ -345,7 +334,9 @@ public function getOwner($path) {
* @return bool
*/
public function unshareStorage() {
\OC::$server->getShareManager()->deleteFromSelf($this->newShare, $this->user);
foreach ($this->groupedShares as $share) {
\OC::$server->getShareManager()->deleteFromSelf($share, $this->user);
}
return true;
}

Expand All @@ -361,7 +352,7 @@ public function acquireLock($path, $type, ILockingProvider $provider) {
$targetStorage->acquireLock($targetInternalPath, $type, $provider);
// lock the parent folders of the owner when locking the share as recipient
if ($path === '') {
$sourcePath = $this->ownerView->getPath($this->newShare->getNodeId());
$sourcePath = $this->ownerView->getPath($this->superShare->getNodeId());
$this->ownerView->lockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true);
}
}
Expand All @@ -377,7 +368,7 @@ public function releaseLock($path, $type, ILockingProvider $provider) {
$targetStorage->releaseLock($targetInternalPath, $type, $provider);
// unlock the parent folders of the owner when unlocking the share as recipient
if ($path === '') {
$sourcePath = $this->ownerView->getPath($this->newShare->getNodeId());
$sourcePath = $this->ownerView->getPath($this->superShare->getNodeId());
$this->ownerView->unlockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true);
}
}
Expand Down
Loading