Send upload length when resuming upload with deferred length #744
Conversation
…the server. And allow resumeUpload if Upload-Defer-Length is set from server.
Acconut
left a comment
Acconut
left a comment
There was a problem hiding this comment.
Thank you for this PR! Overall, taking the information about deferred length from the HEAD response is a good approach to solving this problem. Looking at the implementation, there are a few minor things to improve. Tests are also still missing, which we need to add before this can be merged. Let me know if you need help with this.
|
Thanks for the review, I've updated them based on your comments. I'll add some tests soon |
lib/upload.js
Outdated
| // upload creation. So, if the file reader is done reading, we know the total | ||
| // upload size and can tell the tus server. | ||
| if (this.options.uploadLengthDeferred && done) { | ||
| if (this._uploadLengthDeferred && (!this.options.uploadLengthDeferred || done)) { |
There was a problem hiding this comment.
| if (this._uploadLengthDeferred && (!this.options.uploadLengthDeferred || done)) { | |
| if (this._uploadLengthDeferred || done) { |
I think we can just use this._uploadLengthDeferred here and don't need to consult the original option anymore.
There was a problem hiding this comment.
That will change the behaviour quite a bit. As it is suggested right now, there's some problems:
- We don't always know the size when
this._uploadLengthDeferredis true sinceoptions.uploadLengthDeferredcould be true. In this case, we'd send wrong info. donewill make it always pass the header at the end, which I believe isn't how it should work. It should only send the header on the next known request from what I understand.
If we change it to an AND, it'll kinda work, but only send the length header at the end.
The logic in the commit right now expands to:
(this._uploadLengthDeferred && !this.options.uploadLengthDeferred)
Send header on next opportunity if the request is of known size(this._uploadLengthDeferred && done)
And send at the end if it's a stream
There was a problem hiding this comment.
You're right, my suggestion is wrong. I used || instead of && on accident and my suggestion would be:
| if (this._uploadLengthDeferred && (!this.options.uploadLengthDeferred || done)) { | |
| if (this._uploadLengthDeferred && done) { |
If we change it to an AND, it'll kinda work, but only send the length header at the end.
Yes, that's right and that's how tus-js-client has handled deferred lengths in the past. It always sent the length header once the file source is closed because then we know the file size for sure. I don't see a problem with keeping this behavior. Or do you?
There was a problem hiding this comment.
Honestly, I'm quite new to the protocol and library so I think you'd know more. But I was looking at this line over at the protocol documentation:
That led me to believe that it should be the first possible instance. Although, I guess both ways should work anyway?
There was a problem hiding this comment.
Hey @Acconut, just want to bump this PR, any thoughts on this? Thanks 😄
|
@Acconut I've added 2 tests. And also updated a fix for calculating the total size on that code block. Before the fix, it'd calculate the size of the current request and send that rather than the total size |
Acconut
left a comment
Acconut
left a comment
There was a problem hiding this comment.
Thank you again for your efforts put into this fix! I'm sorry for letting it fall off my radar! I've merged the latest main branch, addressed my review comment, and improved the tests. So this is good to go now!
Acconut
changed the title
2 participants
This PR fixes issue #182 where user cannot upload against a deferred resource.
The 2 main changes are:
Upload-Lengthheader check ifUpload-Defer-Lengthexists in the HEAD responseUpload-Lengthheader in the next request ifoptions.uploadLengthDeferredis false - because we would know the length straight awayThe code is mostly taken from the relevant issue, so credits goes there. But I've also modified it slightly to simplify the logic.
Closes #182