fix: pin type filtering in pin.ls #2228
Conversation
Old version returned indirect pin when `pin ls -t direct <path>` was executed. This fixes filtering and adds necessary tests. It also tweaks error handling to match behavior from go-ipfs/js-ipfs-http-client and pass interop tests from ipfs-inactive/interface-js-ipfs-core#375 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
| @@ -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), | |||
There was a problem hiding this comment.
This is the fix for filtering 🙃
| @@ -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`)) | |||
There was a problem hiding this comment.
Changed to match error from go-ipfs
Interop test for this is added in https://github.com/ipfs/interface-js-ipfs-core/pull/375/files#diff-6a00eb1307f801bb37999815584589bdR190
| @@ -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) | |||
There was a problem hiding this comment.
Removed message prefix to match error from go-ipfs.
Interop test for this is added in https://github.com/ipfs/interface-js-ipfs-core/pull/375/files#diff-6a00eb1307f801bb37999815584589bdR199
| @@ -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) { | |||
There was a problem hiding this comment.
expectPinned was not checking if returned type match requested one, so I've added double-check for that.
|
|
||
| it('direct for path (no match)', function (done) { | ||
| pin.ls(`/ipfs/${pins.root}/mercury/wiki.md`, { type: 'direct' }, (err, pinset) => { | ||
| expect(err).to.exist() |
There was a problem hiding this comment.
To make it easier to maintain the error message is not hardcoded here, but checked in interop tests:
|
@alanshaw ok to merge this and ipfs-inactive/interface-js-ipfs-core#375 before the fix gets old again? :) |
This PR adds more detailed tests for `pin.ls`, including one that guards against issue described in ipfs-inactive/js-ipfs-http-client#875 refs ipfs/js-ipfs#2228 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
Summary
This PR fixes filtering, improves interop with go-ipfs and adds missing tests for
pin lsDetails
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
pin lsinterop tests: fix: pin.ls ignored opts when hash was present ipfs-inactive/interface-js-ipfs-core#375(this PR is not blocked by interop tests, old ones were less strict)