fix: pin type filtering in pin.ls (#2228)

### Summary

This PR fixes filtering, improves interop with go-ipfs and adds missing tests for `pin ls`

### Details

Old version returned indirect pin when `pin ls -t direct <path>` was executed.

This PR also tweaks error handling to match behavior from go-ipfs/js-ipfs-http-client and pass improved interop tests added in ipfs-inactive/interface-js-ipfs-core#375

I've added some inline comments, hope it helps in review.

### Related

- Improved `pin ls` interop tests: ipfs-inactive/interface-js-ipfs-core#375 
  (this PR is not blocked by interop tests, old ones were less strict)
- We need this fix for embedded js-ipfs in Brave 🦁 ipfs/ipfs-companion#716

License: MIT
Signed-off-by: Marcin Rataj <[email protected]>

package.json

@@ -189,7 +189,7 @@
     "execa": "^2.0.3",
     "form-data": "^2.5.0",
     "hat": "0.0.3",
-    "interface-ipfs-core": "^0.105.1",
+    "interface-ipfs-core": "~0.107.3",
     "ipfsd-ctl": "^0.43.0",
     "libp2p-websocket-star": "~0.10.2",
     "ncp": "^2.0.0",

src/core/components/dag.js

@@ -101,7 +101,7 @@ module.exports = function dag (self) {
         if (split.length > 0) {
           path = split.join('/')
         } else {
-          path = '/'
+          path = path || '/'
         }
       } else if (Buffer.isBuffer(cid)) {
         try {
@@ -115,7 +115,7 @@ module.exports = function dag (self) {
         self._preload(cid)
       }
 
-      if (path === undefined || path === '/') {
+      if (path == null || path === '/') {
         self._ipld.get(cid).then(
           (value) => {
             callback(null, {

src/core/components/pin.js

@@ -327,7 +327,7 @@ module.exports = (self) => {
         // check the pinned state of specific hashes
         waterfall([
           (cb) => resolvePath(self.object, paths, cb),
-          (hashes, cb) => mapSeries(hashes, (hash, done) => pin._isPinnedWithType(hash, types.all, done), cb),
+          (hashes, cb) => mapSeries(hashes, (hash, done) => pin._isPinnedWithType(hash, type, done), cb),
           (results, cb) => {
             results = results
               .filter(result => result.pinned)
@@ -348,12 +348,12 @@ module.exports = (self) => {
               })
 
             if (!results.length) {
-              return cb(new Error(`Path is not pinned`))
+              return cb(new Error(`path '${paths}' is not pinned`))
             }
 
             cb(null, results)
           }
-        ], callback)
+        ], (err, results) => err ? callback(err) : callback(null, results)) // we don't want results equal [undefined] when err is present
       } else {
         // show all pinned items of type
         let pins = []

src/http/api/resources/pin.js

@@ -57,7 +57,7 @@ exports.ls = {
     try {
       result = await ipfs.pin.ls(path, { type })
     } catch (err) {
-      throw Boom.boomify(err, { message: 'Failed to list pins' })
+      throw Boom.boomify(err)
     }
 
     return h.response({

test/core/pin.js

@@ -9,6 +9,7 @@ chai.use(dirtyChai)
 
 const fs = require('fs')
 
+const CID = require('cids')
 const IPFS = require('../../src/core')
 const createTempRepo = require('../utils/create-repo-nodejs')
 const expectTimeout = require('../utils/expect-timeout')
@@ -44,14 +45,22 @@ describe('pin', function () {
   let pin
   let repo
 
-  function expectPinned (hash, type, pinned = true) {
+  function expectPinned (hash, type = pinTypes.all, pinned = true) {
     if (typeof type === 'boolean') {
       pinned = type
-      type = undefined
+      type = pinTypes.all
     }
 
-    return pin._isPinnedWithType(hash, type || pinTypes.all)
-      .then(result => expect(result.pinned).to.eql(pinned))
+    return pin._isPinnedWithType(hash, type)
+      .then(result => {
+        expect(result.pinned).to.eql(pinned)
+        if (type === pinTypes.indirect) {
+          // indirect pins return a CID of recursively pinned root instead of 'indirect' string
+          expect(CID.isCID(result.reason)).to.be.true()
+        } else if (type !== pinTypes.all) {
+          expect(result.reason).to.eql(type)
+        }
+      })
   }
 
   async function clearPins () {
@@ -159,6 +168,7 @@ describe('pin', function () {
     it('recursive', function () {
       return pin.add(pins.root)
         .then(() => {
+          expectPinned(pins.root, pinTypes.recursive)
           const pinChecks = Object.values(pins)
             .map(hash => expectPinned(hash))
 
@@ -169,7 +179,7 @@ describe('pin', function () {
     it('direct', function () {
       return pin.add(pins.root, { recursive: false })
         .then(() => Promise.all([
-          expectPinned(pins.root),
+          expectPinned(pins.root, pinTypes.direct),
           expectPinned(pins.solarWiki, false)
         ]))
     })
@@ -242,7 +252,7 @@ describe('pin', function () {
           )
       })
 
-      it('direct', function () {
+      it('all direct', function () {
         return pin.ls({ type: 'direct' })
           .then(out =>
             expect(out).to.deep.include.members([
@@ -252,7 +262,7 @@ describe('pin', function () {
           )
       })
 
-      it('recursive', function () {
+      it('all recursive', function () {
         return pin.ls({ type: 'recursive' })
           .then(out =>
             expect(out).to.deep.include.members([
@@ -262,7 +272,7 @@ describe('pin', function () {
           )
       })
 
-      it('indirect', function () {
+      it('all indirect', function () {
         return pin.ls({ type: 'indirect' })
           .then(out =>
             expect(out).to.deep.include.members([
@@ -275,6 +285,78 @@ describe('pin', function () {
             ])
           )
       })
+
+      it('direct for CID', function () {
+        return pin.ls(pins.mercuryDir, { type: 'direct' })
+          .then(out =>
+            expect(out).to.have.deep.members([
+              { type: 'direct',
+                hash: pins.mercuryDir }
+            ])
+          )
+      })
+
+      it('direct for path', function () {
+        return pin.ls(`/ipfs/${pins.root}/mercury/`, { type: 'direct' })
+          .then(out =>
+            expect(out).to.have.deep.members([
+              { type: 'direct',
+                hash: pins.mercuryDir }
+            ])
+          )
+      })
+
+      it('direct for path (no match)', function (done) {
+        pin.ls(`/ipfs/${pins.root}/mercury/wiki.md`, { type: 'direct' }, (err, pinset) => {
+          expect(err).to.exist()
+          expect(pinset).to.not.exist()
+          done()
+        })
+      })
+
+      it('direct for CID (no match)', function (done) {
+        pin.ls(pins.root, { type: 'direct' }, (err, pinset) => {
+          expect(err).to.exist()
+          expect(pinset).to.not.exist()
+          done()
+        })
+      })
+
+      it('recursive for CID', function () {
+        return pin.ls(pins.root, { type: 'recursive' })
+          .then(out =>
+            expect(out).to.have.deep.members([
+              { type: 'recursive',
+                hash: pins.root }
+            ])
+          )
+      })
+
+      it('recursive for CID (no match)', function (done) {
+        return pin.ls(pins.mercuryDir, { type: 'recursive' }, (err, pinset) => {
+          expect(err).to.exist()
+          expect(pinset).to.not.exist()
+          done()
+        })
+      })
+
+      it('indirect for CID', function () {
+        return pin.ls(pins.solarWiki, { type: 'indirect' })
+          .then(out =>
+            expect(out).to.have.deep.members([
+              { type: `indirect through ${pins.root}`,
+                hash: pins.solarWiki }
+            ])
+          )
+      })
+
+      it('indirect for CID (no match)', function (done) {
+        pin.ls(pins.root, { type: 'indirect' }, (err, pinset) => {
+          expect(err).to.exist()
+          expect(pinset).to.not.exist()
+          done()
+        })
+      })
     })
   })