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.

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

fix: remove just uploaded files in case of error - also for storages … #40892

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Jul 26, 2023

…which do not support part files

Description

Storage which does not support part files (external storage and s3) can leave broken files in the user space.
This change cleans up these broken files.

Alternative approach to #40891 - which is breaking apis too badly ...

Related Issue

How Has This Been Tested?

One has to force a failure during upload - e.g. adding one byte to the expected size at

$expected = $_SERVER['CONTENT_LENGTH'];

  • have trash enabled
  • have encryption enabled
  • to test with posix storage
  • to test with s3 storage
  • to test with different external storage
  • to test with and without chunking (smaller and greater 10MB
  1. Upload file
  2. see the error message on the browser
  3. assert that the file is not on storage
  4. assert that the file is not in DB

Screenshots (if appropriate):

image

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:
  • Changelog item, see TEMPLATE

@DeepDiver1975 DeepDiver1975 force-pushed the fix/remove-failed-upload-file-2 branch 2 times, most recently from 8390339 to a080a2c Compare July 26, 2023 10:37
@DeepDiver1975 DeepDiver1975 force-pushed the fix/remove-failed-upload-file-2 branch from a080a2c to 6369bb9 Compare July 26, 2023 11:37
@phil-davis phil-davis requested a review from pako81 July 26, 2023 12:29
@jnweiger
Copy link
Contributor

Confirmed. This fixes the broken files after timeout.

@pako81
Copy link

pako81 commented Jul 26, 2023

merge it?

@phil-davis
Copy link
Contributor

merge it?

AFAIK it is fine to merge. Who decides to press the merge button these days?

@DeepDiver1975
Copy link
Member Author

I would like to add some unit tests for this ..... but I can do this in a follow up pr as well ....

@phil-davis
Copy link
Contributor

@DeepDiver1975 also needs a changelog entry.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/remove-failed-upload-file-2 branch 4 times, most recently from 7f99193 to 3ced26a Compare August 2, 2023 08:20
@DeepDiver1975 DeepDiver1975 force-pushed the fix/remove-failed-upload-file-2 branch from 3ced26a to b8e8e58 Compare August 3, 2023 10:22
@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

84.6% 84.6% Coverage
0.0% 0.0% Duplication

@DeepDiver1975
Copy link
Member Author

now squashed and with changelog

@phil-davis
Copy link
Contributor

@pako81 this has been reviewed by multiple people and approved.
I guess we should merge it.

@pako81 pako81 merged commit e4bf50d into master Aug 8, 2023
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the fix/remove-failed-upload-file-2 branch August 8, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants