feat: add gateway route to daemon #968

daviddias · 2017-08-29T08:09:41Z

daviddias

This is almost there. Right now it needs a cleanup and tests, gateway tests :)

daviddias · 2017-08-29T14:49:21Z

gulpfile.js

@@ -4,7 +4,7 @@ const gulp = require('gulp')
 const parallel = require('async/parallel')
 const series = require('async/series')
 const createTempRepo = require('./test/utils/create-repo-nodejs.js')
-const HTTPAPI = require('./src/http-api')
+const HTTPAPI = require('./src/http')


It makes sense to call this http module now daemon

@diasdavid Yeah it does, but in go-ipfs it's http , so should we stick to the reference implementation ? I don't mind the rename though
go-ipfs

Edit: added link.

Let's keep http then :)

daviddias · 2017-08-29T14:49:38Z

src/http/api/resources/files.js

+// const mime = require('mime-types')
+// const GatewayResolver = require('../gateway/resolver')
+// const PathUtils = require('../gateway/utils/path')
+// const Stream = require('stream')


If not used please delete.

daviddias · 2017-08-29T14:49:58Z

src/http/api/resources/files.js

+//              }
+//            })
+//   }
+// }


If not used please delete.

daviddias · 2017-08-29T14:50:44Z

test/gateway/index.js

+  //
+  //   fs.readdirSync(path.join(__dirname, '/over-ipfs-api'))
+  //     .forEach((file) => require('./over-ipfs-api/' + file)(ctl))
+  // })


Why disabling these tests? They should continue active and passing at all times.

These tests are still active in the http-api tests, but not for the gateway tests. I thought it might be bad to duplicate them.

If it is not used, don't let it comment out. The Gateway requires different kind of tests.

got it 👍

daviddias · 2017-09-04T09:10:38Z

@ya7ya @harshjv awesome work! Just gave it a try run and worked flawlessly 👌🏽

daviddias · 2017-09-04T09:11:53Z

Ok, spoke too soon, got a ton (infinite) errors while downloading a video:

Debug: internal, implementation, error
    Error: write after end
    at writeAfterEnd (_stream_writable.js:234:12)
    at PassThrough.Writable.write (_stream_writable.js:285:5)
    at pull.drain (/Users/imp/code/js-ipfs/src/http/gateway/resources/gateway.js:87:30)
    at /Users/imp/code/js-ipfs/node_modules/pull-stream/sinks/drain.js:24:37
    at drain (/Users/imp/code/js-ipfs/node_modules/stream-to-pull-stream/index.js:141:18)
    at Stream.<anonymous> (/Users/imp/code/js-ipfs/node_modules/stream-to-pull-stream/index.js:150:5)
    at emitOne (events.js:115:13)
    at Stream.emit (events.js:210:7)
    at next (/Users/imp/code/js-ipfs/node_modules/pull-stream-to-stream/index.js:120:13)
    at /Users/imp/code/js-ipfs/node_modules/pull-stream/throughs/map.js:19:9

That said, the streaming didn't stop nor the node crashed.

daviddias

applying as comments as I can push review of a PR I opened

daviddias · 2017-09-04T09:30:40Z

src/http/gateway/resolver.js

+   })
+})
+
+const noop = function () {}


Move to top of the file and replace for function noop () {}

daviddias · 2017-09-04T09:30:57Z

src/http/gateway/resolver.js

+const resolveDirectory = promisify((ipfs, path, multihash, callback) => {
+  if (!callback) {
+    callback = noop
+  }


Can be replaced by callback = callback || noop

daviddias · 2017-09-04T09:31:35Z

src/http/gateway/resolver.js

+    callback = noop
+  }
+
+  mh.validate(mh.fromB58String(multihash))


This means that it won't work with CID. Needs to be using the https://github.com/ipld/js-cid module

daviddias · 2017-09-04T09:32:24Z

src/http/gateway/resolver.js

+
+  ipfs
+   .object
+   .get(multihash, { enc: 'base58' })


I prefer to use the DAG API for this ipfs.dag.get if possible (if it isn't, report as a bug why)

daviddias · 2017-09-04T09:33:47Z

src/http/gateway/resolver.js

+     }
+
+     return callback(null, html.build(path, links))
+   })


Mixing Promises and Callbacks has been historically a recipe for problems. Here the error is not being caught (promise) and the promise is possibly returning before the callback fires.

Let's continue using Callbacks everywhere when possible.

daviddias · 2017-09-04T09:38:04Z

src/http/gateway/routes/gateway.js

