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

Backend errors not shown in record file upload screen and failed files cannot remove #2804

Closed
geekdinazor opened this issue Aug 23, 2024 · 6 comments · Fixed by inveniosoftware/invenio-rdm-records#1802
Labels
bug Something isn't working

Comments

@geekdinazor
Copy link

geekdinazor commented Aug 23, 2024

Package version (if known): 12.0.2

Describe the bug

Backend errors not shown in UI while uploading file to record and failed files cannot remove.

Steps to Reproduce

  1. Add RECORDS_RESOURCES_ALLOW_EMPTY_FILES = False flag to invenio.cfg in order to restrict empty files on invenio-rdm instance.
  2. Upload an empty file (0 byte) to the record.
  3. The upload fails and progress bar turns red, but no error popup or message appears in page.
  4. Click trashcan icon to remove failed file but file still shown in file list.

Expected behavior

File upload errors from the backend should be displayed in the UI, and failed upload should be removed when trashcan icon clicked.

Screenshots (if applicable)

backend_error
remove_bug

Additional context

It is essential that all errors originating from the backend are displayed in the UI and all backend errored files must be removable for future use cases.

/cc @hbayindir @hcansu

@geekdinazor
Copy link
Author

geekdinazor commented Oct 10, 2024

Hi @ntarocco, this PR only checks if the file is empty before upload. Backend errors are still not addressed. Can you re-open the issue? We implemented sanity checks that raise a StorageError inherited exception after the file is uploaded if it's corrupted or the file extension doesn't match the content. A similar error can be triggered by raising an UnexpectedFileSizeError after the fp.close() line (https://github.com/inveniosoftware/invenio-files-rest/blob/v2.2.1/invenio_files_rest/storage/pyfs.py#L134).

@ntarocco
Copy link
Contributor

Hi @ntarocco, this PR only checks if the file is empty before upload. Backend errors are still not addressed. Can you re-open the issue? We implemented sanity checks that raise a StorageError inherited exception after the file is uploaded if it's corrupted or the file extension doesn't match the content. A similar error can be triggered by raising an UnexpectedFileSizeError after the fp.close() line (https://github.com/inveniosoftware/invenio-files-rest/blob/v2.2.1/invenio_files_rest/storage/pyfs.py#L134).

This is already checked here.
What is the exact issue happening now?

@geekdinazor
Copy link
Author

The problem is that if any error came from the backend during file upload (not just the empty file error, any backend error), the upload progress bar turns red, but no error message is displayed in the UI. This issue isn't specific to empty files, all backend errors should be visible in the UI.

@Samk13
Copy link
Member

Samk13 commented Oct 13, 2024

Hi @geekdinazor
I submitted the PR that addresses the empty file error, explicitly mentioning that this fix avoids the unnecessary round trip to the back end. Since this specific issue has been handled, we close this issue.

If you are encountering other recurring backend errors, please feel free to open a new issue with clear reproduction steps, and we'll look into it.

Also, note that a rewrite of the upload logic is planned soon to support multi-part file uploads, which will likely affect the current implementation, so this issue of showing all back-end errors on the UI could also be addressed as well.

@hbayindir
Copy link

Hi @Samk13,

I think there was a small misunderstanding in the issue. In our implementation, we make additional checks in the backend, namely file format checks, sizes, a couple of Zip Bomb protections and so on...

What we want is to float these errors to the front end, so the user can know what went wrong during the file upload, and fix the problems and retry uploading.

Is it possible to re-open the issue from the perspective, or if the new file uploader gonna support this as well, we might end up integrating that one as well.

Thanks a ton for the hard work,

Best regards.

@Samk13
Copy link
Member

Samk13 commented Oct 28, 2024

Hi @hbayindir,
We can keep this one closed and open a new issue with clear repro steps.
Of course, you're more than welcome to submit a PR that implements this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants