Skip to content
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

[stable9.1] Fix unmerged shares repair targetdecision #25812

Merged
merged 2 commits into from
Aug 17, 2016
Merged

[stable9.1] Fix unmerged shares repair targetdecision #25812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03a65e5
Improve file_target finding logic when repairing unmerged shares
Aug 12, 2016
8cca364
Fix unmerged shares repair with mixed group and direct shares
Aug 12, 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
52 changes: 50 additions & 2 deletions lib/private/Repair/RepairUnmergedShares.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ private function buildPreparedQueries() {
*/
$query = $this->connection->getQueryBuilder();
$query
->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type')
->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type', 'stime')
->from('share')
->where($query->expr()->eq('share_type', $query->createParameter('shareType')))
->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths')))
Expand Down Expand Up @@ -148,6 +148,54 @@ private function getSharesWithUser($shareType, $shareWiths) {
return $groupedShares;
}

/**
* Decide on the best target name based on all group shares and subshares,
* goal is to increase the likeliness that the chosen name matches what
* the user is expecting.
*
* For this, we discard the entries with parenthesis "(2)".
* In case the user also renamed the duplicates to a legitimate name, this logic
* will still pick the most recent one as it's the one the user is most likely to
* remember renaming.
*
* If no suitable subshare is found, use the least recent group share instead.
*
* @param array $groupShares group share entries
* @param array $subShares sub share entries
*
* @return string chosen target name
*/
private function findBestTargetName($groupShares, $subShares) {
$pickedShare = null;
// sort by stime, this also properly sorts the direct user share if any
@usort($subShares, function($a, $b) {
if ($a['stime'] < $b['stime']) {
Copy link
Member

Choose a reason for hiding this comment

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

Worth to consider this solution to sort this: http://stackoverflow.com/a/2852918

return -1;
} else if ($a['stime'] > $b['stime']) {
return 1;
}

return 0;
});

foreach ($subShares as $subShare) {
// skip entries that have parenthesis with numbers
if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we might be matching more entries than we should.

Some files that would match:

  • (1) First chapter
  • A file with () in the name
  • Another file with (12) in the name
  • Some files ()

Maybe a regex like /\([0-9]+\)$/ is better to match duplicated entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the reason I decided against adding a "$" was so we can also match "test (2).txt". Maybe I should spend the time to build a proper regexp then... or trim the extension before checking ?

Copy link
Member

Choose a reason for hiding this comment

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

After checking how the names are generated, /\(\d+\)(\.[^\.]+)?$/ is a better option. It should match:

  • "filename (1).txt"
  • "filename (2)"
  • "filename (1)(2).txt"

but not

  • "filename ().txt"
  • "filename ()"
  • "filename (1)."
  • "filename (1).txt.txt"
  • "filename (1)..txt"

continue;
}
// pick any share found that would match, the last being the most recent
$pickedShare = $subShare;
}

// no suitable subshare found
if ($pickedShare === null) {
// use least recent group share target instead
$pickedShare = $groupShares[0];
}

return $pickedShare['file_target'];
}

/**
* Fix the given received share represented by the set of group shares
* and matching sub shares
Expand All @@ -171,7 +219,7 @@ private function fixThisShare($groupShares, $subShares) {
return false;
}

$targetPath = $groupShares[0]['file_target'];
$targetPath = $this->findBestTargetName($groupShares, $subShares);

// check whether the user opted out completely of all subshares
$optedOut = true;
Expand Down
111 changes: 101 additions & 10 deletions tests/lib/Repair/RepairUnmergedSharesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase {
/** @var \OCP\IDBConnection */
private $connection;

/** @var int */
private $lastShareTime;

protected function setUp() {
parent::setUp();

Expand Down Expand Up @@ -92,6 +95,9 @@ protected function setUp() {
}
}));

// used to generate incremental stimes
$this->lastShareTime = time();

/** @var \OCP\IConfig $config */
$this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager);
}
Expand All @@ -108,6 +114,7 @@ protected function deleteAllShares() {
}

private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) {
$this->lastShareTime += 100;
$qb = $this->connection->getQueryBuilder();
$values = [
'share_type' => $qb->expr()->literal($type),
Expand All @@ -119,7 +126,7 @@ private function createShare($type, $sourceId, $recipient, $targetName, $permiss
'file_source' => $qb->expr()->literal($sourceId),
'file_target' => $qb->expr()->literal($targetName),
'permissions' => $qb->expr()->literal($permissions),
'stime' => $qb->expr()->literal(time()),
'stime' => $qb->expr()->literal($this->lastShareTime),
];
if ($parentId !== null) {
$values['parent'] = $qb->expr()->literal($parentId);
Expand Down Expand Up @@ -204,7 +211,7 @@ public function sharesDataProvider() {
[
// #2 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share
// - one subshare for each group share, both with parenthesis
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
Expand All @@ -218,7 +225,7 @@ public function sharesDataProvider() {
[
['/test', 31],
['/test', 31],
// reset to original name
// reset to original name as the sub-names have parenthesis
['/test', 31],
['/test', 31],
// leave unrelated alone
Expand All @@ -228,6 +235,54 @@ public function sharesDataProvider() {
[
// #3 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share, both renamed manually
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// child of the previous ones
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (1 legit paren)', 31, 0],
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (2 legit paren)', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
],
[
['/test', 31],
['/test', 31],
// reset to less recent subshare name
['/test_renamed (2 legit paren)', 31],
['/test_renamed (2 legit paren)', 31],
// leave unrelated alone
['/test (4)', 31],
]
],
[
// #4 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share, one with parenthesis
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// child of the previous ones
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0],
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
],
[
['/test', 31],
['/test', 31],
// reset to less recent subshare name but without parenthesis
['/test_renamed', 31],
['/test_renamed', 31],
// leave unrelated alone
['/test (4)', 31],
]
],
[
// #5 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share
// - first subshare not renamed (as in real world scenario)
// - but the targets do not match when grouped
Expand All @@ -251,7 +306,7 @@ public function sharesDataProvider() {
]
],
[
// #4 bogus share:
// #6 bogus share:
// - outsider shares with group1, group2
// - one subshare for each group share
// - non-matching targets
Expand All @@ -276,7 +331,7 @@ public function sharesDataProvider() {
]
],
[
// #5 bogus share:
// #7 bogus share:
// - outsider shares with group1, group2
// - one subshare for each group share
// - non-matching targets
Expand All @@ -301,7 +356,7 @@ public function sharesDataProvider() {
]
],
[
// #6 bogus share:
// #8 bogus share:
// - outsider shares with group1, group2 and also user2
// - one subshare for each group share
// - one extra share entry for direct share to user2
Expand Down Expand Up @@ -329,7 +384,7 @@ public function sharesDataProvider() {
]
],
[
// #7 bogus share:
// #9 bogus share:
// - outsider shares with group1 and also user2
// - no subshare at all
// - one extra share entry for direct share to user2
Expand All @@ -350,7 +405,7 @@ public function sharesDataProvider() {
]
],
[
// #8 legitimate share with own group:
// #10 legitimate share with own group:
// - insider shares with both groups the user is already in
// - no subshares in this case
[
Expand All @@ -368,7 +423,7 @@ public function sharesDataProvider() {
]
],
[
// #9 legitimate shares:
// #11 legitimate shares:
// - group share with same group
// - group share with other group
// - user share where recipient renamed
Expand All @@ -392,7 +447,7 @@ public function sharesDataProvider() {
]
],
[
// #10 legitimate share:
// #12 legitimate share:
// - outsider shares with group and user directly with different permissions
// - no subshares
// - same targets
Expand All @@ -410,6 +465,42 @@ public function sharesDataProvider() {
['/test (4)', 31],
]
],
[
// #13 bogus share:
// - outsider shares with group1, user2 and then group2
// - user renamed share as soon as it arrived before the next share (order)
// - one subshare for each group share
// - one extra share entry for direct share to user2
// - non-matching targets
[
// first share with group
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
// recipient renames
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/first', 31, 0],
// then direct share, user renames too
[Constants::SHARE_TYPE_USER, 123, 'user2', '/second', 31],
// another share with the second group
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// use renames it
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/third', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31],
],
[
// group share with group1 left alone
['/test', 31],
// first subshare repaired
['/third', 31],
// direct user share repaired
['/third', 31],
// group share with group2 left alone
['/test', 31],
// second subshare repaired
['/third', 31],
// leave unrelated alone
['/test (5)', 31],
]
],
];
}

Expand Down