Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: add gateway route to daemon #968

Merged
merged 5 commits into from
Sep 8, 2017
Merged

feat: add gateway route to daemon #968

merged 5 commits into from
Sep 8, 2017

Conversation

daviddias
Copy link
Member

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ya7ya and @harshjv !

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

@@ -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')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to call this http module now daemon

Copy link
Contributor

@ya7ya ya7ya Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep http then :)

// const mime = require('mime-types')
// const GatewayResolver = require('../gateway/resolver')
// const PathUtils = require('../gateway/utils/path')
// const Stream = require('stream')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not used please delete.

// }
// })
// }
// }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not used please delete.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it 👍

@daviddias
Copy link
Member Author

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

image

@daviddias
Copy link
Member Author

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.

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
})

const noop = function () {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const resolveDirectory = promisify((ipfs, path, multihash, callback) => {
if (!callback) {
callback = noop
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be replaced by callback = callback || noop

callback = noop
}

mh.validate(mh.fromB58String(multihash))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ipfs
.object
.get(multihash, { enc: 'base58' })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return callback(null, html.build(path, links))
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


gateway.route({
method: '*',
path: '/ipfs/{hash*}',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</body>
</html>
`
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require('./routes')(this.server)
require('./api/routes')(this.server)
// load gateway routes
require('./gateway/routes')(this.server)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

const path = require('path')
const clean = require('../utils/clean')

describe('HTTP GATEWAY', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GATEWAY can be Gateway as it is not an acronym

clean(repoTests)
done()
})
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@daviddias
Copy link
Member Author

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.

log('leaf node: ', currentMultihash)

// TODO: Check if it is a directory by using Unixfs Type, right now
// it won't detect empty dirs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ya7ya ya7ya mentioned this pull request Sep 4, 2017
4 tasks
daviddias pushed a commit that referenced this pull request Sep 5, 2017
* 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
Copy link
Member Author

Travis is really unhappy here.

daviddias pushed a commit that referenced this pull request Sep 6, 2017
* 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
done()
})
})
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ya7ya what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

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.

})

it('stream a large file', (done) => {
let bigFileHash = 'Qme79tX2bViL26vNjPsF3DP1R9rMKMvnPYJiKTTKPrXJjq'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://ipfs.io/ipfs/Qme79tX2bViL26vNjPsF3DP1R9rMKMvnPYJiKTTKPrXJjq

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

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
Copy link
Member Author

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

image

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

http.api = new API(repoPath, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const pathUtil = require('../utils/path')

function getParentDirectoryURL (originalParts) {
const parts = originalParts.splice()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants