Adding repair step for sub share #30186

sharidas · 2018-01-18T14:39:17Z

Adding repair step for sub share.
When share table due to some magic gets
duplicate entries for sub share ( except the id or primary
key ), then this repair step comes for rescue.

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

Description

Remove the duplicate entries from the share table which can cause exception described in the issue.

Related Issue

#27990

Motivation and Context

For some magical reason, if the share table gets populated with duplicate entries then we would have to remove the duplicate entry. Keeping the row with minimum id. Else exception would be thrown as mentioned in the issue.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

sharidas · 2018-01-18T14:40:49Z

Need to write unit test. But so far with the scenario mentioned in the issue, when I run the maintenance:repair command the output is as follows:

sujith@sujith-Inspiron-5567 ~/test/owncloud2 $ ./occ maintenance:repair 
Cannot load Xdebug - it was already loaded
ownCloud is in maintenance mode - no app have been loaded

 - Repair mime types
 - Detect file cache entries with path that does not match parent-child relationships
 - Generate ETags for file where no ETag is present.
     - ETags have been fixed for 0 files/folders.
 - Clean tags and favorites
     - 0 tags of deleted users have been removed.
     - 0 tags for delete files have been removed.
     - 0 tag entries for deleted tags have been removed.
     - 0 tags with no entries have been removed.
 - Drop old database tables
 28/28 [============================] 100%
 - Drop old background jobs
 - Remove getetag entries in properties table
     - Removed 0 unneeded "{DAV:}getetag" entries from properties table.
 - Repair outdated OCS IDs
 - Repair invalid shares
 - Remove old share propagation app entries
 - Move user avatars outside the homes to the new location
 - Remove shares of a users root folder
 - Repair unmerged shares
 - Disable extra themes
 - Repair sub shares
     - Removed 1 shares where duplicate rows where found
sujith@sujith-Inspiron-5567 ~/test/owncloud2 $ ./occ maintenance:repair 
Cannot load Xdebug - it was already loaded
ownCloud is in maintenance mode - no app have been loaded

 - Repair mime types
 - Detect file cache entries with path that does not match parent-child relationships
 - Generate ETags for file where no ETag is present.
     - ETags have been fixed for 0 files/folders.
 - Clean tags and favorites
     - 0 tags of deleted users have been removed.
     - 0 tags for delete files have been removed.
     - 0 tag entries for deleted tags have been removed.
     - 0 tags with no entries have been removed.
 - Drop old database tables
 28/28 [============================] 100%
 - Drop old background jobs
 - Remove getetag entries in properties table
     - Removed 0 unneeded "{DAV:}getetag" entries from properties table.
 - Repair outdated OCS IDs
 - Repair invalid shares
 - Remove old share propagation app entries
 - Move user avatars outside the homes to the new location
 - Remove shares of a users root folder
 - Repair unmerged shares
 - Disable extra themes
 - Repair sub shares
sujith@sujith-Inspiron-5567 ~/test/owncloud2 $

codecov · 2018-01-18T18:00:02Z

Codecov Report

Merging #30186 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #30186      +/-   ##
============================================
+ Coverage     58.36%   58.36%   +<.01%     
- Complexity    18566    18573       +7     
============================================
  Files          1093     1094       +1     
  Lines         63771    63805      +34     
============================================
+ Hits          37217    37242      +25     
- Misses        26554    26563       +9

Impacted Files	Coverage Δ	Complexity Δ
drone/src/lib/private/Group/Database.php	`98.36% <0%> (-1.64%)`	`24% <0%> (ø)`
drone/src/lib/private/Repair.php	`27.16% <0%> (-0.69%)`	`21% <0%> (ø)`
drone/src/lib/private/Repair/RepairSubShares.php	`81.25% <0%> (ø)`	`7% <0%> (?)`
drone/src/lib/private/Server.php	`82.69% <0%> (+0.14%)`	`129% <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 e1c4425...2f09bd8. Read the comment docs.

sharidas · 2018-01-19T12:21:42Z

Created the tests for this repair PR.

PVince81 · 2018-01-19T14:14:04Z

lib/private/Repair/RepairSubShares.php

+
+	public function run(IOutput $output) {
+		$deletedEntries = 0;
+		$this->removeRowsWithDuplicateDataExceptId();


this function's name is lying. It is not removing rows with duplicate data except id. It's only setting some attributes

@PVince81 Yes its making the query statement with attributes. Will get it corrected. It was there in my mind sometime back and then got slipped :( Thanks.

PVince81

Looks fine, see comment

PVince81 · 2018-01-22T14:45:19Z

tests/lib/Repair/RepairSubSharesTest.php

+					'share_with' => $qb->expr()->literal($userName.$groupCount),
+					'uid_owner' => $qb->expr()->literal('admin'),
+					'uid_initiator' => $qb->expr()->literal('admin'),
+					'parent' => $qb->expr()->literal(1),


I suggest you also vary the parent value considering that we use it on the group by

PVince81 · 2018-01-22T14:46:22Z

tests/lib/Repair/RepairSubSharesTest.php

+				])
+				->execute();
+
+			if (($i%3) === 0) {


a bit more fresh air appreciated

Adding repair step for sub share. When share table due to some magic gets duplicate entries ( except the id or primary key ), then this repair step comes for rescue. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2018-02-07T12:17:55Z

any update ?

PVince81

👍

@tom @butonic any comments on the query from https://github.com/owncloud/core/pull/30186/files#diff-c7d3960633d9f6fe9eb517281d947301R61 regarding performance ?

PVince81 · 2018-02-23T14:18:42Z

@sharidas please backport

sharidas · 2018-03-05T10:16:37Z

Backport PR: #30695

phil-davis · 2018-03-21T02:59:58Z

Backport PR has been done and merged.

lock · 2019-07-31T18:02:42Z

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.

sharidas added the 2 - Developing label Jan 18, 2018

sharidas added this to the development milestone Jan 18, 2018

sharidas self-assigned this Jan 18, 2018

sharidas force-pushed the repair-step-subshare branch from a2eb7a9 to b6f6ca9 Compare January 19, 2018 12:18

PVince81 reviewed Jan 19, 2018

View reviewed changes

sharidas force-pushed the repair-step-subshare branch 3 times, most recently from a689afc to 8d67c73 Compare January 21, 2018 06:30

sharidas changed the title ~~[WIP] Adding repair step for sub share~~ [Adding repair step for sub share Jan 22, 2018

sharidas changed the title ~~[Adding repair step for sub share~~ Adding repair step for sub share Jan 22, 2018

PVince81 reviewed Jan 22, 2018

View reviewed changes

sharidas force-pushed the repair-step-subshare branch from 8d67c73 to f3ff72a Compare January 23, 2018 07:02

Adding repair step for sub share

2f09bd8

Adding repair step for sub share. When share table due to some magic gets duplicate entries ( except the id or primary key ), then this repair step comes for rescue. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the repair-step-subshare branch from f3ff72a to 2f09bd8 Compare January 23, 2018 07:40

ownclouders added the status/STALE label Jan 31, 2018

ownclouders removed the status/STALE label Feb 7, 2018

sharidas added 3 - To Review and removed 2 - Developing labels Feb 13, 2018

PVince81 approved these changes Feb 13, 2018

View reviewed changes

ownclouders added the status/STALE label Feb 21, 2018

sharidas removed the status/STALE label Feb 22, 2018

ownclouders added the status/STALE label Feb 22, 2018

PVince81 merged commit 48ef206 into master Feb 23, 2018

ownclouders removed the status/STALE label Feb 23, 2018

PVince81 deleted the repair-step-subshare branch February 23, 2018 14:18

PVince81 added the backport-request label Feb 23, 2018

sharidas mentioned this pull request Mar 5, 2018

[stable10] Adding repair step for sub share #30695

Merged

9 tasks

phil-davis removed the backport-request label Mar 21, 2018

PVince81 mentioned this pull request Apr 3, 2018

Email autocomplete for link shares #30987

Merged

9 tasks

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding repair step for sub share #30186

Adding repair step for sub share #30186

sharidas commented Jan 18, 2018

sharidas commented Jan 18, 2018

codecov bot commented Jan 18, 2018 •

edited

Loading

sharidas commented Jan 19, 2018

PVince81 Jan 19, 2018

sharidas Jan 19, 2018

PVince81 left a comment

PVince81 Jan 22, 2018

PVince81 Jan 22, 2018

PVince81 commented Feb 7, 2018

PVince81 left a comment

PVince81 commented Feb 23, 2018

sharidas commented Mar 5, 2018

phil-davis commented Mar 21, 2018

lock bot commented Jul 31, 2019

Adding repair step for sub share #30186

Adding repair step for sub share #30186

Conversation

sharidas commented Jan 18, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

sharidas commented Jan 18, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

sharidas commented Jan 19, 2018

PVince81 Jan 19, 2018

Choose a reason for hiding this comment

sharidas Jan 19, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Jan 22, 2018

Choose a reason for hiding this comment

PVince81 Jan 22, 2018

Choose a reason for hiding this comment

PVince81 commented Feb 7, 2018

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Feb 23, 2018

sharidas commented Mar 5, 2018

phil-davis commented Mar 21, 2018

lock bot commented Jul 31, 2019

codecov bot commented Jan 18, 2018 •

edited

Loading