Fix/files get #499

dignifiedquire · 2016-09-16T16:39:53Z

Refactor cli tests to reduce code duplication and increase covered test cases. Also better failure mode, showing stderr and stdin when a cli tests fails.
Fix bugs revealed due to better cli tests in
- files get
- files cat
- block stat
- bootstrap add
- bootstrap rm

dignifiedquire · 2016-09-22T10:13:23Z

This can be reviewed for now, no need to wait until we fix the bitswap issues.

daviddias · 2016-09-22T13:52:54Z

@dignifiedquire can you add more description. It says that it fixes files.get. What was found, what was achieved? Looks like there is some solid progress though, 👍

dignifiedquire · 2016-09-22T14:10:34Z

@diasdavid added a description on what happened^^

daviddias · 2016-09-23T09:42:35Z

thank you @dignifiedquire, left a bunch of comments :)

dignifiedquire · 2016-09-23T11:52:06Z

thank you @dignifiedquire, left a bunch of comments :)

not seeing any in this PR

daviddias · 2016-09-22T13:53:10Z

package.json

@@ -43,6 +43,7 @@
    "aegir": "^8.0.1",
    "buffer-loader": "0.0.1",
    "chai": "^3.5.0",
+    "execa": "^0.4.0",


what's this for?

To spawn the cli in a consistent and nice way.

daviddias · 2016-09-22T13:54:01Z

src/cli/commands/bitswap/stat.js

@@ -23,8 +23,7 @@ module.exports = {
        stats.Wantlist = stats.Wantlist || []
        stats.Peers = stats.Peers || []

-        console.log(`
-bitswap status
+        console.log(`bitswap status


Are we happy with the new "Printer" by @victorbjelkholm? Wanna use that?

As I mentioned in another PR I would rather do that change as a single PR on its own, there are already too many things mixed in here.

daviddias · 2016-09-22T13:54:24Z

src/cli/commands/block/stat.js

@@ -24,7 +23,7 @@ module.exports = {
          throw err
        }

-        console.log('Key:', bs58.encode(stats.key).toString())
+        console.log('Key:', stats.key)


Isn't this a multihash?

Not currently

Right https://github.com/ipfs/interface-ipfs-core/tree/master/API/block#stat :)

daviddias · 2016-09-22T13:56:32Z

src/cli/commands/files/cat.js

      if (err) {
        throw err
      }

-      ipfs.files.cat(path, onFile)
+      file.pipe(process.stdout)


These are not changes to get

I believe we have agreed during a js-ipfs sprint call that we would start settling on async, since not is is modular, more battle tested and the documentation is just great. If we bring on new deps like run-waterfall, better just do the async/waterfall

I don't remember making that decision, but happy to move all these run-* to modular async.

These are not changes to get

As mentioned in the description, fixes all over including files cat ;)

daviddias · 2016-09-22T13:57:14Z

src/cli/commands/files/get.js

      const dirPath = path.join(dir, file.path)
      // Check to see if the result is a directory
-      if (file.dir === false) {
+      if (file.content) {


daviddias · 2016-09-22T14:07:07Z

src/http-api/routes/bootstrap.js

-        query: {
-          arg: Joi.string().required(), // multiaddr to add
-          default: Joi.boolean()
-        }


Why remove the Joi validation?

It is the only place in the http api where we have validation.

It was incorrect as it threw errors on the global stream header.

It is the only place in the http api where we have validation

It was a start, a good practice introduced by @xicombd.

It was incorrect as it threw errors on the global stream header.

Why exactly? That sounds like a Joi bug, this is only checking the query param

I don't know why, but it wouldn't allow for any additional query parameters to be passed along except for the ones that were part of this validation.

IRC log

11:04 <@daviddias> I don't believe we can break the user API in multihashing, have been thinking about it and the best option ( given that having both API would multiply deps) is to have multihashing-async module 11:04 <@daviddias> And recommend devs to use it in multihashing, explaining why 11:14 <@daviddias> On the first PR, my last comment is to Lee Joi validation, if there are more optional Args, then so be it :) 11:14 <@daviddias> s/Lee/lets keep 11:17 <@daviddias> @dignifiedquire: ^ :) 11:18 <dignifiedquire> so you mean creating a new module for multihashing? 11:20 <dignifiedquire> if we recommend devs to use the new version, we can just do a major version bump, with a changelog, that way everyone who wants to not refactor their code can stay on the old version, and we don't have to maintain to different versions 11:21 <dignifiedquire> daviddias: re Joi validation as I said in my comment I wasn't able to get that working, Joi would always throw if js-ipfs-api passed it something it didn't know 11:26 <dignifiedquire> daviddias: ^^ 12:17 <@daviddias> dignifiedquire: ACK, what I meant is: the args that will be passed are defined on the spec 12:18 <@daviddias> so we can list them

Actually, the offending parameter is not documented in the HTTP API: http://docs.ipfs.apiary.io/#reference/bootstrap/add/add does not list stream-channels, which is always set to true by js-ipfs-api: https://github.com/ipfs/js-ipfs-api/blob/master/src/request-api.js#L92

Let's add that as an issue in the http spec and add that rule here

daviddias · 2016-09-22T14:07:22Z

src/http-api/routes/bootstrap.js

-          arg: Joi.string().required(), // multiaddr to rm
-          all: Joi.boolean()
-        }
-      }


same as above

daviddias · 2016-09-22T14:07:58Z

test/cli/test-bitswap.js

 const bs58 = require('bs58')

 const HttpAPI = require('../../src/http-api')
 const createTempNode = require('../utils/temp-node')
 const repoPath = require('./index').repoPath
+const ipfs = require('../utils/ipfs')(repoPath)


Is there a better name for that ipfs file?

I couldn't come up with one, happy to hear suggestions.

ipfs-exec or ipfs-run?

I like ipfs-exec

daviddias · 2016-09-22T14:09:36Z

test/cli/test-bitswap.js

-        })
+      return ipfs('bitswap wantlist').then((out) => {
+        expect(out).to.be.eql(key)
+      })


This should be happening inside of the callback of the .inject

Look closely, the inject call is just so we get the right response, but the actual command shouldn't happen inside, otherwise it would hang forever.

daviddias · 2016-09-22T14:11:17Z

test/cli/test-bitswap.js

-          done()
-        })
+    it('stat', () => {
+      return ipfs('bitswap stat').then((out) => {


In this new CLI library, if an exit code is different than 0, where do we get that error? Also, where is stderr?

If you look into test/utils/ipfs.js you see it asserts that the exitcode is 0 and that stderr is ''. It returns a promise and will reject that promise when an error happens which will in turn display the failed things like exitcode and stderr.

stderr might not always be ''.

If we want the command to success it will, for when you want it to fail, there is ipfs.fail('my cmd') which expects failure and allows you to assert agains stderr

sweet!

Can you document all of that in that ipfs-exec module? :)

daviddias · 2016-09-24T01:31:55Z

@dignifiedquire, my bad, I used the review tool and forgot to click submit

daviddias · 2016-09-30T06:27:27Z

Seems that Joi is the last bit fo make this happen:

Fix/files get #499 (comment)

Also, want to squash commits?

dignifiedquire · 2016-09-30T09:00:29Z

Also, want to squash commits?

When CI is green and you gave your thumbs up

daviddias · 2016-09-30T10:30:51Z

@dignifiedquire rebased, tests pass on CI, but fail locally:

  1) HTTP API ## interface-ipfs-core tests over ipfs-api .files callback API .add add a nested dir as array:

      Uncaught AssertionError: expected 'test-folder/files' to equal 'test-folder'
      + expected - actual

      -test-folder/files
      +test-folder

      at node_modules/interface-ipfs-core/lib/files.js:152:34

Does it work on your machine?

dignifiedquire · 2016-09-30T10:49:14Z

@diasdavid try running them again, they pass for me

daviddias · 2016-09-30T13:46:39Z

Got them running (had to do 2 fresh npm installs)

- Change bullet formatting to make it possible to use the markdown headers to link specifically to the MFS portion of this document. - Link out from MFS section to the ProtoSchool tutorial as part of Q3 IPFS Docs initiative (see issue ProtoSchool/protoschool.github.io#260)

dignifiedquire added the status/in-progress In progress label Sep 16, 2016

dignifiedquire mentioned this pull request Sep 16, 2016

bitswap is 'requesting one block at a time' #498

Closed

dignifiedquire self-assigned this Sep 19, 2016

dignifiedquire mentioned this pull request Sep 21, 2016

[WIP] Improve interop ipfs/js-ipfs-bitswap#39

Closed

dignifiedquire added the needs review label Sep 22, 2016

dignifiedquire removed their assignment Sep 22, 2016

dignifiedquire changed the title ~~[WIP] Fix/files get~~ Fix/files get Sep 23, 2016

daviddias reviewed Sep 24, 2016

View reviewed changes

dignifiedquire force-pushed the fix/files-get branch from 2ccd14d to e8b2c3c Compare September 25, 2016 18:35

daviddias removed the needs review label Sep 26, 2016

dignifiedquire added 4 commits September 30, 2016 11:13

fix(files.get): fix the command

7015586

test: fix current tests, add more cli tests

5fe4674

refactor: replace run-* with async, ipfs->ipfs-exec

8509199

feat(http-api): add joi validation to bootstrap

028a98c

daviddias force-pushed the fix/files-get branch from 16e48f6 to 028a98c Compare September 30, 2016 10:14

daviddias merged commit c8dbf6b into master Sep 30, 2016

daviddias deleted the fix/files-get branch September 30, 2016 13:46

daviddias removed the status/in-progress In progress label Sep 30, 2016

This was referenced Apr 7, 2022

[Snyk] Fix for 1 vulnerabilities rtpro/js-ipfs#53

Open

[Snyk] Fix for 1 vulnerabilities thexdesk/js-ipfs#14

Open

[Snyk] Fix for 1 vulnerabilities thexdesk/js-ipfs#15

Open

awaisab172 mentioned this pull request Jun 21, 2023

[Snyk] Security upgrade libp2p from 0.26.2 to 0.27.0 awaisab172/js-ipfs#57

Open

Fix/files get #499

Fix/files get #499

Conversation

dignifiedquire commented Sep 16, 2016 • edited Loading

dignifiedquire commented Sep 22, 2016

daviddias commented Sep 22, 2016 • edited Loading

dignifiedquire commented Sep 22, 2016

daviddias commented Sep 23, 2016

dignifiedquire commented Sep 23, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 24, 2016

daviddias commented Sep 30, 2016 • edited by dignifiedquire Loading

dignifiedquire commented Sep 30, 2016

daviddias commented Sep 30, 2016 • edited Loading

dignifiedquire commented Sep 30, 2016

daviddias commented Sep 30, 2016

dignifiedquire commented Sep 16, 2016 •

edited

Loading

daviddias commented Sep 22, 2016 •

edited

Loading

dignifiedquire Sep 24, 2016 •

edited

Loading

daviddias Sep 28, 2016 •

edited

Loading

daviddias commented Sep 30, 2016 •

edited by dignifiedquire

Loading

daviddias commented Sep 30, 2016 •

edited

Loading