X Tutup
Skip to content

Remove the uploaded file if there is a checksum error#35294

Merged
micbar merged 2 commits intomasterfrom
checksum_partial_upload
May 31, 2019
Merged

Remove the uploaded file if there is a checksum error#35294
micbar merged 2 commits intomasterfrom
checksum_partial_upload

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented May 21, 2019 •
edited by micbar
Loading

Description

If there is a checksum error while uploading the file, we remove the file, which will also clear the checksum in order to allow to reupload the file again

Related Issue

https://github.com/owncloud/enterprise/issues/3101

Motivation and Context

How Has This Been Tested?

I haven't been able to reproduce the exact issue in master. Checked with aborted uploads (using the checksum header) as well as sending wrong checksum headers: partial file doesn't end in the storage and no information is stored in the DB. This means that the problem might be already solved in master without the need to this patch.
I haven't tested in 10.1.1 nor 10.2.0 yet

Screenshots (if appropriate):

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
  • Documentation ticket raised:

Open tasks:

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

@jvillafanez jvillafanez self-assigned this May 21, 2019
mrow4a
mrow4a approved these changes May 21, 2019
View reviewed changes
Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

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

@jvillafanez I think it is good to have it to prevent issues

@jvillafanez jvillafanez force-pushed the checksum_partial_upload branch from 48a5c69 to e6cd505 Compare May 22, 2019 07:19
@codecov
Copy link

codecov bot commented May 22, 2019

Codecov Report

Merging #35294 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35294      +/-   ##
============================================
- Coverage     65.54%   65.53%   -0.01%     
  Complexity    18647    18647              
============================================
  Files          1218     1218              
  Lines         70546    70548       +2     
  Branches       1288     1288              
============================================
  Hits          46236    46236              
- Misses        23933    23935       +2     
  Partials        377      377
Flag Coverage Δ Complexity Δ
#javascript 53.69% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.9% <0%> (-0.01%) 18647 <0> (ø)
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Connector/Sabre/File.php 82.63% <0%> (-0.54%) 115 <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 74ead9c...e6cd505. Read the comment docs.

@codecov
Copy link

codecov bot commented May 22, 2019 •
edited
Loading

Codecov Report

Merging #35294 into master will increase coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35294      +/-   ##
============================================
+ Coverage     65.53%   65.54%   +<.01%     
  Complexity    18648    18648              
============================================
  Files          1218     1218              
  Lines         70549    70550       +1     
  Branches       1288     1288              
============================================
+ Hits          46236    46240       +4     
+ Misses        23936    23933       -3     
  Partials        377      377
Flag Coverage Δ Complexity Δ
#javascript 53.69% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.9% <80%> (ø) 18648 <3> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Connector/Sabre/File.php 84.19% <80%> (+1.02%) 115 <3> (ø) ⬇️

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 5f787f9...88ae634. Read the comment docs.

@micbar
Copy link
Contributor

micbar commented May 24, 2019

@jvillafanez could you check the unit tests?
Would it be possible to have coverage?

@jvillafanez jvillafanez force-pushed the checksum_partial_upload branch from 49bb43c to 430f613 Compare May 30, 2019 10:01
@jvillafanez jvillafanez force-pushed the checksum_partial_upload branch from 430f613 to 88ae634 Compare May 30, 2019 13:25
@micbar micbar merged commit e6a4b4d into master May 31, 2019
@delete-merged-branch delete-merged-branch bot deleted the checksum_partial_upload branch May 31, 2019 09:22
@micbar
Copy link
Contributor

micbar commented May 31, 2019

@jvillafanez please backport :-)

@phil-davis phil-davis mentioned this pull request Jun 5, 2019
Merged
@phil-davis
Copy link
Contributor

phil-davis commented Jun 5, 2019

Backport stable10 #35429

@dpakach dpakach mentioned this pull request Jun 21, 2019
Open
@davitol davitol mentioned this pull request Sep 3, 2019
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrow4a mrow4a mrow4a approved these changes

Assignees

@jvillafanez jvillafanez

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvillafanez @micbar @phil-davis @mrow4a
X Tutup