+
+  gateway.route({
+    method: '*',
+    path: '/ipfs/{hash*}',


It will be a snap to add /ipld in the future :)

daviddias · 2017-09-04T09:39:10Z

src/http/gateway/utils/html.js

+    </body>
+    </html>
+  `
+}


Ok, so this is the file that generates the Dir preview 👍

Will considering renaming it to /dir-view/index.js and then /dir-view/style.js

daviddias · 2017-09-04T09:41:58Z

src/http/index.js

-          require('./routes')(this.server)
+          require('./api/routes')(this.server)
+          // load gateway routes
+          require('./gateway/routes')(this.server)


daviddias · 2017-09-04T09:42:39Z

test/gateway/index.js

+const path = require('path')
+const clean = require('../utils/clean')
+
+describe('HTTP GATEWAY', () => {


GATEWAY can be Gateway as it is not an acronym

daviddias · 2017-09-04T09:43:01Z

test/gateway/index.js

+      clean(repoTests)
+      done()
+    })
+  })


Does the gateway need the go-ipfs test repo? It would be better to see it working with the default js-ipfs repo.

daviddias · 2017-09-04T09:46:10Z

Pushed my review. Mostly it is structure/cosmetic changes. We do need a test that streams a large file to ensure that is working properly.

Also, currently the tests are all happening in Node.js land. It would be ideal to run them in the browser so that browser things get checked.

One more thing: Currently CORS is being set by default to *, this will have to change.

daviddias · 2017-09-04T10:10:05Z

src/http/gateway/resolver.js

+        log('leaf node: ', currentMultihash)
+
+        // TODO: Check if it is a directory by using Unixfs Type, right now
+        // it won't detect empty dirs


Should be checking to see if it's https://github.com/ipfs/js-ipfs-unixfs/blob/master/src/unixfs.proto.js#L6

Otherwise, it will fail for empty dirs or misbehave for shards.

* feat: add gateway route to daemon * feat: adding Gateway endeavour - improve codebase * clean up, remove commented out lines. (#971) * clean up, remove commented out lines. * cleaning code, removing commented out blocks. * gateway initial tests. * clean up , working tests on node v8.4.0 License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * fix using js-ipfs-repo in gateway tests. License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * Using unix-fs to detect dirs, replacing object.get with DAG.get, CID checks License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * rename checkHash -> checkCID License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * gateway tests: init a fresh repo

daviddias · 2017-09-06T07:42:18Z

Travis is really unhappy here.

* feat: add gateway route to daemon * feat: adding Gateway endeavour - improve codebase * clean up, remove commented out lines. (#971) * clean up, remove commented out lines. * cleaning code, removing commented out blocks. * gateway initial tests. * clean up , working tests on node v8.4.0 License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * fix using js-ipfs-repo in gateway tests. License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * Using unix-fs to detect dirs, replacing object.get with DAG.get, CID checks License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * rename checkHash -> checkCID License: MIT Signed-off-by: Yahya <ya7yaz@gmail.com> * gateway tests: init a fresh repo

daviddias · 2017-09-06T08:05:09Z

test/gateway/index.js

+        done()
+      })
+    })
+  })


I believe that is missing to merge this PR is more tests.

There should be tests for:

Fetching a file with a mime-type that is not text (i.e a pdf or an image)

Loading a webpage (one for each type of index.html)

Loading a webpage from a pagh (QmHASH/a/b/c -> loads the webpage in that path)

And other tests for future PR (I don't want to block this any longer)

CORS tests (make sure config is respected)

@ya7ya what do you think?

@diasdavid I'm working on tests now, It took me a minute to figure out aegir :D and interface-ipfs-core. and i'm trying to follow that to complete the tests for the gateway.

I'll add the tests you mentioned above and check the Error with libp2p.dial you mentioned aswell. I'll try to get this done As soon as possible 👍

daviddias · 2017-09-06T08:06:42Z

Just saw this error running the tests locally:

Debug: internal, implementation, error
    AssertionError [ERR_ASSERTION]: The libp2p node is not started yet
    at Node.dial (/Users/imp/code/js-ipfs/node_modules/libp2p/src/index.js:251:5)
    at libp2p.dial (/Users/imp/code/js-ipfs/node_modules/ipfs-bitswap/src/network.js:167:21)
    at swarm.dial (/Users/imp/code/js-ipfs/node_modules/libp2p/src/index.js:265:18)
    at ms.select (/Users/imp/code/js-ipfs/node_modules/libp2p-swarm/src/dial.js:193:20)
    at f (/Users/imp/code/js-ipfs/node_modules/once/once.js:25:25)
    at select (/Users/imp/code/js-ipfs/node_modules/multistream-select/src/dialer/index.js:76:16)
    at pullLP.decodeFromReader (/Users/imp/code/js-ipfs/node_modules/multistream-select/src/select.js:20:14)
    at Object.reader.read [as cb] (/Users/imp/code/js-ipfs/node_modules/pull-length-prefixed/src/decode.js:88:16)
    at drain (/Users/imp/code/js-ipfs/node_modules/pull-reader/index.js:42:23)
    at /Users/imp/code/js-ipfs/node_modules/pull-reader/index.js:59:18
    at /Users/imp/code/js-ipfs/node_modules/pull-reader/index.js:20:7
    at /Users/imp/code/js-ipfs/node_modules/pull-reader/index.js:110:13
    at onNext (/Users/imp/code/js-ipfs/node_modules/pull-catch/index.js:10:48)
    at drain (/Users/imp/code/js-ipfs/node_modules/stream-to-pull-stream/index.js:141:18)

Weirdly it is not crashing the tests and so it doesn't get detected by CI.

daviddias · 2017-09-07T05:23:14Z

test/gateway/index.js

+    })
+
+    it('stream a large file', (done) => {
+      let bigFileHash = 'Qme79tX2bViL26vNjPsF3DP1R9rMKMvnPYJiKTTKPrXJjq'


@ya7ya where does this hash/file come from? Doesn't seem it got added anywhere

Trying to load it through the gateway ipfs.io, I get:

https://ipfs.io/ipfs/Qme79tX2bViL26vNjPsF3DP1R9rMKMvnPYJiKTTKPrXJjq

@diasdavid thats from the 15mb random file in the fixtures in interface-ipfs-core , I just noticed I forgot to add it to files.add in the before section. So sorry about this.

daviddias · 2017-09-07T05:38:02Z

Ok, got it to pass all tests locally.

Amazing work and effort @ya7ya and @harshjv, thank you so much for taking this on and not letting it go until it was completed 💯

@dignifiedquire @victorbjelkholm @dryajov would you like to do a review before the merge? This is a big PR that has gone through many iterations, having an extra pair of eyeballs reviewing it would be good :)

daviddias · 2017-09-07T06:23:49Z

Seems that although it passes locally (Mac OS X) flawlessly, it always fails on Circle with Timeouts:

daviddias · 2017-09-07T06:46:42Z

test/gateway/index.js

-    http.api = new API(undefined, {
+    const repoPath = os.tmpdir() + '/ipfs-' + Math.random().toString().substring(2, 8) + '-' + Date.now()
+
+    http.api = new API(repoPath, {


Thought this would fix it. Seems it is not the issue.

dignifiedquire · 2017-09-07T07:48:06Z

src/http/gateway/dir-view/index.js

+const pathUtil = require('../utils/path')
+
+function getParentDirectoryURL (originalParts) {
+  const parts = originalParts.splice()


why splice? for getting an array copy we use .slice usually

daviddias commented Aug 29, 2017

View reviewed changes

ya7ya mentioned this pull request Aug 30, 2017

clean up, remove commented out lines. #971

Merged

3 tasks

daviddias force-pushed the feat/gateway branch 3 times, most recently from 2fcbeb8 to dd5a5e6 Compare September 3, 2017 07:46

This was referenced Sep 3, 2017

Next milestone. ipfs-shipyard/ipfs-service-worker-demos#6

Closed

⚡️ v0.26.0 RELEASE 🚀 #986

Closed

daviddias force-pushed the feat/gateway branch from dd5a5e6 to d05591a Compare September 4, 2017 09:06

daviddias commented Sep 4, 2017

View reviewed changes

ya7ya mentioned this pull request Sep 4, 2017

Changes required for #968 feat/Gateway #989

Merged

4 tasks

daviddias force-pushed the feat/gateway branch from 2349f1e to 9dc8c99 Compare September 6, 2017 07:43

daviddias commented Sep 6, 2017

View reviewed changes

This was referenced Sep 6, 2017

Essential Tests for Feat/gateway #997

Closed

[FIXED] Essential Tests for Feat/gateway #999

Merged

daviddias commented Sep 7, 2017

View reviewed changes

daviddias requested review from victorb, dignifiedquire and dryajov September 7, 2017 05:36

daviddias commented Sep 7, 2017

View reviewed changes

dignifiedquire reviewed Sep 7, 2017

View reviewed changes

snyk-bot mentioned this pull request Oct 22, 2022

[Snyk] Security upgrade aegir from 36.2.3 to 37.1.1 devcode1981/js-ipfs#13

Open

baby636 mentioned this pull request May 4, 2023

[Snyk] Security upgrade aegir from 33.2.4 to 37.2.0 baby636/js-ipfs#530

Open

This was referenced Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 baby636/js-ipfs#651

Open

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 baby636/js-ipfs#654

Open

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 baby636/js-ipfs#655

Open

internetbuilder mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade aegir from 34.1.0 to 37.1.1 internetbuilder/js-ipfs#119

Open

feat: add gateway route to daemon #968

feat: add gateway route to daemon #968

Conversation

daviddias commented Aug 29, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ya7ya Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 4, 2017

daviddias commented Sep 4, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 4, 2017

Choose a reason for hiding this comment

daviddias commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 7, 2017

daviddias commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ya7ya Aug 30, 2017 •

edited

Loading