X Tutup
Skip to content

Command to troubleshoot invalid shares generated by transfer ownership#37860

Merged
micbar merged 1 commit intomasterfrom
feature/repair-transfered-shares
Sep 21, 2020
Merged

Command to troubleshoot invalid shares generated by transfer ownership#37860
micbar merged 1 commit intomasterfrom
feature/repair-transfered-shares

Conversation

@mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Aug 30, 2020 •
edited
Loading

Adds new commands that detect and fix invalid shares generated by transfer ownership.

  • add logging of invalid shares
  • add option to fix invalid shares
  • decide whether we want to release this as command (as is), or as update repair steps, or just as patch for specific customers -> and add unit tests if first 2 options (@micbar @pmaier1 )
  • add docs - see issue files:troubleshoot-transfer-ownership docs#2794
  • add unit and integration tests

1. Invalid initiator

This has been triggered by past bug, that has been fixed in #37791

$ occ files:troubleshoot-transfer-ownership invalid-initiator

SEARCHING FOR RESHARES THAT HAVE INVALID UID_INITIATOR(RESHARER) - RESHARER WHICH DOES NOT HAVE HIS SHARE MOUNTED ANY MORE AND THUS SHARE IS CORRUPTED
Invalid reshare initiator found for user test4 in shares: 
id=2,file_source=8,uid_owner=test1,uid_initiator=test4,share_with=test3
FOUND 1 INVALID INITIATOR RESHARES

2. Invalid owner_uid (detached shares from their file storages, these would not show in UI to the user unless adjusted)

This could have been created for three reasons I can think of:

  • mismatching initiator (above case 1) somehow lead to this
  • fact that Share\IManager->transferShares is not wrapped with transaction, and thus any termination in the middle could have resulted in mismatch between share. uid_owner and storages.id
  • past bug that is yet not identified or has been already fixed
$ occ files:troubleshoot-transfer-ownership invalid-owner

SEARCHING FOR SHARES THAT HAVE INVALID UID_OWNER - UID_OWNER VALUES THAT HAVE NOT BEEN UPDATED TO MATCH REQUIRED TRANSFERRED STORAGE
Invalid shares:
id=1,file_source=8,storage=home::test1,uid_owner=test4,uid_initiator=test4,share_with=test2
FOUND 1 INVALID SHARE OWNERS

Related: https://github.com/owncloud/enterprise/issues/4121

@mrow4a mrow4a self-assigned this Aug 30, 2020
@update-docs
Copy link

update-docs bot commented Aug 30, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch from bb1dfdf to e8034c2 Compare August 30, 2020 18:00
@pmaier1
Copy link
Contributor

pmaier1 commented Aug 31, 2020

@micbar Please check and let's discuss then.

@mmattel
Copy link
Contributor

mmattel commented Aug 31, 2020 •
edited
Loading

Please use a table (default) and json (pretty) print (optional) output format for better readbility of the result, like we have starting with in eg

core/apps/files_external/lib/Command/ListCommand.php

Line 300 in c556e9d

if ($outputIsJson) {

If this progresses, you MUST file a docs issue (and a docs PR)

@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch from e8034c2 to 8e392d3 Compare September 13, 2020 14:52
@mrow4a mrow4a marked this pull request as ready for review September 13, 2020 14:55
@mrow4a mrow4a requested a review from karakayasemi September 13, 2020 14:55
@mrow4a
Copy link
Contributor Author

mrow4a commented Sep 13, 2020 •
edited
Loading

@karakayasemi @pmaier1 @pako81 this new command took me significant amount of time, and due to very low usage I suggest to not invest much time into adding unit tests here (it is used only by customer impacted by past bug or heavily using transfer ownership). We could however release it and add documentation (or add tests in the future if this command stops working).

@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch 4 times, most recently from 870124c to 0efb20c Compare September 14, 2020 08:10
@micbar
Copy link
Contributor

micbar commented Sep 14, 2020

@mrow4a Feel free to add basic integration tests.

@codecov
Copy link

codecov bot commented Sep 14, 2020 •
edited
Loading

Codecov Report

Merging #37860 into master will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37860      +/-   ##
============================================
- Coverage     64.75%   64.71%   -0.04%     
- Complexity    19408    19442      +34     
============================================
  Files          1285     1286       +1     
  Lines         75830    76020     +190     
  Branches       1336     1336              
============================================
+ Hits          49101    49196      +95     
- Misses        26335    26430      +95     
  Partials        394      394
Flag Coverage Δ Complexity Δ
#javascript 54.06% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.88% <50.00%> (-0.05%) 19442.00 <34.00> (+34.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 84.18% <50.00%> (ø) 49.00 <0.00> (ø)
...iles/lib/Command/TroubleshootTransferOwnership.php 50.00% <50.00%> (ø) 34.00 <34.00> (?)

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 3137c9b...0405d40. Read the comment docs.

@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch from 0efb20c to 5030c5d Compare September 20, 2020 10:08
karakayasemi
karakayasemi approved these changes Sep 20, 2020
View reviewed changes
Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

Code looks okay and harmless. I am aware of the problems for adding tests from the last time. It is up to you to whether add some simple tests.

@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch from 5030c5d to 85792f8 Compare September 20, 2020 12:33
@mrow4a
Copy link
Contributor Author

mrow4a commented Sep 20, 2020

@karakayasemi it was agreed to add tests and doc to this command , and so did

@mrow4a mrow4a requested a review from karakayasemi September 20, 2020 12:36
@micbar
Copy link
Contributor

micbar commented Sep 20, 2020

awesome! THX!

Will approve after CI is green.

@mrow4a
Command to troubleshoot invalid shares created by transfer ownership
0405d40 
add integration tests

add troubleshoot transfer ownership unit
@mrow4a mrow4a force-pushed the feature/repair-transfered-shares branch from 85792f8 to 0405d40 Compare September 20, 2020 17:16
@mmattel
Copy link
Contributor

mmattel commented Sep 20, 2020

Please add a docs issue (and PR if possible) because this is a new occ command that needs to be documented.

@mrow4a
Copy link
Contributor Author

mrow4a commented Sep 20, 2020 •
edited
Loading

@mmattei will create a PR with doc

phil-davis
phil-davis reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Tests look good - 🥇

@phil-davis phil-davis mentioned this pull request Sep 21, 2020
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Sep 21, 2020

I raised docs issue owncloud/docs#2794

@phil-davis
Copy link
Contributor

phil-davis commented Sep 21, 2020

@micbar please force-merge if you are OK with the codecov differences.

@micbar micbar merged commit 43ad56d into master Sep 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/repair-transfered-shares branch September 21, 2020 14:19
@phil-davis phil-davis mentioned this pull request Sep 23, 2020
Merged
11 tasks
@mrow4a mrow4a mentioned this pull request Sep 30, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis left review comments

+1 more reviewer

@karakayasemi karakayasemi karakayasemi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mrow4a mrow4a

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mrow4a @pmaier1 @mmattel @micbar @phil-davis @karakayasemi
X Tutup