Fix a number of issues with the Range header handling for serving files

Fix a number of issues with the Range header handling for serving files

* Handling end position after file length
* Handling suffix range longer than file length
* Ignore syntactically invalid Range headers
* Return 416 (Requested range not satisfiable) if start < end

Added tests for these cases.

BUG=http://dartbug.com/21587
R=ajohnsen@google.com, kustermann@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41775

[Anders Johnsen, 2014-11-13 16:12:31]

lgtm, with a suggestion for simplifying the range code.

I guess this is initiated by real world scenarios with bad range headers? :)

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:219: if (range != null) {
This range handling is getting quite complicated. Perhaps we should pull it out
to a separate method, that e.g. returns 'false' if it it didn't end up sending
anything.

[kustermann, 2014-11-14 13:25:27]

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:213: response.close();
Maybe set the content-length header in case of HEAD requests, so people can
figure out how big the file is?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:239: if (end > length) {
Since end is inclusive, shouldn't this be "end >= length" instead of "end >
length"?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:247: return;
Maybe use cascades:

response
    ..statusCode = 
    ..close();

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:256: "bytes
$start-$end/$length");
Cascades?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:101: print([invocation.memberName,
invocation.isGetter, invocation.isSetter, invocation.isMethod,
invocation.isAccessor]);
long line

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:186: String get mockContentBinary =>
_buffer;
That seems wrong. [_buffer] is of type List<int>

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:150: if(_isMockTestExpando[currentTestCase]) {
space after 'if'

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:162: assert(_isMockTestExpando[currentTestCase]
== false);
This assert is not doing anything, If you go into the if(), then it will return
before coming here, right?

Is it just for documentation purposes?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:165: return _getAsBytes(port, path, from, to);
_getAsBytes is returning Future<List<int>> but the type signature of the above
function is

Future<String> getAsBytes()

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:202: new HttpClient()
Who is closing HttpClient here?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:212: new HttpClient()
ditto.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:218: new HttpClient()
ditto

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:528: solo_group('range', () {
Remove the solo_ here, otherwise we loose all other test coverage.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:566: () => test(9, 9),
Add a case for 10 here, for the off-by one issue.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:571:
testVirtualDir('suffix-range', (dir) {
There is a test missing for 'prefix-range', right?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:574:
..writeAsBytesSync(fileContent);
No actual need for cascasde (same above and below).

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:648: var virDir = new
VirtualDirectory(dir.path);
Maybe just move these repeated lines into a setUp() routine?

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:664: }
Maybe extract these redundant lines for getting the bytes/headers/status-code
into a function.

[Søren Gjesse, 2014-11-14 15:27:30]

PTAL

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:213: response.close();
On 2014/11/14 13:25:26, kustermann wrote:
> Maybe set the content-length header in case of HEAD requests, so people can
> figure out how big the file is?

Yes, done. Also added it for serving the whole file - as that was also missing.

Moved the HEAD check to just before serving content to get all the headers.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:219: if (range != null) {
On 2014/11/13 16:12:31, Anders Johnsen wrote:
> This range handling is getting quite complicated. Perhaps we should pull it
out
> to a separate method, that e.g. returns 'false' if it it didn't end up sending
> anything.

I will postpone this to a separate CL, as I will also add support for multiple
ranges.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:239: if (end > length) {
On 2014/11/14 13:25:26, kustermann wrote:
> Since end is inclusive, shouldn't this be "end >= length" instead of "end >
> length"?

Yes. Good catch. Added additional test.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:247: return;
On 2014/11/14 13:25:27, kustermann wrote:
> Maybe use cascades:
> 
> response
>     ..statusCode = 
>     ..close();

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/lib/src/...
pkg/http_server/lib/src/virtual_directory.dart:256: "bytes
$start-$end/$length");
On 2014/11/14 13:25:27, kustermann wrote:
> Cascades?

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:101: print([invocation.memberName,
invocation.isGetter, invocation.isSetter, invocation.isMethod,
invocation.isAccessor]);
On 2014/11/14 13:25:27, kustermann wrote:
> long line

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:186: String get mockContentBinary =>
_buffer;
On 2014/11/14 13:25:27, kustermann wrote:
> That seems wrong. [_buffer] is of type List<int>

Fixed. The beauty of ignoring the analyzer.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:150: if(_isMockTestExpando[currentTestCase]) {
On 2014/11/14 13:25:27, kustermann wrote:
> space after 'if'

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:162: assert(_isMockTestExpando[currentTestCase]
== false);
On 2014/11/14 13:25:27, kustermann wrote:
> This assert is not doing anything, If you go into the if(), then it will
return
> before coming here, right?
> 
> Is it just for documentation purposes?

Honestly don't know. It is in all these methods.

Will remove all in a separate CL.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:165: return _getAsBytes(port, path, from, to);
On 2014/11/14 13:25:27, kustermann wrote:
> _getAsBytes is returning Future<List<int>> but the type signature of the above
> function is
> 
> Future<String> getAsBytes()

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:202: new HttpClient()
On 2014/11/14 13:25:27, kustermann wrote:
> Who is closing HttpClient here?

Nobody. Added close.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:212: new HttpClient()
On 2014/11/14 13:25:27, kustermann wrote:
> ditto.

Ditto.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:218: new HttpClient()
On 2014/11/14 13:25:27, kustermann wrote:
> ditto

Ditto.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:528: solo_group('range', () {
On 2014/11/14 13:25:27, kustermann wrote:
> Remove the solo_ here, otherwise we loose all other test coverage.

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:566: () => test(9, 9),
On 2014/11/14 13:25:27, kustermann wrote:
> Add a case for 10 here, for the off-by one issue.

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:571:
testVirtualDir('suffix-range', (dir) {
On 2014/11/14 13:25:27, kustermann wrote:
> There is a test missing for 'prefix-range', right?

Yes, added.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:574:
..writeAsBytesSync(fileContent);
On 2014/11/14 13:25:27, kustermann wrote:
> No actual need for cascasde (same above and below). 

Done.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:648: var virDir = new
VirtualDirectory(dir.path);
On 2014/11/14 13:25:27, kustermann wrote:
> Maybe just move these repeated lines into a setUp() routine?

Added a prepare method. Cannot use unittest setUp as it need the 'dir'.

https://codereview.chromium.org/721213002/diff/20001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:664: }
On 2014/11/14 13:25:27, kustermann wrote:
> Maybe extract these redundant lines for getting the bytes/headers/status-code
> into a function.

Done.

[kustermann, 2014-11-17 11:15:30]

lgtm

https://codereview.chromium.org/721213002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/721213002/diff/40001/CHANGELOG.md#newcode1
CHANGELOG.md:1: # 0.9.4
This CHANGELOG.md is in the root of the dart directory. Could you modifiy the
one in pkg/http_server/CHANGELOG.md instead?

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/htt...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:192: String get mockContentBinary =>
_buffer;
This is still a string and _buffer is still List<int>

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/uti...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:88: car client = new HttpClient();
car -> var.

Does this even run?

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/vir...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:535: }
Are we leaking these files/directories or will they be cleaned up by some code ?

In case there is no cleanup code, could you add a .whenComplete() on each test
and call a cleanup(dir) method?

[Søren Gjesse, 2014-11-17 13:13:23]

https://codereview.chromium.org/721213002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/721213002/diff/40001/CHANGELOG.md#newcode1
CHANGELOG.md:1: # 0.9.4
On 2014/11/17 11:15:29, kustermann wrote:
> This CHANGELOG.md is in the root of the dart directory. Could you modifiy the
> one in pkg/http_server/CHANGELOG.md instead?

There was no CHANGELOG.md in pkg/http_server. Moved it in there.

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/htt...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/htt...
pkg/http_server/test/http_mock.dart:192: String get mockContentBinary =>
_buffer;
On 2014/11/17 11:15:29, kustermann wrote:
> This is still a string and _buffer is still List<int>

Had an unsaved buffer :-\. Done.

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/uti...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/uti...
pkg/http_server/test/utils.dart:88: car client = new HttpClient();
On 2014/11/17 11:15:30, kustermann wrote:
> car -> var.
> 
> Does this even run?

It does on non-checked mode. Changed to var.

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/vir...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/721213002/diff/40001/pkg/http_server/test/vir...
pkg/http_server/test/virtual_directory_test.dart:535: }
On 2014/11/17 11:15:30, kustermann wrote:
> Are we leaking these files/directories or will they be cleaned up by some code
?
> 
> In case there is no cleanup code, could you add a .whenComplete() on each test
> and call a cleanup(dir) method?

The 'testVirtualDir' used below does the cleanup.

[Søren Gjesse, 2014-11-17 14:21:18]

Committed patchset #4 (id:60001) manually as 41775 (presubmit successful).