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

pkg/http_server/test/virtual_directory_test.dart

Index: pkg/http_server/test/virtual_directory_test.dart
diff --git a/pkg/http_server/test/virtual_directory_test.dart b/pkg/http_server/test/virtual_directory_test.dart
index bc054622ae426a64522f2452def3aee40c45162f..d57d8a1c272b7275d4b6c2e618dd40aa12a8120e 100644
--- a/pkg/http_server/test/virtual_directory_test.dart
+++ b/pkg/http_server/test/virtual_directory_test.dart
@@ -525,6 +525,153 @@ void main() {
     });
   });
 
+  solo_group('range', () {

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

Remove the solo_ here, otherwise we loose all other test coverage.

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

Done.

+    testVirtualDir('range', (dir) {
+      var fileContent = [0, 1, 2, 3, 4, 5, 6, 7 ,8, 9];
+      new File('${dir.path}/file')
+          ..writeAsBytesSync(fileContent);
+      var virDir = new VirtualDirectory(dir.path);
+
+      Future test(
+          int from, int to, [List<int> expected, String contentRange]) {
+        if (expected == null) {
+          expected = fileContent.sublist(from, to + 1);
+        }
+        if (contentRange == null) {
+          contentRange = 'bytes $from-$to/${fileContent.length}';
+        }
+        return getAsBytes(virDir, '/file', from: from, to: to)
+            .then(expectAsync((result) {
+              expect(result, expected);
+              return getHeaders(virDir, '/file', from: from, to: to)
+                  .then(expectAsync((headers) {
+                    expect(headers[HttpHeaders.CONTENT_RANGE][0],
+                           contentRange);
+                    return getStatusCodeForVirtDir(
+                        virDir, '/file', from: from, to: to)
+                        .then(expectAsync((statusCode) {
+                              expect(statusCode, HttpStatus.PARTIAL_CONTENT);
+                            }));
+                  }));
+            }));
+      }
+
+      return Future.forEach([
+          () => test(0, 0),
+          () => test(0, 1),
+          () => test(1, 2),
+          () => test(1, 9),
+          () => test(0, 9),
+          () => test(8, 9),
+          () => test(9, 9),

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

Add a case for 10 here, for the off-by one issue.

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

Done.

+          () => test(0, 1000, fileContent, 'bytes 0-9/10'),
+      ], (f) => f().then(expectAsync((_) {})));
+    });
+
+    testVirtualDir('suffix-range', (dir) {

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

There is a test missing for 'prefix-range', right?

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

Yes, added.

+      var fileContent = [0, 1, 2, 3, 4, 5, 6, 7 ,8, 9];
+      new File('${dir.path}/file')
+          ..writeAsBytesSync(fileContent);

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

No actual need for cascasde (same above and below). 

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

Done.

+      var virDir = new VirtualDirectory(dir.path);
+
+      Future test(int to, [List<int> expected, String contentRange]) {
+        if (expected == null) {
+          expected = fileContent.sublist(fileContent.length - to,
+                                         fileContent.length);
+        }
+        if (contentRange == null) {
+          contentRange = 'bytes ${fileContent.length - to}-'
+                         '${fileContent.length - 1}/'
+                         '${fileContent.length}';
+        }
+        return getAsBytes(virDir, '/file', to: to)
+            .then(expectAsync((result) {
+              expect(result, expected);
+              return getHeaders(virDir, '/file', to: to)
+                  .then(expectAsync((headers) {
+                    expect(headers[HttpHeaders.CONTENT_RANGE][0],
+                           contentRange);
+                    return getStatusCodeForVirtDir(
+                        virDir, '/file', to: to)
+                        .then(expectAsync((statusCode) {
+                              expect(statusCode, HttpStatus.PARTIAL_CONTENT);
+                            }));
+                  }));
+            }));
+      }
+
+      return Future.forEach([
+          () => test(1),
+          () => test(2),
+          () => test(9),
+          () => test(10),
+          () => test(11, fileContent, 'bytes 0-9/10'),
+          () => test(1000, fileContent, 'bytes 0-9/10')
+      ], (f) => f().then(expectAsync((_) {})));
+    });
+
+    testVirtualDir('unsatisfiable-range', (dir) {
+      var fileContent = [0, 1, 2, 3, 4, 5, 6, 7 ,8, 9];
+      new File('${dir.path}/file')
+          ..writeAsBytesSync(fileContent);
+      var virDir = new VirtualDirectory(dir.path);
+
+      Future test(int from, int to) {
+        return getAsBytes(virDir, '/file', from: from, to: to)
+            .then(expectAsync((result) {
+              expect(result.length, 0);
+              return getHeaders(virDir, '/file', from: from, to: to)
+                  .then(expectAsync((headers) {
+                    expect(headers[HttpHeaders.CONTENT_RANGE], isNull);
+                    return getStatusCodeForVirtDir(
+                        virDir, '/file', from: from, to: to)
+                        .then(expectAsync((statusCode) {
+                              var expectedStatusCode =
+                                  HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE;
+                              expect(statusCode, expectedStatusCode);
+                            }));
+                  }));
+            }));
+      }
+
+      return Future.forEach([
+          () => test(10, 11),
+          () => test(10, 1000),
+          () => test(1000, 1000)
+      ], (f) => f().then(expectAsync((_) {})));
+    });
+
+    testVirtualDir('invalid-range', (dir) {
+      var fileContent = [0, 1, 2, 3, 4, 5, 6, 7 ,8, 9];
+      new File('${dir.path}/file')
+          ..writeAsBytesSync(fileContent);
+      var virDir = new VirtualDirectory(dir.path);

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

Maybe just move these repeated lines into a setUp() routine?

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

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

+
+      Future test(int from, int to) {
+        return getAsBytes(virDir, '/file', from: from, to: to)
+            .then(expectAsync((result) {
+              expect(result, fileContent);
+              return getHeaders(virDir, '/file', from: from, to: to)
+                  .then(expectAsync((headers) {
+                    expect(headers[HttpHeaders.CONTENT_RANGE], isNull);
+                    return getStatusCodeForVirtDir(
+                        virDir, '/file', from: from, to: to)
+                        .then(expectAsync((statusCode) {
+                              expect(statusCode, HttpStatus.OK);
+                            }));
+                  }));
+            }));
+      }

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

Maybe extract these redundant lines for getting the bytes/headers/status-code
into a function.

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

Done.

+
+      return Future.forEach([
+          () => test(1, 0),
+          () => test(10, 0),
+          () => test(1000, 999),
+          () => test(null, 0),  // This is effectively range 10-9.
+      ], (f) => f().then(expectAsync((_) {})));
+    });
+  });
+
   group('error-page', () {
     testVirtualDir('default', (dir) {
         var virDir = new VirtualDirectory(pathos.join(dir.path, 'foo'));