MAID-2488 feat/webFetch: impl multipart range requests #223

hunterlester · 2018-04-27T23:17:19Z

If a multipart range is desired, the range value may be an array instead of an object.

const range = [
        {
          start: 2,
          end: 4
        },
        {
          start: 7,
          end: 9,
        },
        {
          start: 13,
         end: 17,
        },
];

What is returned is still an object with headers and body expect the body is an array of objects with headers and body properties:

{
  "headers": {
       "Content-Type": "multipart/byteranges",
       "Content-Length": 200
   }.
   body: [
       {
           "headers": {
                "Content-Type: "video/mp4,
                "Content-Range":  bytes 4-15/200
            },
            body: <Buffer>
       },
        {
           "headers": {
                "Content-Type: "video/mp4,
                "Content-Range":  bytes 40-99/200
            },
            body: <Buffer>
       },
        {
           "headers": {
                "Content-Type: "video/mp4,
                "Content-Range":  bytes 150-175/200
            },
            body: <Buffer>
       },
   ]
}

This seemed most appropriate way to structure response based on https://stackoverflow.com/questions/41348421/how-to-properly-parse-multipart-byteranges-responses-using-typescript-javascript, https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests, and node-formidable/formidable#390

coveralls · 2018-04-28T17:22:36Z

Coverage decreased (-0.03%) to 91.828% when pulling e88c5d8 on hunterlester:multipart into dadd895 on maidsafe:master.

coveralls · 2018-04-28T17:22:36Z

Coverage increased (+0.3%) to 93.176% when pulling 03dba25 on hunterlester:multipart into 3631731 on maidsafe:master.

bochaco · 2018-05-01T16:41:56Z

test/browsing.js

-        ));
+      const domain = await createRandomDomain(content, '/streaming.mp4');
+      const app = await createAnonTestApp();
+      return should(app.webFetch(`safe://${domain}/streaming.mp4`, { range: { start: 0, end: content.length + 5 } }))


I wouldn't change the end value to content.length + 5 as the test is trying to make sure that the very first byte out of range gets properly picked up as incorrect, that's where usually the issues are.

bochaco · 2018-05-01T16:47:19Z

test/browsing.js

@@ -93,6 +93,14 @@ describe('Browsing', () => {
      ));
  }).timeout(20000);

+  it('fetches file with non-explicit mime type', async () => {


What's the difference between this test and the test on line 193 named /index.html ?
Also, what do you mean by non-explicit mime type for this test?

This test differs with the one on line 193 by creating a file without an extension, so a mimetype cannot be detected.

We could then add the validation for the mime types to the tests

bochaco · 2018-05-01T17:08:00Z

src/app.js

@@ -168,9 +168,6 @@ class SAFEApp extends EventEmitter {
    if (!url) return Promise.reject(makeError(errConst.MISSING_URL.code, errConst.MISSING_URL.msg));

    const parsedUrl = parseUrl(url);
-    if (!parsedUrl) {


Why this is not needed anymore?
Also, we need to update the documentation as it seems it now accepts either an object or an array.

This line isn't needed anymore because !url is already handled on line 168. If there is an error parsing the url it will be handled by the the parseUrl function.

What is it that accepts an object or an array? The webFetch function for it's url parameter? I just see that we handle objects for options parameter.

When you say it will be handled by parseUrl what do you mean? if the url is not null but is an invalid url, what will happen?
The url is a string.

Currently Promise.reject(makeError(errConst.MISSING_URL.code, errConst.MISSING_URL.msg)); is returned if !parseUrl, but this condition can never be met because Node's url.parse function will throw an error if no URL string is passed to it and also the function doesn't consider any string to be an invalid URL, it will simply return an URL object.

bochaco · 2018-05-01T17:15:55Z

src/app.js

+          data = await openedFile.read(start, lengthToRead);
+        }
+
+        const mimeType = multipart ? 'multipart/byteranges' : false


Do we need the false in the boolean operation?
I would rather break this into and if/else as it's a bit confusing and error prone.

bochaco · 2018-05-01T17:17:32Z

src/app.js

+            const partLengthToRead = (partEnd - partStart) + 1; // account for 0 index
+            const byteSegment = await openedFile.read(partStart, partLengthToRead);
+            const partEndByte = (partEnd === fileSize - 1) ? fileSize - 1 : partEnd;
+            const mimeType = mime.getType(nodePath.extname(filePath))


It seems you don't need this within the iteration

The mimeType variable? That should be different from the parent scope mimeType.
The parent response header mimeType is multipart/byteranges while the byte range parts should have a mime type that corresponds to the file which they belong.

But the value you are setting to mimeType is always the same as it doesn't depend on any of the values in each iteration, right?

bochaco · 2018-05-01T17:26:07Z

test/browsing.js

@@ -326,6 +333,99 @@ describe('Browsing', () => {
            .be.rejectedWith('Invalid range start value')
        ));
    }); // .timeout(20000);
+
+    it('fetches multipart ranges', async () => {


We should be testing the headers as we do for single range tests.

bochaco · 2018-05-01T17:31:06Z

Perhaps we can return the array with headers and bodies but with an attributed called parts rather than body, it seems that's closer to what happens in HTTP since the repose contains multiple parts, e.g.:

{
  "headers": {
       "Content-Type": "multipart/byteranges",
       "Content-Length": 200
   }.
   parts: [
       {
           "headers": {
                "Content-Type: "video/mp4,
                "Content-Range":  bytes 4-15/200
            },
            body: <Buffer>
       },
       ...
   ]
}

hunterlester · 2018-05-01T22:46:28Z

@bochaco Yeah, I wasn't sure about this because I can't find documentation about how to encode multipart byte ranges with JSON: https://developer.mozilla.org/en-US/docs/Web/API/Response

bochaco · 2018-05-02T09:50:20Z

src/app.js

-
+          range = options.range;
+          multipart = range.length > 1;
+          start = options.range.start || consts.pubConsts.NFS_FILE_START;


So are we making the start optional as well? if so we'd need to add tests for that

MAID-2488 feat/webFetch: impl multipart range requests

13ca8c6

test/webFetch: adds tests for multipart partial content request

81dc410

hunterlester changed the title ~~[WIP] MAID-2488 feat/webFetch: impl multipart range requests~~ MAID-2488 feat/webFetch: impl multipart range requests Apr 28, 2018

hunterlester mentioned this pull request Apr 30, 2018

MAID-2488 feat/webFetch: impl multipart range requests joshuef/peruse#80

Merged

bochaco suggested changes May 1, 2018

View reviewed changes

bochaco reviewed May 2, 2018

View reviewed changes

hunterlester and others added 3 commits May 4, 2018 09:12

fix/review: addresses PR review

2b8d638

Merge branch 'master' into multipart

6cc244f

Merge branch 'master' into multipart

069dfae

bochaco approved these changes May 16, 2018

View reviewed changes

Merge branch 'master' into multipart

03dba25

bochaco merged commit 4644583 into maidsafe-archive:master May 16, 2018

hunterlester deleted the multipart branch June 6, 2018 18:04

MAID-2488 feat/webFetch: impl multipart range requests #223

MAID-2488 feat/webFetch: impl multipart range requests #223

Uh oh!

Conversation

hunterlester commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 28, 2018

Uh oh!

coveralls commented Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hunterlester May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bochaco commented May 1, 2018

Uh oh!

hunterlester commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hunterlester commented Apr 27, 2018 •

edited

Loading

coveralls commented Apr 28, 2018 •

edited

Loading

hunterlester May 1, 2018 •

edited

Loading