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

Support multipart range requests #1420

Merged
merged 1 commit into from Jan 27, 2020
Merged

Conversation

fatkodima
Copy link
Contributor

Closes #559

else
# Partial content, multiple ranges:
status = 206
headers[CONTENT_TYPE] = "multipart/byteranges; boundary=#{MULTIPART_BOUNDARY}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I only have a short time to review this, but I'm excited to see this PR.

Is AaB03x sufficient as a boundary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, why not? The same boundary is used for multipart forms

MULTIPART_BOUNDARY = "AaB03x"

end

def multipart_heading(range)
<<-EOF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we encode this as a string rather than a heredoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to stick here with existing code styles, like

def content_for_tempfile(io, file, name)
<<-EOF
--#{MULTIPART_BOUNDARY}\r
Content-Disposition: form-data; name="#{name}"; filename="#{Utils.escape(file.original_filename)}"\r
Content-Type: #{file.content_type}\r
Content-Length: #{::File.stat(file.path).size}\r
\r
#{io.read}\r
EOF
end
def content_for_other(file, name)
<<-EOF
--#{MULTIPART_BOUNDARY}\r
Content-Disposition: form-data; name="#{name}"\r
\r
#{file}\r
EOF
end

and like examples in many test cases.
So should I change this to string?

@jeremyevans
Copy link
Contributor

@fatkodima I think the implementation looks pretty good. I think you can keep using the existing static boundary. I also think you can keep using the heredoc, you don't need to change it to a string. Can you please rebase this against master? Please make sure that it works for multiple ranges the same as for single ranges (which now use BaseIterator, see 3936b90).

@fatkodima
Copy link
Contributor Author

@jeremyevans Updated.

@jeremyevans
Copy link
Contributor

Looks good. I tested locally with curl -r and got the expected results. I plan on merging in a few days unless other committer objects.

@jeremyevans jeremyevans merged commit 756708f into rack:master Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants