Handle non-TUS uploads called directly with upload path #1314
Conversation
|
cool, will test right away! |
pkg/storage/fs/ocis/upload.go
Outdated
| // Upload corresponding to this ID was not found. | ||
| // Assume that this corresponds to the resource path to which the file has to be uploaded. | ||
| buf := &bytes.Buffer{} | ||
| length, err := io.Copy(buf, r) |
There was a problem hiding this comment.
this buffers the whole file in memory before writing it to disk.
There was a problem hiding this comment.
I think it makes more sense to revert the changes and use the lookup of the node before ... I can look into making the chunking work.
There was a problem hiding this comment.
and this reads consumes the body, which makes the subsequent writeChunk not read anything ...
There was a problem hiding this comment.
@butonic the refactoring concerns less the chunking part and more that we can support both TUS and non-TUS uploads out of the box without ugly config changes.
This step was just to get the length of the upload since that is not explicitly provided. One way to get around this is to set SizeIsDeferred to true, since we never really use it for the normal uploads.
There was a problem hiding this comment.
Yes ... and Upload() was implementing the non tus based upload. For tus you need InitiateUpload and UseIn, which you correctly check in the dataprovider ... why did you need to touch the Upload implementation?
There was a problem hiding this comment.
The latest commit gets rid of the buffer issue.
The aim was to have uniform implementation for all types of uploads. Calling InitiateUpload for TUS uploads and not doing so for others didn't seem ideal. Also, since all the file systems did in fact have the UseIn method, the PUT calls were handled by a wrapper around the tus client which was also a hack.
We wanted to add support for multiple protocols as started in #828, so all of these should ideally have the same workflow: Call InitiateUpload -> Send PUT/POST call to the returned URL
|
Ok, this now works properly with ocis and the accounts service. @ishank011 Great job on the refactoring! Let's see what the reva testsuite says. |
|
all green! |
No description provided.