Make file creation and validation happen prior to blob upload #140
Conversation
|
Fixes #124. |
dkc/core/rest/file.py
Outdated
| return HttpResponseRedirect(file.blob.url) | ||
| if file.blob: # FileFields are falsy when not populated with a file | ||
| return HttpResponseRedirect(file.blob.url) | ||
| return FileResponse(BytesIO(), filename=file.name, content_type=file.content_type) |
There was a problem hiding this comment.
If the file hasn't been uploaded yet, I think this should return a 202, 204, or 404 response. To me, a partially uploaded file shouldn't be considered a fully valid resource by end API users.
There was a problem hiding this comment.
Good point, I'm not sure why I didn't think of something like 204. I wanted to specifically avoid 4xx, thinking of someone trying to sync a tree with a partially uploaded file, and I didn't want to break the sync operation if a pending file exists.
There was a problem hiding this comment.
I don't think I've fully thought this through, but don't we want file upload to appear to be atomic from the perspective of read-only API endpoints? That is, the file will be non-existent / invisible until the blob (and potentially the hashsum?) is ready.
There was a problem hiding this comment.
Perhaps so, but that's a lot more complicated to engineer. If you have a design in mind, please detail it.
There was a problem hiding this comment.
As a start, we could filter the FileViewSet queryset for listing / details to always exclude anything without a blob; that would cause partial files to effectively be invisible in a folder listing. However the folder itself would already have its size incremented, so an attentive observer would notice that certain folder sizes were listed as larger as the sum of their contents.
From a user experience perspective, I'm not sure which is preferable.
I agree that to have fully robust atomicity, we'd have to maintain a separate set of quota reservations or have more complicated logic for reporting effective folder sizes.
zachmullen
force-pushed
the
file-pre-creation
branch
from
4e64e80 to
a438b43
Compare
|
@brianhelba this is ready for final review. |
This is the first of two PRs associated with #124
This provides the upload flow changes so that all uploads are pre-approved. It does not provide any of the security improvements we'd like. I may wait for kitware-resonant/django-s3-file-field#204 to land before tackling the policy enforcement side of this, as it could simplify the implementation here. #139 represents the work needed on the second PR.