Skip to content

Improve error handling for incoming shares poll command#34992

Merged
VicDeo merged 1 commit into
masterfrom
implement-34981
Apr 9, 2019
Merged

Improve error handling for incoming shares poll command#34992
VicDeo merged 1 commit into
masterfrom
implement-34981

Conversation

@VicDeo

@VicDeo VicDeo commented Apr 8, 2019

Copy link
Copy Markdown
Contributor

Description

Output more details when the share is skipped due to exception

Related Issue

How Has This Been Tested?

  1. Created a federated share,
  2. Accepted it
  3. Changed token on the owner side to be invalid.
  4. executed occ incoming-shares:poll on the recipient side

Screenshots (if appropriate):

> php occ incoming-shares:poll
Skipping external share with id "1225" from remote "http://localhost/~deo/stable10". Reason: "Sabre\HTTP\ClientHttpException: Unauthorized"

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@VicDeo VicDeo added this to the development milestone Apr 8, 2019
@VicDeo VicDeo self-assigned this Apr 8, 2019
@VicDeo VicDeo force-pushed the implement-34981 branch from 5b6d050 to cfe8c6e Compare April 8, 2019 22:55
@codecov

codecov Bot commented Apr 8, 2019

Copy link
Copy Markdown

Codecov Report

Merging #34992 into master will decrease coverage by 17.07%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #34992       +/-   ##
=============================================
- Coverage     65.37%   48.29%   -17.08%     
=============================================
  Files          1213      109     -1104     
  Lines         70380    10580    -59800     
  Branches       1295     1295               
=============================================
- Hits          46009     5110    -40899     
+ Misses        23997     5096    -18901     
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 38.17% <ø> (-28.65%) 0 <ø> (-18588)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1095 more

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 a6aac17...cfe8c6e. Read the comment docs.

@codecov

codecov Bot commented Apr 8, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #34992 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34992      +/-   ##
============================================
+ Coverage     65.37%   65.39%   +0.01%     
- Complexity    18588    18589       +1     
============================================
  Files          1213     1213              
  Lines         70380    70400      +20     
  Branches       1295     1295              
============================================
+ Hits          46009    46035      +26     
+ Misses        23997    23991       -6     
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.84% <100%> (+0.02%) 18589 <1> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...atedfilesharing/lib/Command/PollIncomingShares.php 98.11% <100%> (+22.35%) 11 <1> (+1) ⬆️
apps/files_trashbin/lib/Expiration.php 96.55% <0%> (-1.73%) 29% <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 a6aac17...cfe8c6e. Read the comment docs.

@VicDeo VicDeo requested a review from PVince81 April 9, 2019 07:34
@PVince81

PVince81 commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

is the share automatically deleted through polling when it becomes invalid ? what if you run the command again, does the skipped entry still appear or not ?

PVince81
PVince81 reviewed Apr 9, 2019
View reviewed changes
Comment thread apps/federatedfilesharing/lib/Command/PollIncomingShares.php
$qb->expr()->literal($relativeMountPoint)
));
$result = $qb->execute();
$externalShare = $result->fetch();

@PVince81 PVince81 Apr 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

didn't we have fetchOne() ?

@VicDeo VicDeo Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we do, in lib/private/legacy/db/statementwrapper.php

@VicDeo VicDeo Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But as you see this is a legacy class and Doctrine native method fetch does exactly the same. (fetch fetches the next row while fetchAll fetches them all)

PVince81
PVince81 approved these changes Apr 9, 2019
View reviewed changes

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@PVince81

PVince81 commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

@VicDeo feel free to merge and backport after clarifying

@VicDeo

VicDeo commented Apr 9, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@PVince81 >is the share automatically deleted through polling when it becomes invalid ? what if you run the command again, does the skipped entry still appear or not ?

I tried polling only for external shares with invalid token. In this case skipped entries still appear on the next poll.

@VicDeo VicDeo mentioned this pull request Apr 9, 2019
Merged
9 tasks
@VicDeo VicDeo merged commit a26c309 into master Apr 9, 2019
@delete-merged-branch delete-merged-branch Bot deleted the implement-34981 branch April 9, 2019 11:01
@VicDeo

VicDeo commented Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

Stable10: #34993

@lock lock Bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@PVince81 PVince81 PVince81 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@VicDeo VicDeo

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

Improve error handling for incoming shares poll command

3 participants

@VicDeo @PVince81 @phil-davis