-
Notifications
You must be signed in to change notification settings - Fork 586
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 for range request queries #526
Conversation
lib/plug/static.ex
Outdated
encoding = file_encoding(conn, path, false, false) | ||
serve_range(encoding, range, segments, options) | ||
end | ||
defp serve_range(_conn, _path, _segments, range, _gzip?, _brotli?, _options) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function takes too many parameters (arity is 7, max is 5).
lib/plug/static.ex
Outdated
encoding = file_encoding(conn, path, gzip?, brotli?) | ||
serve_static(encoding, segments, options) | ||
end | ||
defp serve_range(conn, path, segments, [range], _gzip?, _brotli?, options) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function takes too many parameters (arity is 7, max is 5).
lib/plug/static.ex
Outdated
@@ -177,6 +181,100 @@ defmodule Plug.Static do | |||
h in only or match?({0, _}, prefix != [] and :binary.match(h, prefix)) | |||
end | |||
|
|||
defp serve_range(conn, path, segments, [], gzip?, brotli?, options) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function takes too many parameters (arity is 7, max is 5).
Not sure yet how to adjust send_range/7.
@josevalim et al: I believe this is ready for review and merge. |
lib/plug/static.ex
Outdated
raise InvalidRangeError | ||
end | ||
|
||
@byte_range_pattern ~r/^\s*bytes=([0-9]+)?-([0-9]+)?\s*$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please parse the bytes accordingly instead of relying on regexes. See the functions in Plug.Conn.Utils that can help you parse params. If necessary, add new functions there.
lib/plug/static.ex
Outdated
|
||
defp start_and_end([_, range_start], file_size) | ||
when is_binary(range_start) | ||
do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the code style consistent with the remaining of the Plug codebase. In this particular case, everything can fit a single line.
lib/plug/static.ex
Outdated
defp send_range(conn, path, range_start, range_end, file_size, segments, options) do | ||
%{headers: headers} = options | ||
length = (range_end - range_start) + 1 | ||
content_type = segments |> List.last |> MIME.from_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to support custom content types, as here: https://github.com/scouten/plug/blob/a20a9a45aa04e86291ef6e67cd6b5390ccfba8ce/lib/plug/static.ex#L288-L289
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to add a test for this too.
test/plug/static_test.exs
Outdated
test "returns 416 if range is contains non-integers" do | ||
exception = assert_raise Plug.Static.InvalidRangeError, | ||
"invalid range for static asset", | ||
fn -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please start fn ->
in the previous line and outdent the whole function 2 spaces. The same for the examples below.
Sorry for the delay in reviewing this. I have added some comments. |
lib/plug/static.ex
Outdated
@byte_range_pattern ~r/^\s*bytes=([0-9]+)?-([0-9]+)?\s*$/ | ||
|
||
defp serve_range({:ok, conn, file_info, path}, range, segments, options) do | ||
file_size = elem(file_info, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use record matching: file_info(size: size) = file_info
lib/plug/static.ex
Outdated
:entire_file -> | ||
serve_static({:ok, conn, file_info, path}, segments, options) | ||
:invalid -> | ||
raise InvalidRangeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is raising the reasonable thing to do here? You said we don't support all syntaxes, so maybe we should rather send the whole file instead of crashing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find this exact scenario in the spec but it seems like, generally, if there's a range request the server does not understand the header should be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, reading the spec, I can find support for both approaches:
A server that supports range requests MAY ignore or reject a Range header field that consists of more than two overlapping ranges, or a set of many small ranges that are not listed in ascending order, since both are indications of either a broken client or a deliberate denial-of-service attack (Section 6.1).
or (later in same section):
If all of the preconditions are true, the server supports the Range header field for the target resource, and the specified range(s) are invalid or unsatisfiable, the server SHOULD send a 416 (Range Not Satisfiable) response.
The 416 (Range Not Satisfiable) status code indicates that none of the ranges in the request's Range header field (Section 3.1) overlap the current extent of the selected resource or that the set of ranges requested has been rejected due to invalid ranges or an excessive request of small or overlapping ranges.
For byte ranges, failing to overlap the current extent means that the first-byte-pos of all of the byte-range-spec values were greater than the current length of the selected representation. When this status code is generated in response to a byte-range request, the sender SHOULD generate a Content-Range header field specifying the current length of the selected representation (Section 4.2).
I think the argument being made here is that returning the entire file may be "friendlier" than returning a 416, and I could probably go along with that. I'll put it in a separate commit within this PR so we can revert it if you disagree.
lib/plug/static.ex
Outdated
serve_range(encoding, range, segments, options) | ||
end | ||
defp serve_range(_conn, _path, _segments, _range, _options) do | ||
raise InvalidRangeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is raising the reasonable thing to do here? You said we don't support all syntaxes, so maybe we should rather send the whole file instead of crashing.
@josevalim @ericmj this feedback all seems reasonable. I'll work on this over the next couple of weeks and send you a ping when I think it's ready for re-review. |
Remaining issues: |
@josevalim @ericmj Ping. I think this is ready for re-review. |
lib/plug/static.ex
Outdated
|
||
conn | ||
|> put_resp_header("content-type", content_type) | ||
|> put_resp_header("accept-ranges", "bytes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header should be set for all responses from Plug.Static, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we not support caching or compression for range requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericmj I'm open to working up a variant that supports caching, but I don't think I have the time or knowledge to properly implement compression in this context. Given that the primary use case that I'm familiar with (video playback) applies to content that is already compressed, I'm not sure that adding more compression makes sense.
lib/plug/static.ex
Outdated
file_info(size: file_size) = file_info | ||
|
||
parsed_range = range | ||
|> parse_range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add parenthesis to all function calls.
Also paging @TheSquad which implemented a plug that does this. |
OK, I've added parens and accept-ranges header as requested. Still looking into caching implementation. |
lib/plug/static.ex
Outdated
defp check_bounds({range_start, range_end}, _file_size), do: {range_start, range_end} | ||
|
||
defp to_integer(str) do | ||
{num, _} = Integer.parse(str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can return :error
and that would cause an exception. I would attempt to write it like this:
defp start_and_end("-" <> rest, file_size) do
case Integer.parse(rest) do
{last, ""} -> {file_size - last, file_size - 1}
_ -> :error
end
end
defp start_and_end(range, file_size) do
case Integer.parse(range) do
{first, "-"} ->
{first, file_size - 1}
{first, "-" <> rest} ->
case Integer.parse(rest) do
{last, ""} -> {first, last}
_ -> :error
end
_ ->
:error
end
end
and then in serve_range
you could drive through all of those decisions:
with %{"bytes" => bytes} <- Plug.Conn.Utils.params(range),
{first, last} <- start_and_end(bytes, file_size),
:ok <- check_bounds(first, last) do
send_range(conn, path, range_start, range_end, file_size, segments, options)
else
_ -> serve_static({:ok, conn, file_info, path}, segments, options)
end
Note it uses a check_bounds
implementation that returns :ok
or :error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably add a test that makes sure it doesn't crash if someone sends a bad range such as 00-FF.
@josevalim @ericmj Ping. Ready for re-review. There was a fairly substantial refactor since prior versions to make caching work properly with range requests. |
Thank you @scouten! This is a great addition. |
@josevalim @ericmj thanks for your patient reviews and thanks for accepting this contribution! |
As mentioned in #523, we would like to serve some light static video content through a Phoenix-based web site and are encountering the issue described in http://stackoverflow.com/questions/36576801/serving-http-range-request-with-phoenix.
This PR adds part of the range request protocol defined in RFC 7233 to Plug.Static.
This implementation supports requests specifying a single range of bytes, which appears to be sufficient to enable video playback in Safari. A client may request multiple byte ranges in the same request, but I am not proposing to implement that in this PR.
Submitting now for preliminary / architectural review. IMHO it is not yet ready for merge.
My to do list (I will probably get back to this later in the week):