[api] Improve normal file upload design for public APIs #3878
Conversation
Harshg999
changed the title
|
|
Harshg999
force-pushed
the
fix-upload
branch
from
1ffe9bf
to
d112725
Compare
|
|
Harshg999
changed the title
Harshg999
requested review from
wing2fly,
amitsrivastava,
tabraiz12,
JohanAhlen,
bjornalm,
ranade1,
athithyaaselvam and
agl29
There was a problem hiding this comment.
Nice work, I have a couple of thoughts without having looked to deep into the python code.
- I think I have raised this before, but why are we not taking the opportunity while building completely new APIs to implement the new Error handling? Both the UI and the API are being rebuilt so I doubt there will be a better opportunity.
https://docs.google.com/document/d/1PLaOEZnDHI0GiCqCzkRpnVnrenPE8LbGm1WlaJTn0cg/edit?usp=sharing
- Since this is a professional file service API, we should consider the difference between Base-2 and Base-10 when dealing with file sizes, e.g. how to calculate and how to communicate sizes in a correct and consistent manner. I don't know how big of an issue this is for our new filebrowser, but I think we should to look into it.
For instance, I notice that FILE_UPLOAD_CHUNK_SIZE is calculated as Base-2 and should be expressed as KiB, MiB, GiB etc but is communicated as base-10.
We should look into which files systems use Base-2 (like macOS Terminal and unix) and which use base-10 (like S3) and have a strategy for dealing with that. My fear is that in the context of big data these differences can have a large impact if we don't get it right.
- We should also add unit tests to this new API
| # Check if the file type is restricted | ||
| _, file_type = os.path.splitext(uploaded_file.name) | ||
| if RESTRICT_FILE_EXTENSIONS.get() and file_type.lower() in [ext.lower() for ext in RESTRICT_FILE_EXTENSIONS.get()]: | ||
| return HttpResponse(f'File type "{file_type}" is not allowed. Please choose a file with a different extetypension.', status=400) |
There was a problem hiding this comment.
nit: spelling extetypension
There was a problem hiding this comment.
two questions
- Is there room for a longer error description, e.g including while formats that are allowed?
- are we exposing the allowed types via some setting or API so that we can prevent those files from being uplaaded in the UI in the first place?
|
|
||
| if request.fs.isdir(dest_path) and posixpath.sep in uploaded_file.name: | ||
| raise Exception(f'Upload failed: {posixpath.sep} is not allowed in the filename {uploaded_file.name}.') # TODO: status code | ||
| if request.fs.exists(filepath): |
There was a problem hiding this comment.
I think ideally there should be an option to override an existing file here if the user has already been informed and made that choice. E.g. in the case of an updated version of the file but at same file path.
| if request.META.get('upload_failed'): | ||
| raise Exception(request.META.get('upload_failed')) # TODO: Check error message and status code | ||
| # Check if the file size exceeds the maximum allowed size | ||
| max_size = MAX_FILE_SIZE_UPLOAD_LIMIT.get() |
There was a problem hiding this comment.
Is this the same for all file systems?
What changes were proposed in this pull request?
settings.pywhen the request reaches the first middleware.How was this patch tested?